SONARPY-2303 Stop running V1 type inference during the indexing phase #2131

guillaume-dequenne-sonarsource · 2024-11-05T10:53:37Z

SONARPY-2303

sonarqube-next · 2024-11-06T16:35:58Z

Quality Gate failed

Failed conditions
17 New issues
85.6% Coverage on New Code (required ≥ 95%)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

guillaume-dequenne-sonarsource · 2024-12-05T13:38:25Z

python-frontend/src/main/java/org/sonar/plugins/python/api/PythonInputFileContext.java

@@ -59,7 +64,7 @@ public CacheContext cacheContext() {

  @Beta
  public Collection<Symbol> stubFilesSymbols() {
-    return TypeShed.stubFilesSymbols();
+    return projectLevelSymbolTable.typeShedDescriptorsProvider().stubFilesSymbols(projectLevelSymbolTable);


Note: I notice it's a bit weird to pass the projectLevelSymbolTable to the typeShedDescriptorsProvider when it is itself a part of the projectLevelSymbolTable. However, given it is code meant to disappear after we finish the removal of the V1 engine, I think it's okay to keep as it is.

why not delegate this method inside the ProjectLevelSymbolTable to hide it behind the scenes?

maksim-grebeniuk-sonarsource · 2024-12-06T09:25:58Z

...rontend/src/main/java/org/sonar/python/semantic/v2/typeshed/TypeShedDescriptorsProvider.java

+   * Returns stub symbols to be used by SonarSecurity.
+   * Ambiguous symbols that only contain class symbols are disambiguated with latest Python version.
+   */
+  public Collection<Symbol> stubFilesSymbols(ProjectLevelSymbolTable projectLevelSymbolTable) {


I think that the Descriptors provider should not operate with the Symbol model, it should just provide descriptors. The conversion from the descriptor to the symbol model is on the projectLevelSymbolTable side

maksim-grebeniuk-sonarsource · 2024-12-06T09:39:42Z

python-frontend/src/main/java/org/sonar/python/index/DescriptorUtils.java

      default:
        throw new IllegalStateException(String.format("Error while creating a Symbol from a Descriptor: Unexpected descriptor kind: %s", descriptor.kind()));
    }
  }

+  private static Descriptor recreateDescriptorFromAlias(AliasDescriptor aliasDescriptor) {
+    return switch (aliasDescriptor.originalDescriptor().kind()) {


if replace this kind switch-case with just instanceof if-else - it will allow to get rid of unsafe class casts in the recreateFunctionDescriptor and the recreateClassDescriptor methods like:

private static Descriptor recreateDescriptorFromAlias(AliasDescriptor aliasDescriptor) { if (aliasDescriptor.originalDescriptor() instanceof FunctionDescriptor functionDescriptor) { return recreateFunctionDescriptor(aliasDescriptor, functionDescriptor); } else if (aliasDescriptor.originalDescriptor() instanceof ClassDescriptor classDescriptor) { return recreateClassDescriptor(aliasDescriptor, classDescriptor); } throw new IllegalStateException(String.format("Error while recreating a descriptor from an alias: Unexpected alias kind: %s", aliasDescriptor.kind())); }

thomas-serre-sonarsource

LGTM! Great job!!
Thanks for rewriting the git history; it was appreciated for the review.

I left mainly questions but no show-stopper.
I put it back in progress for you to answer them.

thomas-serre-sonarsource · 2024-12-06T08:20:06Z

python-frontend/src/main/java/org/sonar/python/semantic/ProjectLevelSymbolTable.java

    var typesBySymbol = typeInferenceV2.inferTypes(fileInput);
+


Suggested change

thomas-serre-sonarsource · 2024-12-06T08:20:14Z

python-frontend/src/main/java/org/sonar/python/semantic/ProjectLevelSymbolTable.java

    importsByModule.put(fullyQualifiedModuleName, typeInferenceV2.importedModulesFQN());
+


Suggested change

python-frontend/src/main/java/org/sonar/python/semantic/v2/BasicTypeTable.java

...c/main/java/org/sonar/python/semantic/v2/converter/AliasDescriptorToPythonTypeConverter.java

thomas-serre-sonarsource · 2024-12-06T09:21:53Z

...rontend/src/main/java/org/sonar/python/semantic/v2/typeshed/TypeShedDescriptorsProvider.java

+  public Collection<Symbol> stubFilesSymbols(ProjectLevelSymbolTable projectLevelSymbolTable) {
+    if (cachedSymbols != null) {
+      return cachedSymbols;
+    }
+    Collection<Descriptor> allDescriptors = new ArrayList<>(stubFilesDescriptors());
+    allDescriptors.addAll(builtinDescriptors().values());
+    record Pair(Symbol symbol, Descriptor descriptor){}
+
+    Map<String, Pair> symbolsByFqn = new HashMap<>();
+    cachedSymbols = new HashSet<>();
+
+    for (Descriptor descriptor : allDescriptors) {
+      if (descriptor.fullyQualifiedName() != null) {
+        Pair pair = symbolsByFqn.computeIfAbsent(descriptor.fullyQualifiedName(), k ->
+          new Pair(DescriptorUtils.symbolFromDescriptor(descriptor, projectLevelSymbolTable, null, new HashMap<>(), new HashMap<>()), descriptor));
+        cachedSymbols.add(pair.symbol());
+      }
+    }
+    return cachedSymbols;
+  }


Why do you have to introduce a record here?

Suggested change

public Collection<Symbol> stubFilesSymbols(ProjectLevelSymbolTable projectLevelSymbolTable) {

if (cachedSymbols != null) {

return cachedSymbols;

}

Collection<Descriptor> allDescriptors = new ArrayList<>(stubFilesDescriptors());

allDescriptors.addAll(builtinDescriptors().values());

record Pair(Symbol symbol, Descriptor descriptor){}

Map<String, Pair> symbolsByFqn = new HashMap<>();

cachedSymbols = new HashSet<>();

for (Descriptor descriptor : allDescriptors) {

if (descriptor.fullyQualifiedName() != null) {

Pair pair = symbolsByFqn.computeIfAbsent(descriptor.fullyQualifiedName(), k ->

new Pair(DescriptorUtils.symbolFromDescriptor(descriptor, projectLevelSymbolTable, null, new HashMap<>(), new HashMap<>()), descriptor));

cachedSymbols.add(pair.symbol());

}

}

return cachedSymbols;

}

public Collection<Symbol> stubFilesSymbols(ProjectLevelSymbolTable projectLevelSymbolTable) {

if (cachedSymbols != null) {

return cachedSymbols;

}

Collection<Descriptor> allDescriptors = new ArrayList<>(stubFilesDescriptors());

allDescriptors.addAll(builtinDescriptors().values());

Map<String, Symbol> symbolsByFqn = new HashMap<>();

cachedSymbols = new HashSet<>();

for (Descriptor descriptor : allDescriptors) {

if (descriptor.fullyQualifiedName() != null) {

Symbol symbol = symbolsByFqn.computeIfAbsent(descriptor.fullyQualifiedName(), k -> DescriptorUtils.symbolFromDescriptor(descriptor, projectLevelSymbolTable, null, new HashMap<>(), new HashMap<>()));

cachedSymbols.add(symbol);

}

}

return cachedSymbols;

}

I don't really remember why I needed that in a previous iteration...but certainly I don't anymore, good catch!

sonar-python-plugin/src/main/java/org/sonar/plugins/python/indexer/SonarQubePythonIndexer.java

python-frontend/src/main/java/org/sonar/python/semantic/ProjectLevelSymbolTable.java

thomas-serre-sonarsource · 2024-12-06T09:47:23Z

python-frontend/src/main/java/org/sonar/python/index/DescriptorUtils.java

      default:
        throw new IllegalStateException(String.format("Error while creating a Symbol from a Descriptor: Unexpected descriptor kind: %s", descriptor.kind()));
    }
  }

+  private static Descriptor recreateDescriptorFromAlias(AliasDescriptor aliasDescriptor) {


I think there are no unit tests for this function, especially in the case that the original descriptor is not a class or a function.

And actually there was a bug in the logger, as I was logging the type of the AliasDescriptor instead of the original descriptor, good catch!

thomas-serre-sonarsource

LGTM! Well done 💪 🚀

sonarqube-next · 2024-12-06T15:20:43Z

Quality Gate passed

Issues
0 New issues
3 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
97.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

guillaume-dequenne-sonarsource changed the title ~~SONARPY-2303~~ SONARPY-2303 Stop running V1 type inference during the indexing phase Nov 5, 2024

maksim-grebeniuk-sonarsource force-pushed the MMF-3945 branch from 20f960f to 8db40ed Compare November 5, 2024 12:44

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 2 times, most recently from 0be3f9a to 6e6e789 Compare November 5, 2024 14:39

guillaume-dequenne-sonarsource force-pushed the MMF-3945 branch 2 times, most recently from 6152f29 to cd4f1a6 Compare November 6, 2024 08:25

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 3 times, most recently from 4ca0b3f to 1759e47 Compare November 6, 2024 09:15

guillaume-dequenne-sonarsource force-pushed the MMF-3945 branch 2 times, most recently from 7fe14fd to b6d1e22 Compare November 6, 2024 15:58

Base automatically changed from MMF-3945 to master November 6, 2024 16:31

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 17 times, most recently from 24cf2b8 to 344bd13 Compare December 4, 2024 15:02

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 6 times, most recently from d3b2340 to b880c3a Compare December 5, 2024 11:13

guillaume-dequenne-sonarsource requested a review from thomas-serre-sonarsource December 5, 2024 12:57

guillaume-dequenne-sonarsource commented Dec 5, 2024

View reviewed changes

guillaume-dequenne-sonarsource marked this pull request as ready for review December 6, 2024 08:51

maksim-grebeniuk-sonarsource reviewed Dec 6, 2024

View reviewed changes

thomas-serre-sonarsource requested changes Dec 6, 2024

View reviewed changes

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 3 times, most recently from 6ccfe06 to ee46413 Compare December 6, 2024 13:14

guillaume-dequenne-sonarsource requested a review from thomas-serre-sonarsource December 6, 2024 13:21

thomas-serre-sonarsource approved these changes Dec 6, 2024

View reviewed changes

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch 2 times, most recently from 335718a to d01706f Compare December 6, 2024 14:14

guillaume-dequenne-sonarsource enabled auto-merge (rebase) December 6, 2024 15:09

guillaume-dequenne-sonarsource disabled auto-merge December 6, 2024 15:09

guillaume-dequenne-sonarsource added 4 commits December 6, 2024 16:15

SONARPY-2414 Introduce AliasDescriptor

caf2abc

SONARPY-2303 Stop running V1 type inference during the indexing phase

eab62f4

SONARPY-2415 Add stubs for pymsql, mysql and pgdb

d7b3283

Update protobuf

bfa085c

guillaume-dequenne-sonarsource force-pushed the SONARPY-2303 branch from d01706f to bfa085c Compare December 6, 2024 15:15

guillaume-dequenne-sonarsource enabled auto-merge (rebase) December 6, 2024 15:15

guillaume-dequenne-sonarsource merged commit 74cb3f2 into master Dec 6, 2024
9 of 10 checks passed

guillaume-dequenne-sonarsource deleted the SONARPY-2303 branch December 6, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-2303 Stop running V1 type inference during the indexing phase #2131

SONARPY-2303 Stop running V1 type inference during the indexing phase #2131

guillaume-dequenne-sonarsource commented Nov 5, 2024 •

edited by jira bot

Loading

sonarqube-next bot commented Nov 6, 2024

guillaume-dequenne-sonarsource Dec 5, 2024

maksim-grebeniuk-sonarsource Dec 6, 2024

maksim-grebeniuk-sonarsource Dec 6, 2024

maksim-grebeniuk-sonarsource Dec 6, 2024

thomas-serre-sonarsource left a comment

thomas-serre-sonarsource Dec 6, 2024

thomas-serre-sonarsource Dec 6, 2024

thomas-serre-sonarsource Dec 6, 2024

guillaume-dequenne-sonarsource Dec 6, 2024

thomas-serre-sonarsource Dec 6, 2024

guillaume-dequenne-sonarsource Dec 6, 2024

thomas-serre-sonarsource left a comment

sonarqube-next bot commented Dec 6, 2024

		importsByModule.put(fullyQualifiedModuleName, typeInferenceV2.importedModulesFQN());

SONARPY-2303 Stop running V1 type inference during the indexing phase #2131

SONARPY-2303 Stop running V1 type inference during the indexing phase #2131

Conversation

guillaume-dequenne-sonarsource commented Nov 5, 2024 • edited by jira bot Loading

sonarqube-next bot commented Nov 6, 2024

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-serre-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-serre-sonarsource left a comment

Choose a reason for hiding this comment

sonarqube-next bot commented Dec 6, 2024

Quality Gate passed

guillaume-dequenne-sonarsource commented Nov 5, 2024 •

edited by jira bot

Loading