SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model #2097

Seppli11 · 2024-10-23T14:27:05Z

Part of

guillaume-dequenne-sonarsource

The changes on the rule look good.
I'm not fully certain on the changes for the propagation of unary expression as I believe it should probably be done in FlowSensitiveTypeInference. I also think it would have deserved an explicitly separate ticket.
Small nitpick on the commit history that is not as clean as it could be.

guillaume-dequenne-sonarsource · 2024-10-24T15:09:37Z

python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java

@@ -49,12 +52,40 @@ public class IdentityComparisonWithCachedTypesCheck extends PythonSubscriptionCh
  public static final String IS_QUICK_FIX_MESSAGE = "Replace with \"==\"";
  public static final String IS_NOT_QUICK_FIX_MESSAGE = "Replace with \"!=\"";

+  /**
+   * Fully qualified names of constructors and functions that would are guaranteed to create fresh objects with


I know you didn't add this, but there's a typo here (that would are guaranteed -> that are guaranteed). Can you take the opportunity to fix it?

guillaume-dequenne-sonarsource · 2024-10-24T15:11:23Z

python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java

   * <code>is</code>-comparison with such a reference will always return <code>False</code>.
   * <p>
   * Note that these are fully qualified names of (value-level) expressions, not types.
   */
-  private static final Set<String> FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF = new HashSet<>(
-    asList("frozenset", "bytes", "int", "float", "str", "tuple", "hash"));
+  private static final List<String> FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF =


Note: I'm confused to see this being further changed in this propagate unary expressions commit. I feel it would have been cleaner to squash this with the first commit.

guillaume-dequenne-sonarsource · 2024-10-24T15:12:40Z

python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py

@@ -18,6 +18,9 @@ def literal_comparison(param):
    (4, 5) is param # Noncompliant {{Replace this "is" operator with "=="; identity operator is not reliable here.}}
    #      ^^

+    param is -1 #Noncompliant


Nitpick: We usually add a space # Noncompliant.
Also, just out of curiosity did you add these cases because of regressions on ruling?

Yes, it was from a ruling

guillaume-dequenne-sonarsource · 2024-10-24T15:21:12Z

...n-frontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java

@@ -152,6 +156,23 @@ public void visitNumericLiteral(NumericLiteral numericLiteral) {
    numericLiteralImpl.typeV2(new ObjectType(pythonType, new ArrayList<>(), new ArrayList<>()));
  }

