SONARPY-747: S5864: 'raise' used with a non-exception type (SonarSource#844)

guillaume-dequenne · web-flow · commit ecf0a79cd720 · 2020-08-19T16:52:54.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
@@ -24,16 +24,20 @@
 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.Symbol;
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.RaiseStatement;
 import org.sonar.plugins.python.api.tree.Token;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.types.InferredType;
 import org.sonar.python.types.InferredTypes;
+import org.sonar.python.types.TypeShed;
 
 import static org.sonar.python.types.InferredTypes.containsDeclaredType;
 import static org.sonar.python.types.InferredTypes.typeClassLocation;
+import static org.sonar.python.types.InferredTypes.typeName;
 
 @Rule(key = "S5864")
 public class ConfusingTypeCheckingCheck extends PythonSubscriptionCheck {
@@ -43,6 +47,7 @@ public void initialize(Context context) {
     new IncompatibleOperandsCheck().initialize(context);
     new ItemOperationsTypeCheck().initialize(context);
     new IterationOnNonIterableCheck().initialize(context);
+    context.registerSyntaxNodeConsumer(Tree.Kind.RAISE_STMT, ConfusingTypeCheckingCheck::checkIncorrectExceptionType);
   }
 
   private static class NonCallableCalledCheck extends NonCallableCalled {
@@ -136,12 +141,30 @@ boolean isAsyncIterable(Expression expression) {
       // No need to check again if the type contains a declared type
       return type.declaresMember("__aiter__");
     }
+  }
 
-    private static String nameFromExpression(Expression expression) {
-      if (expression.is(Tree.Kind.NAME)) {
-        return ((Name) expression).name();
-      }
-      return null;
+  private static void checkIncorrectExceptionType(SubscriptionContext ctx) {
+    RaiseStatement raiseStatement = (RaiseStatement) ctx.syntaxNode();
+    if (raiseStatement.expressions().isEmpty()) {
+      return;
+    }
+    Expression raisedExpression = raiseStatement.expressions().get(0);
+    InferredType type = raisedExpression.type();
+    if (!containsDeclaredType(type)) {
+      return;
+    }
+    if (!type.isCompatibleWith(InferredTypes.runtimeType(TypeShed.typeShedClass("BaseException")))) {
+      String expressionName = nameFromExpression(raisedExpression) != null ? String.format("\"%s\"", nameFromExpression(raisedExpression)) : "this expression";
+      String typeName = typeName(type);
+      ctx.addIssue(raiseStatement, String.format("Fix this \"raise\" statement; Previous type checks suggest that %s has type \"%s\" and is not an exception.",
+        expressionName, typeName));
+    }
+  }
+
+  private static String nameFromExpression(Expression expression) {
+    if (expression.is(Tree.Kind.NAME)) {
+      return ((Name) expression).name();
     }
+    return null;
   }
 }
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
@@ -60,6 +60,12 @@ public void iteration_on_non_iterable() {
     assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/iterationOnNonIterable.py");
   }
 
+  @Test
+  public void incorrect_exception_type() {
+    PythonCheckVerifier.verify("src/test/resources/checks/confusingTypeChecking/incorrectExceptionType.py", check);
+    assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/incorrectExceptionType/incorrectExceptionType.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/incorrectExceptionType.py b/python-checks/src/test/resources/checks/confusingTypeChecking/incorrectExceptionType.py
@@ -0,0 +1,20 @@
+class A: ...
+class CustomException(BaseException): ...
+class AnotherException(CustomException): ...
+
+def my_int() -> int:
+  ...
+
+def custom(param1: A, param2: CustomException, param3: AnotherException):
+  raise param1  # Noncompliant {{Fix this "raise" statement; Previous type checks suggest that "param1" has type "A" and is not an exception.}}
+# ^^^^^^^^^^^^
+  raise param2
+  raise param3
+
+
+def builtin(param1: str, param2: Exception, param3: BaseException, param4: ValueError):
+  raise param1  # Noncompliant {{Fix this "raise" statement; Previous type checks suggest that "param1" has type "str" and is not an exception.}}
+  raise param2
+  raise param3
+  raise param4
+  raise my_int()  # Noncompliant {{Fix this "raise" statement; Previous type checks suggest that this expression has type "int" and is not an exception.}}
