SONARPY-1798 Try to resolve built-in types for names which have no symbol #1774

maksim-grebeniuk-sonarsource · 2024-04-25T13:00:23Z

No description provided.

guillaume-dequenne-sonarsource

This PR is a good example of what I'm trying to promote and why.

I believe that if this PR had been focused on fixing the following problem:

"If a name does not have any binding usage, let's search in the builtins module to allocate a type for it"

It could have been much smaller and focused, easier to review and to make progress (carving it out of the ArgumentNumberCheck was a great change, IMO - it would have been even better to do it from the start).

I also have a question regarding names whose type is a builtin: what does it mean for them in terms of symbol? I believe for now, they would have a type but no symbol (because no assignment). Is this intentional? I don't have a strong opinion on the behavior at this point, but I think we should probably test/document this. It may have impacts on rules that aim to detect undefined symbols.

guillaume-dequenne-sonarsource · 2024-04-25T15:05:06Z

python-frontend/src/main/java/org/sonar/python/types/v2/ClassType.java

+  public boolean isOrExtends(String s) {
+//    TODO: implement and maybe it should accept PythonType instead of string
+    return false;
+  }


Do we need this as part of this PR? It's probably better to introduce it when we need it.

excluded from PR

guillaume-dequenne-sonarsource · 2024-04-25T15:08:11Z

python-frontend/src/test/java/org/sonar/python/types/v2/ClassTypeTest.java

@@ -157,8 +157,7 @@ void builtin_parent() {
    );
    ClassType classB = classTypes.get(1);
    assertThat(classB.superClasses()).hasSize(2);
-    // FIXME: ensure builtin parent is resolved
-    assertThat(classB.hasUnresolvedHierarchy()).isTrue();
+    assertThat(classB.hasUnresolvedHierarchy()).isFalse();


Could you add some assertions?
Now that the hierarchy is resolved, it would make sense to assert on parents/members to ensure information about BaseException is correctly retrieved (e.g members, etc...).

It may also make sense to have an additional test with a builtin parent that actually has an unresolved type hierarchy to ensure the information is not lost then.

guillaume-dequenne-sonarsource · 2024-04-25T15:15:08Z

python-frontend/src/main/java/org/sonar/python/tree/SubscriptionExpressionImpl.java

