SONARPY-761 Rule S5845: Assertions of dissimilar types should not be made (#1175)

Author: rudy-regazzoni-sonarsource

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

@@ -74,6 +74,7 @@
 import org.sonar.python.checks.regex.SuperfluousCurlyBraceCheck;
 import org.sonar.python.checks.regex.UnquantifiedNonCapturingGroupCheck;
 import org.sonar.python.checks.regex.VerboseRegexCheck;
+import org.sonar.python.checks.tests.AssertOnDissimilarTypesCheck;
 import org.sonar.python.checks.tests.AssertOnTupleLiteralCheck;
 import org.sonar.python.checks.tests.DedicatedAssertionCheck;
 import org.sonar.python.checks.tests.ImplicitlySkippedTestCheck;
@@ -94,6 +95,7 @@ public static Iterable<Class> getChecks() {
       AnchorPrecedenceCheck.class,
       ArgumentNumberCheck.class,
       ArgumentTypeCheck.class,
+      AssertOnDissimilarTypesCheck.class,
       AssertOnTupleLiteralCheck.class,
       BackslashInStringCheck.class,
       BackticksUsageCheck.class,

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

@@ -206,9 +206,10 @@ public boolean canImplementEqOrNe(Expression expression) {
     @Override
     String builtinTypeCategory(InferredType inferredType) {
       // inferredType will always contain a declared type because of the check done inside 'areIdentityComparableOrNone'
-      return BUILTINS_TYPE_CATEGORY.keySet().stream()
+      Map<String, String> builtinsTypeCategory = InferredTypes.getBuiltinsTypeCategory();
+      return builtinsTypeCategory.keySet().stream()
         .filter(typeName -> typeSymbols(inferredType).stream().map(Symbol::fullyQualifiedName).allMatch(typeName::equals))
-        .map(BUILTINS_TYPE_CATEGORY::get).findFirst().orElse(null);
+        .map(builtinsTypeCategory::get).findFirst().orElse(null);
     }
 
     @Override

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

@@ -20,36 +20,18 @@
 package org.sonar.python.checks;
 
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.HashSet;
 import javax.annotation.CheckForNull;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.tree.BinaryExpression;
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.Tree;
-import org.sonar.plugins.python.api.types.BuiltinTypes;
 import org.sonar.plugins.python.api.types.InferredType;
 
 public abstract class SillyEquality extends PythonSubscriptionCheck {
 
   private static final HashSet<String> CONSIDERED_OPERATORS = new HashSet<>(Arrays.asList("==", "!="));
-  protected static final HashMap<String, String> BUILTINS_TYPE_CATEGORY = new HashMap<>();
-
-  private static final String NUMBER = "number";
-
-  static {
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.STR, BuiltinTypes.STR);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.INT, NUMBER);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.FLOAT, NUMBER);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.COMPLEX, NUMBER);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.BOOL, NUMBER);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.LIST, BuiltinTypes.LIST);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.SET, BuiltinTypes.SET);
-    BUILTINS_TYPE_CATEGORY.put("frozenset", BuiltinTypes.SET);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.DICT, BuiltinTypes.DICT);
-    BUILTINS_TYPE_CATEGORY.put(BuiltinTypes.TUPLE, BuiltinTypes.TUPLE);
-  }
 
   @Override
   public void initialize(Context context) {

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

@@ -23,6 +23,7 @@
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.types.InferredType;
+import org.sonar.python.types.InferredTypes;
 
 import static org.sonar.plugins.python.api.types.BuiltinTypes.NONE_TYPE;
 
@@ -43,9 +44,7 @@ public boolean canImplementEqOrNe(Expression expression) {
   @CheckForNull
   @Override
   String builtinTypeCategory(InferredType inferredType) {
-    return BUILTINS_TYPE_CATEGORY.keySet().stream()
-      .filter(inferredType::canOnlyBe)
-      .map(BUILTINS_TYPE_CATEGORY::get).findFirst().orElse(null);
+    return InferredTypes.getBuiltinCategory(inferredType);
   }
 
   @Override

python-checks/src/main/java/org/sonar/python/checks/tests/AssertOnDissimilarTypesCheck.java

@@ -0,0 +1,169 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks.tests;
+
+import java.util.Comparator;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.symbols.Usage;
+import org.sonar.plugins.python.api.tree.ArgList;
+import org.sonar.plugins.python.api.tree.Argument;
+import org.sonar.plugins.python.api.tree.AssignmentStatement;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.QualifiedExpression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.types.InferredType;
+import org.sonar.python.tests.UnittestUtils;
+import org.sonar.python.tree.TreeUtils;
+import org.sonar.python.types.InferredTypes;
+
+import static org.sonar.plugins.python.api.types.BuiltinTypes.NONE_TYPE;
+
+@Rule(key = "S5845")
+public class AssertOnDissimilarTypesCheck extends PythonSubscriptionCheck {
+  private static final String MESSAGE = "Change this assertion to not compare dissimilar types";
+  private static final String MESSAGE_SECONDARY = "Last assignment of \"%s\"";
+  private static final Set<String> assertToCheckEquality = Set.of("assertEqual", "assertNotEqual");
+  private static final Set<String> assertToCheckIdentity = Set.of("assertIs", "assertIsNot");
+  private static final String FIRST_ARG_KEYWORD = "first";
+  private static final String SECOND_ARG_KEYWORD = "second";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, ctx -> {
+      CallExpression callExpression = (CallExpression) ctx.syntaxNode();
+      if (!callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR)) {
+        return;
+      }
+
+      QualifiedExpression qualifiedExpression = (QualifiedExpression) callExpression.callee();
+      if ((!(qualifiedExpression.qualifier().is(Tree.Kind.NAME) && ((Name) qualifiedExpression.qualifier()).name().equals("self")))
+       || !UnittestUtils.isWithinUnittestTestCase(qualifiedExpression)) {
+        return;
+      }
+
+      checkArguments(ctx, callExpression, qualifiedExpression);
+    });
+  }
+
+  private static void checkArguments(SubscriptionContext ctx, CallExpression callExpression, QualifiedExpression qualifiedExpression) {
+    boolean isAnAssertIdentity = assertToCheckIdentity.contains(qualifiedExpression.name().name());
+    boolean isAnAssertEquality = assertToCheckEquality.contains(qualifiedExpression.name().name());
+
+    if (!isAnAssertEquality && !isAnAssertIdentity) {
+      return;
+    }
+
+    ArgList args = callExpression.argumentList();
+    if (args == null) {
+      return;
+    }
+
+    List<Argument> arguments = args.arguments();
+    RegularArgument firstArg = TreeUtils.nthArgumentOrKeyword(0, FIRST_ARG_KEYWORD, arguments);
+    RegularArgument secondArg = TreeUtils.nthArgumentOrKeyword(1, SECOND_ARG_KEYWORD, arguments);
+    if (firstArg == null || secondArg == null) {
+      return;
+    }
+
+    Expression left = firstArg.expression();
+    Expression right = secondArg.expression();
+
+    if (canArgumentsBeIdentical(left, right)) {
+      return;
+    }
+
+    if (isAnAssertIdentity || !canArgumentsBeEqual(left, right)) {
+      PreciseIssue issue = ctx.addIssue(args, message(left, right));
+      getLastAssignment(left).ifPresent(assign -> issue.secondary(assign, String.format(MESSAGE_SECONDARY, ((Name)left).name())));
+      getLastAssignment(right).ifPresent(assign -> issue.secondary(assign, String.format(MESSAGE_SECONDARY, ((Name)right).name())));
+    }
+  }
+
+  private static String message(Expression left, Expression right) {
+    String leftTypeName = InferredTypes.typeName(left.type());
+    String rightTypeName = InferredTypes.typeName(right.type());
+    String message = MESSAGE;
+    if (leftTypeName != null && rightTypeName != null) {
+      message += " (" + leftTypeName + " and " + rightTypeName + ")";
+    }
+    message += ".";
+    return message;
+  }
+
+  private static Optional<AssignmentStatement> getLastAssignment(Expression expr) {
+    if (!expr.is(Tree.Kind.NAME)) {
+      return Optional.empty();
+    }
+
+    Symbol symbol = ((Name) expr).symbol();
+    if (symbol == null) {
+      return Optional.empty();
+    }
+
+    Usage lastAssignment = symbol.usages().stream()
+      .sorted(Comparator.comparingInt(u -> u.tree().firstToken().line()))
+      .takeWhile(usage -> usage != ((Name) expr).usage())
+      .filter(usage -> usage.kind() == Usage.Kind.ASSIGNMENT_LHS)
+      .reduce((first, second) -> second)
+      .orElse(null);
+
+    return Optional.ofNullable(lastAssignment)
+      .map(assignment -> (AssignmentStatement) TreeUtils.firstAncestorOfKind(assignment.tree(), Tree.Kind.ASSIGNMENT_STMT));
+  }
+
+  private static boolean canArgumentsBeIdentical(Expression left, Expression right) {
+    return left.type().isIdentityComparableWith(right.type()) || left.type().canOnlyBe(NONE_TYPE) || right.type().canOnlyBe(NONE_TYPE);
+  }
+
+  private static boolean canArgumentsBeEqual(Expression left, Expression right) {
+    String leftCategory = InferredTypes.getBuiltinCategory(left.type());
+    String rightCategory = InferredTypes.getBuiltinCategory(right.type());
+    boolean leftCanImplementEqOrNe = canImplementEqOrNe(left);
+    boolean rightCanImplementEqOrNe = canImplementEqOrNe(right);
+
+    if ((leftCategory != null && leftCategory.equals(rightCategory))) {
+      return true;
+    }
+
+    return (leftCanImplementEqOrNe || rightCanImplementEqOrNe)
+      && (leftCategory == null || rightCategory == null)
+      && (leftCategory == null || rightCanImplementEqOrNe)
+      && (rightCategory == null || leftCanImplementEqOrNe);
+  }
+
+  private static boolean canImplementEqOrNe(Expression expression) {
+    InferredType type = expression.type();
+    return type.canHaveMember("__eq__") || type.canHaveMember("__ne__");
+  }
+
+  @Override
+  public CheckScope scope() {
+    return CheckScope.ALL;
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5845.html

@@ -0,0 +1,39 @@
+<p>Calling <code>unittest</code> methods <code>assertEqual</code>, <code>assertNotEqual</code>, <code>assertIs</code> or <code>assertIsNot</code> on
+objects of incompatible types will always fail or always succeed.</p>
+<p>For methods <code>assertEqual</code> and <code>assertNotEqual</code>, arguments' types are incompatible if:</p>
+<pre>
+they are unrelated builtin types such as string and integer.
+</pre>
+<pre>
+they are instances of unrelated classes which do not implement ``++__eq__++`` or ``++__ne__++`` (if a class implements one of these methods it could compare to any other type it wants).
+</pre>
+<p>As for methods <code>assertIs</code> and <code>assertIsNot</code>, if arguments' types are different it is not possible for them to point to the
+same object, thus <code>assertIs</code> will always fail and <code>assertIsNot</code> will always succeed.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+import unittest
+class A(): ...
+class MyTest(unittest.TestCase):
+  def test_something(self):
+    a = A()
+    mydict = {"x": a}
+    self.assertEqual(a, "x")  # Noncompliant
+    self.assertIs(a, "x")  # Noncompliant
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+import unittest
+class A(): ...
+class MyTest(unittest.TestCase):
+  def test_something(self):
+    a = A()
+    mydict = {"x": a}
+    self.assertEqual(a, mydict["x"])  # OK
+    self.assertIs(a, mydict["x"])  # OK
+</pre>
+<h2>See</h2>
+<ul>
+  <li> {rule:python:S2159} Silly equality checks should not be made </li>
+  <li> {rule:python:S3403} Identity operators should not be used with dissimilar types </li>
+</ul>
+

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5845.json

@@ -0,0 +1,17 @@
+{
+  "title": "Assertions comparing incompatible types should not be made",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "tests"
+  ],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-5845",
+  "sqKey": "S5845",
+  "scope": "Tests",
+  "quickfix": "unknown"
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

@@ -141,6 +141,7 @@
     "S5828",
     "S5842",
     "S5843",
+    "S5845",
     "S5850",
     "S5855",
     "S5857",

python-checks/src/test/java/org/sonar/python/checks/tests/AssertOnDissimilarTypesCheckTest.java

@@ -0,0 +1,44 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks.tests;
+
+import org.junit.Test;
+import org.sonar.plugins.python.api.PythonCheck;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class AssertOnDissimilarTypesCheckTest {
+
+  @Test
+  public void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertOnDissimilarTypes.py", new AssertOnDissimilarTypesCheck());
+  }
+
+  @Test
+  public void testAnotherLibrary() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertOnDissimilarTypesAnotherLibrary.py", new AssertOnDissimilarTypesCheck());
+  }
+
+  @Test
+  public void test_scope() {
+    assertThat(new AssertOnDissimilarTypesCheck().scope()).isEqualTo(PythonCheck.CheckScope.ALL);
+  }
+}