SONARPY-741 RSPEC-5864 Identity check between incompatible types

andreaguarino · guillaume-dequenne · commit 12241e6e3333 · 2020-08-20T17:54:34.000+02:00
diff --git a/python-checks/src/main/java/org/sonar/python/checks/ConfusingTypeCheckingCheck.java b/python-checks/src/main/java/org/sonar/python/checks/ConfusingTypeCheckingCheck.java
@@ -19,15 +19,18 @@
  */
 package org.sonar.python.checks;
 
+import java.util.Collection;
 import java.util.List;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.LocationInFile;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.ClassSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.IsExpression;
 import org.sonar.plugins.python.api.tree.Name;
 import org.sonar.plugins.python.api.tree.RaiseStatement;
 import org.sonar.plugins.python.api.tree.Token;
@@ -51,6 +54,7 @@ public void initialize(Context context) {
     new IterationOnNonIterableCheck().initialize(context);
     new SillyEqualityCheck().initialize(context);
     context.registerSyntaxNodeConsumer(Tree.Kind.RAISE_STMT, ConfusingTypeCheckingCheck::checkIncorrectExceptionType);
+    context.registerSyntaxNodeConsumer(Tree.Kind.IS, ConfusingTypeCheckingCheck::checkSillyIdentity);
   }
 
   private static class NonCallableCalledCheck extends NonCallableCalled {
@@ -175,12 +179,7 @@ private static class SillyEqualityCheck extends SillyEquality {
 
     @Override
     boolean areIdentityComparableOrNone(InferredType leftType, InferredType rightType) {
-      if (!containsDeclaredType(leftType) && !containsDeclaredType(rightType)) {
-        return true;
-      }
-      return leftType.equals(rightType)
-        || (typeSymbols(leftType).stream().map(Symbol::fullyQualifiedName).allMatch("NoneType"::equals))
-        || (typeSymbols(rightType).stream().map(Symbol::fullyQualifiedName).allMatch("NoneType"::equals));
+      return ConfusingTypeCheckingCheck.areIdentityComparableOrNone(leftType, rightType);
     }
 
     @Override
@@ -204,4 +203,27 @@ String message(String result) {
       return "Fix this equality check; Previous type checks suggest that operands have incompatible types.";
     }
   }
+
+  private static boolean areIdentityComparableOrNone(InferredType leftType, InferredType rightType) {
+    if (!containsDeclaredType(leftType) && !containsDeclaredType(rightType)) {
+      return true;
+    }
+    Collection<ClassSymbol> leftSymbols = typeSymbols(leftType);
+    Collection<ClassSymbol> rightSymbols = typeSymbols(rightType);
+    return leftSymbols.stream().map(Symbol::fullyQualifiedName).anyMatch(fqn -> fqn == null || rightSymbols.stream().anyMatch(rs -> rs.canBeOrExtend(fqn)))
+      || rightSymbols.stream().map(Symbol::fullyQualifiedName).anyMatch(fqn -> fqn == null || leftSymbols.stream().anyMatch(ls -> ls.canBeOrExtend(fqn)))
+      || (typeSymbols(leftType).stream().map(Symbol::fullyQualifiedName).allMatch("NoneType"::equals))
+      || (typeSymbols(rightType).stream().map(Symbol::fullyQualifiedName).allMatch("NoneType"::equals));
+  }
+
+  private static void checkSillyIdentity(SubscriptionContext ctx) {
+    IsExpression isExpression = (IsExpression) ctx.syntaxNode();
+    InferredType left = isExpression.leftOperand().type();
+    InferredType right = isExpression.rightOperand().type();
+    if (!areIdentityComparableOrNone(left, right)) {
+      Token notToken = isExpression.notToken();
+      Token lastToken = notToken == null ? isExpression.operator() : notToken;
+      ctx.addIssue(isExpression.operator(), lastToken, "Fix this identity check; Previous type checks suggest that operands have incompatible types.");
+    }
+  }
 }
diff --git a/python-checks/src/test/java/org/sonar/python/checks/ConfusingTypeCheckingCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/ConfusingTypeCheckingCheckTest.java
@@ -72,6 +72,12 @@ public void silly_equality() {
     assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/sillyEquality.py");
   }
 
+  @Test
+  public void silly_identity() {
+    PythonCheckVerifier.verify("src/test/resources/checks/confusingTypeChecking/sillyIdentity.py", check);
+    assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/sillyIdentity.py");
+  }
+
   private void assertNoIssuesInCorrespondingBugRule(String path) {
     PythonVisitorContext context = TestPythonVisitorRunner.createContext(new File(path));
     SubscriptionVisitor.analyze(Collections.singletonList(check), context);
diff --git a/python-checks/src/test/resources/checks/confusingTypeChecking/sillyIdentity.py b/python-checks/src/test/resources/checks/confusingTypeChecking/sillyIdentity.py
@@ -0,0 +1,31 @@
+from typing import Union
+class A:
+  def foo(self): ...
+class B: ...
+def custom(x: A, y: B, z: Union['foo', 'bar']):
+  if x is y: ... # Noncompliant {{Fix this identity check; Previous type checks suggest that operands have incompatible types.}}
+  #    ^^
+  if x is not y: ... # Noncompliant
+  #    ^^^^^^
+  if x is x: ...
+  xx = x
+  if x:
+    yy = xx
+  else:
+    yy = y
+  if yy is not xx: ...
+  if z is x: ...
+  if x is z: ...
+
+def builtins(x: int, y: str):
+  if x is y: ... # Noncompliant
+  if x is 'foo': ... # Noncompliant
+  if 42 is y: ... # Noncompliant
+  if x is None: ... # OK
+  if None is y: ... # OK
+
+class Base: ...
+class C(Base): ...
+def with_superclass(x: Base, y: C):
+  if x is y: ... # OK
+  if y is x: ... # OK