@@ -77,4 +79,13 @@ public List<Tree> computeChildren() {
  public Kind getKind() {
    return Kind.SUBSCRIPTION;
  }
+
+  @Override
+  public PythonType typeV2() {


I think it would make sense to have a test for this (e.g is ObjectTypeTest).

I also think the current implementation is not correct. If I have:

my_list = ["hello"] foo(my_list[0])

The type of the argument (= the subscription expression) is str and not list. I think it would therefore make sense for the type of a subscription expression to be evaluated as the type of the attribute, if its own type is a known container type. I think this could deserve a dedicated ticket, with some examples of fixed FNs though.

Ultimately if I remove this method, everything is still green. So maybe it's best to leave this out of this PR for now as it's not really needed.

excluded from PR

guillaume-dequenne-sonarsource · 2024-04-25T16:06:07Z

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

+      .map(TypeInferenceV2::getUsagesType)
+      .or(() -> projectLevelTypeTable.getModule().resolveMember(name.name()))


I think this is a subtle yet impactful change from the previous implementation.

In the previous implementation, any variable that either has multiple assignments or a global declaration usage would not be assigned a type.

Now we fall back to builtins instead, which means that in the following (admittedly convoluted) code:

global int int = 42 int = "hello" int

The final int would be considered to be builtins.int despite the multiple reassignments.

I think the fallback to builtins should only happen if a symbol has no binding usages.

I'm a bit unhappy about the focus of our discussion regarding the imperative loop vs the optional chain. I think the main focus should be on functional behavior rather than technical implementation and I believe this case was an example where we can improve.

Taking the previous implementation as a reference, I believe that a simple

if (types.size().isEmpty() { // the underlying assumption is that each binding usage will have a type and add to the list // if it's not the case, a simple boolean "hasNoBindingUsage" computed during the iteration would do the trick setTypeToName(name, projectLevelTypeTable.getModule().resolveMember(name.name())); }

at the end of the method would have achieved what you're trying to do, without requiring an additional iteration over usages and without risking an undesired functional change.

I also think we are missing tests for this (like variations of the convoluted case I showed), which is hidden due to the optional chain.

Again, I have nothing against optional chains, but they do have limitations that need to be acknowledged and I don't think we should aim to refactor everything to these just for the sake of it.

guillaume-dequenne-sonarsource · 2024-04-25T16:09:36Z

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

@@ -148,8 +148,16 @@ private void addParameter(org.sonar.plugins.python.api.tree.Parameter parameter,
    Token starToken = parameter.starToken();
    if (parameterName != null) {
      ParameterType parameterType = getParameterType(parameter);
-      this.parameters.add(new ParameterV2(parameterName.name(), parameterType.pythonType(), parameter.defaultValue() != null,
-        parameterState.keywordOnly, parameterState.positionalOnly, parameterType.isKeywordVariadic(), parameterType.isPositionalVariadic(), locationInFile(parameter, fileId)));
+      var parameterV2 = new ParameterV2(parameterName.name(),


Do we need the changes regarding FunctionType building for this PR?
I feel we can focus on the builtins fallback which would then be clearly separated from this.

guillaume-dequenne-sonarsource · 2024-04-25T16:13:17Z

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

-  public void createBuiltinModule(ModuleType parent) {
-    var name = "builtins";
-    createModuleFromSymbols(name, parent, TypeShed.builtinSymbols().values());
+  public ModuleType createBuiltinModule() {


I'm not sure yet that I see the value of removing the builtins prefix.

I believe we could still have a builtins dedicated module with a helper method that would help retrieve it so that we don't have to duplicate the "builtins" literal everywhere?

it is mainly needed to convert symbols (old ones) to types, the builtin types don't have builtins in their FQN

I see! I think it was a mistake on our end in the original implementation, though. Since other tools (including the Python interpreter itself) will map builtin types to the builtins module.

Ideally, I think I'd like to follow that convention as well, however I think for now the least disruptive approach is okay (i.e, if this PR still achieves its goals without removing builtins, I'd keep the prefix - and if it breaks a lot of other stuff, we can consider adding it back later instead).

sonarqube-next · 2024-04-26T09:17:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube

guillaume-dequenne-sonarsource

I still have some questions before we proceed with merging this PR.

guillaume-dequenne-sonarsource · 2024-04-26T09:48:11Z

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

  @Override
  public void visitName(Name name) {
    SymbolV2 symbolV2 = name.symbolV2();
    if (symbolV2 == null) {
+      projectLevelTypeTable.getModule().resolveMember(name.name())


I'm not sure I understand this change.

Before, we were falling back to built-in symbols that did not have binding usages.
Now, we're doing it if the symbol is null.

Does that mean that the previous implementation, as well as my suggestion, was wrong? I'd expect this to be explicitly addressed in the comment then. Also, why did the original implementation solve the FNs in nonCallableCalled.py if it was incorrect?

I'm also not sure that we really want the symbol of used built-in names to be null in the first place, which could deserve at least comment / discussion / ticket as well, depending one how we want to proceed with this.

in my implementation, there was no difference between no symbol and symbol doesn't have a binding usage, that's why it has been working.
About symbol for builtin types - agree but it is out of this ticket scope

TODO: add comment with ticket

guillaume-dequenne-sonarsource · 2024-04-26T09:58:24Z

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

@@ -75,8 +75,6 @@ public TypeInferenceV2(ProjectLevelTypeTable projectLevelTypeTable) {
    this.projectLevelTypeTable = projectLevelTypeTable;
  }

-  private static final String BUILTINS = "builtins";


You didn't answer my question about builtins removal.

Given we don't map FQNs to PythonType yet, do we really need to remove it? Does it break something?

I know it looks like I'm pushing to keep it, but I would really like to understand why this PR needed to remove it, given that we haven't made any decisions on how we represent FQNs yet.

cause when you resolve types e.g. for serialized symbols or project level symbols the FQN of type str is just str, it doesn't operate with builtins, and I want to have common behavior for it

…mbol

guillaume-dequenne-sonarsource

LGTM

…mbol (#1774)

maksim-grebeniuk-sonarsource changed the base branch from master to rnd/type-inference-engine-specification April 25, 2024 13:00

maksim-grebeniuk-sonarsource force-pushed the mg/SONARPY-1798 branch from 000ebe2 to 3c31b8b Compare April 25, 2024 13:01

maksim-grebeniuk-sonarsource requested a review from guillaume-dequenne-sonarsource April 25, 2024 13:01

maksim-grebeniuk-sonarsource force-pushed the mg/SONARPY-1798 branch 2 times, most recently from 3a97a6f to b74df45 Compare April 25, 2024 14:38

guillaume-dequenne-sonarsource requested changes Apr 25, 2024

View reviewed changes

maksim-grebeniuk-sonarsource force-pushed the mg/SONARPY-1798 branch from b74df45 to b2a3106 Compare April 26, 2024 08:35

maksim-grebeniuk-sonarsource requested a review from guillaume-dequenne-sonarsource April 26, 2024 08:37

maksim-grebeniuk-sonarsource force-pushed the mg/SONARPY-1798 branch 2 times, most recently from af8dceb to efb7f8b Compare April 26, 2024 09:10

guillaume-dequenne-sonarsource reviewed Apr 26, 2024

View reviewed changes

maksim-grebeniuk-sonarsource requested a review from guillaume-dequenne-sonarsource April 26, 2024 10:27

guillaume-dequenne-sonarsource changed the title ~~SONARPY-1798 Change level of buildting types to root moduletype~~ SONARPY-1798 Try to resolve built-in types for names which have no binding usages Apr 26, 2024

guillaume-dequenne-sonarsource changed the title ~~SONARPY-1798 Try to resolve built-in types for names which have no binding usages~~ SONARPY-1798 Try to resolve built-in types for names which have no symbol Apr 26, 2024

SONARPY-1798 Try to resolve built-in types for names which have no sy…

71dac54

…mbol

maksim-grebeniuk-sonarsource force-pushed the mg/SONARPY-1798 branch from efb7f8b to 71dac54 Compare April 26, 2024 11:37

guillaume-dequenne-sonarsource approved these changes Apr 26, 2024

View reviewed changes

maksim-grebeniuk-sonarsource merged commit cb39e36 into rnd/type-inference-engine-specification Apr 26, 2024
0 of 8 checks passed

maksim-grebeniuk-sonarsource deleted the mg/SONARPY-1798 branch April 26, 2024 11:40

guillaume-dequenne-sonarsource pushed a commit that referenced this pull request May 1, 2024

SONARPY-1798 Try to resolve built-in types for names which have no sy…

1a8795b

…mbol (#1774)

guillaume-dequenne-sonarsource pushed a commit that referenced this pull request May 1, 2024

SONARPY-1798 Try to resolve built-in types for names which have no sy…

be9ce42

…mbol (#1774)

guillaume-dequenne-sonarsource pushed a commit that referenced this pull request May 15, 2024

SONARPY-1798 Try to resolve built-in types for names which have no sy…

5addaf2

…mbol (#1774)

guillaume-dequenne-sonarsource pushed a commit that referenced this pull request May 21, 2024

SONARPY-1798 Try to resolve built-in types for names which have no sy…

3326647

…mbol (#1774)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-1798 Try to resolve built-in types for names which have no symbol #1774

SONARPY-1798 Try to resolve built-in types for names which have no symbol #1774

maksim-grebeniuk-sonarsource commented Apr 25, 2024

guillaume-dequenne-sonarsource left a comment

guillaume-dequenne-sonarsource Apr 25, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource Apr 25, 2024

guillaume-dequenne-sonarsource Apr 25, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource Apr 25, 2024

guillaume-dequenne-sonarsource Apr 25, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource Apr 25, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource Apr 26, 2024

sonarqube-next bot commented Apr 26, 2024

guillaume-dequenne-sonarsource left a comment

guillaume-dequenne-sonarsource Apr 26, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource Apr 26, 2024

maksim-grebeniuk-sonarsource Apr 26, 2024

guillaume-dequenne-sonarsource left a comment

		.map(TypeInferenceV2::getUsagesType)
		.or(() -> projectLevelTypeTable.getModule().resolveMember(name.name()))

SONARPY-1798 Try to resolve built-in types for names which have no symbol #1774

SONARPY-1798 Try to resolve built-in types for names which have no symbol #1774

Conversation

maksim-grebeniuk-sonarsource commented Apr 25, 2024

guillaume-dequenne-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

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

sonarqube-next bot commented Apr 26, 2024

Quality Gate passed

guillaume-dequenne-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

guillaume-dequenne-sonarsource left a comment

Choose a reason for hiding this comment