+  @Override
+  public void visitUnaryExpression(UnaryExpression unaryExpr) {


I think by having this in the trivial type inference visitor rather than the flow-sensitive one, you are actually not covering cases such as:

x = 42 y = -x

The trivial type inference visitor is really meant to evaluate the type of trivial expressions that do not depend on control flow, such as builtins or the names of class and function definitions. It is only used for propagation in some cases where "normal" propagation would not lead to the desired result (see SONARPY-2244 and SONARPY-2245 for an example).

Here, the expression -42 has a trivial type only because the expression 42 has a trivial type (INT), but you don't have a guarantee that the expression for a unary operation will be trivial.

guillaume-dequenne-sonarsource · 2024-10-24T15:23:55Z

python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java

+    PythonType type = expr.typeV2();
+    if (isUnsuitableType(type)) {
+      if (expr instanceof Name name) {
+        SymbolV2 symb = name.symbolV2();


Does the comment and condition below still make sense? I don't think Name#symbolV2 can return null.

Seppli11 · 2024-10-25T14:10:25Z

As discussed, the way this PR implements the propagation of unary expressions means that only the type of unary literal expressions are properly propagated (e.g. -1, but not -x or -(1 + 1)). However, as a baby step, this moves us in the right direction. Therefore, a disabled test was added, testing the more complex behaviour. The test can be enabled as part of SONARPY-2257, which should implement the proper propagation of unary expressions.

guillaume-dequenne-sonarsource

The overall changes look good.
However, I think it would make sense to make intentions a bit clearer.
I'd like you to split this in 3 commits:

Infer types of unary expressions for literals
Implement the changes of the rule (keeping the null check for symbols & the test that covers it, but not the ticket references to SONARPY-2259 and SONARPY-2260)
Document missing symbols for builtins and unassigned variables. (add unit tests in SymbolTableBuilderV2Test highlighting SONARPY-2259 and SONARPY-2260).

(Normally, I'd split the 3rd commit if implementing the migration from scratch, but I think the cost of adding it at this point of the PR is rather low)

guillaume-dequenne-sonarsource · 2024-10-28T08:35:55Z

python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py

+    type(arr) is tuple # see SONARPY-2259
+    some_thing is other_thing # see SONARPY-2260


I'm not sure it makes sense to reference these other tickets here.
I think there is a distinction to be made between:

A test case added to highlight an unwanted behavior of the rule (e.g FP/FN).

A test case added to ensure proper test coverage, because of an unrelated limitation.

In the first case, it is normal to reference the ticket that would fix the unwanted behavior. In the second, fixing the ticket will not change the test result anyway, and the reference to the ticket is likely to stay in the code because of that (c.f Michael's comments on our weekly about stale ticket references in our code). Simply having the case for coverage is enough.

That being said, I think it would make sense to have actual frontend tests for our symbol tables (most likely in SymbolTableBuilderV2Test) that highlight these behaviors and link to a ticket.

guillaume-dequenne-sonarsource · 2024-10-28T09:43:29Z

python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py

@@ -18,6 +18,9 @@ def literal_comparison(param):
    (4, 5) is param # Noncompliant {{Replace this "is" operator with "=="; identity operator is not reliable here.}}
    #      ^^

+    param is -1 # Noncompliant
+    param is 1 + 1 #Noncompliant


Nitpick: Missing space.

guillaume-dequenne-sonarsource · 2024-10-28T09:45:03Z

python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java

+    PythonType type = expr.typeV2();
+    if (isUnsuitableType(type)) {
+      if (expr instanceof Name name) {
+        SymbolV2 symbol = name.symbolV2();


Name#symbolV2 should probably be annotated @CheckForNull since it can return null

guillaume-dequenne-sonarsource

LGTM. When SONARPY-2275 is merged, I believe this can be rebased, which will make ruling green then this can be merged.

…the new type model

…orrect

sonarqube-next · 2024-10-29T14:11:17Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

Seppli11 force-pushed the SONARPY-2204 branch from 774a2e6 to d4837f0 Compare October 23, 2024 15:50

Seppli11 marked this pull request as ready for review October 24, 2024 10:01

Seppli11 requested a review from guillaume-dequenne-sonarsource October 24, 2024 10:02

guillaume-dequenne-sonarsource requested changes Oct 24, 2024

View reviewed changes

Seppli11 force-pushed the SONARPY-2204 branch 2 times, most recently from 5f92243 to 596343c Compare October 25, 2024 13:44

Seppli11 requested a review from guillaume-dequenne-sonarsource October 25, 2024 14:11

guillaume-dequenne-sonarsource requested changes Oct 28, 2024

View reviewed changes

guillaume-dequenne-sonarsource reviewed Oct 28, 2024

View reviewed changes

Seppli11 force-pushed the SONARPY-2204 branch 9 times, most recently from 96fde07 to 2795cfd Compare October 29, 2024 07:55

Seppli11 requested a review from guillaume-dequenne-sonarsource October 29, 2024 11:23

guillaume-dequenne-sonarsource approved these changes Oct 29, 2024

View reviewed changes

Seppli11 force-pushed the SONARPY-2204 branch from 2795cfd to cd6581f Compare October 29, 2024 13:50

Seppli11 added 3 commits October 29, 2024 15:03

SONARPY-2267 Infer types of unary expressions of literals

6cf56be

SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to …

8ceaf06

…the new type model

SONARPY-2259 SONARPY-2260 add disabled tests checking if symbolV2() c…

08dde75

…orrect

Seppli11 force-pushed the SONARPY-2204 branch from cd6581f to 08dde75 Compare October 29, 2024 14:06

Seppli11 merged commit fe7e351 into master Oct 29, 2024
10 checks passed

Seppli11 deleted the SONARPY-2204 branch October 29, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model #2097

SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model #2097

Seppli11 commented Oct 23, 2024 •

edited by jira bot

Loading

guillaume-dequenne-sonarsource left a comment

guillaume-dequenne-sonarsource Oct 24, 2024

guillaume-dequenne-sonarsource Oct 24, 2024

guillaume-dequenne-sonarsource Oct 24, 2024

Seppli11 Oct 25, 2024

guillaume-dequenne-sonarsource Oct 24, 2024

guillaume-dequenne-sonarsource Oct 24, 2024

Seppli11 commented Oct 25, 2024 •

edited

Loading

guillaume-dequenne-sonarsource left a comment

guillaume-dequenne-sonarsource Oct 28, 2024

guillaume-dequenne-sonarsource Oct 28, 2024

guillaume-dequenne-sonarsource Oct 28, 2024

guillaume-dequenne-sonarsource left a comment

sonarqube-next bot commented Oct 29, 2024

		type(arr) is tuple # see SONARPY-2259
		some_thing is other_thing # see SONARPY-2260

SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model #2097

SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model #2097

Conversation

Seppli11 commented Oct 23, 2024 • edited by jira bot Loading

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

Seppli11 commented Oct 25, 2024 • edited Loading

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

guillaume-dequenne-sonarsource left a comment

Choose a reason for hiding this comment

sonarqube-next bot commented Oct 29, 2024

Quality Gate passed

Seppli11 commented Oct 23, 2024 •

edited by jira bot

Loading

Seppli11 commented Oct 25, 2024 •

edited

Loading