SONARPY-1869 Ensure conservative heuristics for generics inheritance

SonarSource · Nov 21, 2024 · 0a4a5b8 · 0a4a5b8
1 parent e4d132b
commit 0a4a5b8
Show file tree

Hide file tree

Showing 5 changed files with 151 additions and 15 deletions.
diff --git a/python-checks/src/test/resources/checks/genericTypeWithoutArgumentImported.py b/python-checks/src/test/resources/checks/genericTypeWithoutArgumentImported.py
@@ -1,4 +1,7 @@
-from typing import Generic
+from typing import Generic, TypeVar
+
+T = TypeVar('T')
+
 class SomeGeneric(Generic[T]):
   ...
 
@@ -7,3 +10,7 @@ class SomeGenericWithTypeParam[T](): ...
 class MyImportedGenericTypeVarChild(SomeGeneric[T]): ...
 class MyImportedNonGenericChild(SomeGeneric): ...
 class MyImportedConcreteChild(SomeGeneric[str]): ...
+
+# U has not been defined
+class SomeGenericIncorrectlyDefined(Generic[U]):
+    ...
diff --git a/python-checks/src/test/resources/checks/genericTypeWithoutArgumentImporting.py b/python-checks/src/test/resources/checks/genericTypeWithoutArgumentImporting.py
@@ -1,5 +1,11 @@
-# mod.py
-from genericTypeWithoutArgumentImported import SomeGeneric, SomeGenericWithTypeParam, MyImportedGenericTypeVarChild, MyImportedNonGenericChild, MyImportedConcreteChild
+from genericTypeWithoutArgumentImported import (
+    SomeGeneric,
+    SomeGenericWithTypeParam,
+    MyImportedGenericTypeVarChild,
+    MyImportedNonGenericChild,
+    MyImportedConcreteChild,
+    SomeGenericIncorrectlyDefined
+)
 
 def local_generic():
     from typing import Generic
@@ -16,8 +22,7 @@ def bar() -> SomeGenericWithTypeParam: # Noncompliant
 
 def returning_imported_child() -> MyImportedGenericTypeVarChild: ... # Noncompliant
 def returning_imported_non_generic_child() -> MyImportedNonGenericChild: ... # OK
-# FP SONARPY-2356: MyImportedConcreteChild is not actually generic (specialized class)
-def returning_imported_concrete_child() -> MyImportedConcreteChild: ... # Noncompliant
+def returning_imported_concrete_child() -> MyImportedConcreteChild: ... # OK
 
 class MyChild(SomeGeneric[T]): ...
 def returning_my_child() -> MyChild: # FN
@@ -30,3 +35,6 @@ def returning_my_non_generic_child() -> MyNonGenericChild: # OK
 class MyConcreteChild(SomeGeneric[str]): ...
 def returning_my_concrete_chil3() -> MyConcreteChild: # OK
     ...
+
+def returning_incorrect_generic() -> SomeGenericIncorrectlyDefined:
+    ...
diff --git a/...rontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java b/...rontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java
@@ -37,9 +37,11 @@
 import org.sonar.plugins.python.api.tree.AliasedName;
 import org.sonar.plugins.python.api.tree.AnnotatedAssignment;
 import org.sonar.plugins.python.api.tree.ArgList;
+import org.sonar.plugins.python.api.tree.AssignmentExpression;
 import org.sonar.plugins.python.api.tree.AssignmentStatement;
 import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
 import org.sonar.plugins.python.api.tree.BinaryExpression;
+import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ClassDef;
 import org.sonar.plugins.python.api.tree.ComprehensionExpression;
 import org.sonar.plugins.python.api.tree.DictCompExpression;
@@ -82,6 +84,7 @@
 import org.sonar.python.tree.SetLiteralImpl;
 import org.sonar.python.tree.StringLiteralImpl;
 import org.sonar.python.tree.SubscriptionExpressionImpl;
+import org.sonar.python.tree.TreeUtils;
 import org.sonar.python.tree.TupleImpl;
 import org.sonar.python.tree.UnaryExpressionImpl;
 import org.sonar.python.types.v2.ClassType;
@@ -113,6 +116,7 @@ public class TrivialTypeInferenceVisitor extends BaseTreeVisitor {
   private final Deque<Scope> typeStack = new ArrayDeque<>();
   private final Set<String> importedModulesFQN = new HashSet<>();
   private final TypeChecker typeChecker;
+  private final List<String> typeVarNames = new ArrayList<>();
 
   private final Map<String, TypeWrapper> wildcardImportedTypes = new HashMap<>();
 
@@ -320,12 +324,17 @@ private void addParentClass(ClassTypeBuilder classTypeBuilder, RegularArgument r
     }
     PythonType argumentType = getTypeV2FromArgument(regularArgument);
     classTypeBuilder.addSuperClass(argumentType);
-    if (typeChecker.typeCheckBuilder().isGeneric().check(argumentType) == TriBool.TRUE && regularArgument.expression().is(Tree.Kind.SUBSCRIPTION)) {
-      // SONARPY-2356: checking that we have a subscription only is too naive (e.g. specialized classes)
+    if (isParentAGenericClass(regularArgument, argumentType)) {
       classTypeBuilder.withIsGeneric(true);
     }
   }
 
+  private boolean isParentAGenericClass(RegularArgument regularArgument, PythonType argumentType) {
+    return typeChecker.typeCheckBuilder().isGeneric().check(argumentType) == TriBool.TRUE
+      && regularArgument.expression() instanceof SubscriptionExpression subscriptionExpression
+      && subscriptionExpression.subscripts().expressions().stream().anyMatch(expression -> expression instanceof Name name && typeVarNames.contains(name.name()));
+  }
+
   private static PythonType getTypeV2FromArgument(RegularArgument regularArgument) {
     Expression expression = regularArgument.expression();
     // Ensure we support correctly typing symbols like "List[str] / list[str]"
@@ -503,6 +512,44 @@ public void visitAnnotatedAssignment(AnnotatedAssignment assignmentStatement) {
       });
   }
 
+  @Override
+  public void visitCallExpression(CallExpression callExpression) {
+    super.visitCallExpression(callExpression);
+    assignPossibleTypeVar(callExpression);
+  }
+
+  private void assignPossibleTypeVar(CallExpression callExpression) {
+    PythonType pythonType = callExpression.callee().typeV2();
+    TriBool check = typeChecker.typeCheckBuilder().isTypeWithName("typing.TypeVar").check(pythonType);
+    if (check == TriBool.TRUE) {
+      Tree parent = TreeUtils.firstAncestor(callExpression, t -> t.is(Tree.Kind.ASSIGNMENT_STMT, Tree.Kind.ANNOTATED_ASSIGNMENT, Tree.Kind.ASSIGNMENT_EXPRESSION));
+      Optional<Name> assignedName = Optional.empty();
+      if (parent instanceof AssignmentStatement assignmentStatement) {
+        assignedName = extractAssignedName(assignmentStatement);
+      }
+      if (parent instanceof AnnotatedAssignment annotatedAssignment) {
+        assignedName = extractAssignedName(annotatedAssignment);
+      }
+      if (parent instanceof AssignmentExpression assignmentExpression) {
+        assignedName = extractAssignedName(assignmentExpression);
+      }
+      assignedName.ifPresent(name -> typeVarNames.add(name.name()));
+    }
+  }
+
+  private static Optional<Name> extractAssignedName(AssignmentExpression assignmentExpression) {
+    return Optional.of(assignmentExpression.lhsName()).map(Name.class::cast);
+  }
+
+  private static Optional<Name> extractAssignedName(AnnotatedAssignment annotatedAssignment) {
+    return Optional.of(annotatedAssignment.variable()).filter(v -> v.is(Tree.Kind.NAME)).map(Name.class::cast);
+  }
+
+  private static Optional<Name> extractAssignedName(AssignmentStatement assignmentStatement) {
+    return assignmentStatement.lhsExpressions().stream().findFirst()
+      .map(ExpressionList::expressions).stream().flatMap(List::stream).findFirst().filter(v -> v.is(Tree.Kind.NAME)).map(Name.class::cast);
+  }
+
   private static Optional<NameImpl> getFirstAssignmentName(AssignmentStatement assignmentStatement) {
     return Optional.of(assignmentStatement)
       .map(AssignmentStatement::lhsExpressions)

diff --git a/python-frontend/src/test/java/org/sonar/python/index/ClassDescriptorTest.java b/python-frontend/src/test/java/org/sonar/python/index/ClassDescriptorTest.java
@@ -109,7 +109,7 @@ void classDescriptorGenerics() {
     ClassDescriptor classDescriptor = lastClassDescriptor(
       "from typing import Generic",
       "class A(Generic[str]): ...");
-    assertThat(classDescriptor.supportsGenerics()).isTrue();
+    assertThat(classDescriptor.supportsGenerics()).isFalse();
     assertDescriptorToProtobuf(classDescriptor);
   }
 

diff --git a/python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeInferenceV2Test.java b/python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeInferenceV2Test.java
@@ -156,21 +156,94 @@ void builtinGenericType() {
   void userDefinedGenericType() {
     FileInput fileInput = inferTypes(
       """
-        from typing import Generic
+        from typing import Generic, TypeVar
+        T = TypeVar('T')
         class MyClass(Generic[T]): ...
         x = MyClass[str]()
         x
         """
     );
-    PythonType classType = ((ClassDef) fileInput.statements().statements().get(1)).name().typeV2();
-    ObjectType xType = (ObjectType) ((ExpressionStatement) fileInput.statements().statements().get(3)).expressions().get(0).typeV2();
+    PythonType classType = ((ClassDef) fileInput.statements().statements().get(2)).name().typeV2();
+    ObjectType xType = (ObjectType) ((ExpressionStatement) fileInput.statements().statements().get(4)).expressions().get(0).typeV2();
     assertThat(xType.unwrappedType()).isEqualTo(classType);
     // SONARPY-2356: Instantiation of specialized classes
     assertThat(xType.attributes()).isEmpty();
   }
 
   @Test
   void inheritedGenericType() {
+    FileInput fileInput = inferTypes(
+      """
+        from typing import Generic, TypeVar
+        T = TypeVar('T')
+        class MyClass(Generic[T]): ...
+        class MyOtherClass(MyClass[T]): ...
+        x = MyOtherClass[str]()
+        x
+        """
+    );
+    ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(3)).name().typeV2();
+    assertThat(myOtherClassType.isGeneric()).isTrue();
+    PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(5)).expressions().get(0).typeV2();
+    assertThat(xType.unwrappedType()).isEqualTo(myOtherClassType);
+  }
+
+  @Test
+  void inheritedGenericTypeUnsupportedExpression() {
+    FileInput fileInput = inferTypes(
+      """
+        from typing import Generic, TypeVar
+        T = TypeVar('T')
+        class MyClass(Generic[T()]): ...
+        class MyOtherClass(MyClass[T]): ...
+        x = MyOtherClass[str]()
+        x
+        """
+    );
+    ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(3)).name().typeV2();
+    assertThat(myOtherClassType.isGeneric()).isFalse();
+    PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(5)).expressions().get(0).typeV2();
+    assertThat(xType.unwrappedType()).isInstanceOf(UnknownType.class);
+  }
+
+  @Test
+  void inheritedGenericTypeVarAnnotatedAssignment() {
+    FileInput fileInput = inferTypes(
+      """
+        from typing import Generic, TypeVar
+        T: TypeVar = TypeVar('T')
+        class MyClass(Generic[T]): ...
+        class MyOtherClass(MyClass[T]): ...
+        x = MyOtherClass[str]()
+        x
+        """
+    );
+    ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(3)).name().typeV2();
+    assertThat(myOtherClassType.isGeneric()).isTrue();
+    PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(5)).expressions().get(0).typeV2();
+    assertThat(xType.unwrappedType()).isEqualTo(myOtherClassType);
+  }
+
+  @Test
+  void inheritedGenericTypeVarAssignmentExpression() {
+    FileInput fileInput = inferTypes(
+      """
+        from typing import Generic, TypeVar
+        foo(T:=TypeVar('T'))
+        class MyClass(Generic[T]): ...
+        class MyOtherClass(MyClass[T]): ...
+        x = MyOtherClass[str]()
+        x
+        """
+    );
+    ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(3)).name().typeV2();
+    assertThat(myOtherClassType.isGeneric()).isTrue();
+    PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(5)).expressions().get(0).typeV2();
+    assertThat(xType.unwrappedType()).isEqualTo(myOtherClassType);
+  }
+
+  @Test
+  void inheritedGenericTypeUndefinedTypeVar() {
     FileInput fileInput = inferTypes(
       """
         from typing import Generic
@@ -181,9 +254,10 @@ class MyOtherClass(MyClass[T]): ...
         """
     );
     ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(2)).name().typeV2();
-    assertThat(myOtherClassType.isGeneric()).isTrue();
+    // TypeVar is undefined: not a proper generic
+    assertThat(myOtherClassType.isGeneric()).isFalse();
     PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(4)).expressions().get(0).typeV2();
-    assertThat(xType.unwrappedType()).isEqualTo(myOtherClassType);
+    assertThat(xType.unwrappedType()).isInstanceOf(UnknownType.class);
   }
 
   @Test
@@ -219,9 +293,9 @@ class MyOtherClass(MyClass[str]): ...
     );
     ClassType myOtherClassType = (ClassType) ((ClassDef) fileInput.statements().statements().get(2)).name().typeV2();
     // SONARPY-2356: MyOtherClass can no longer be considered generic (specialized version of MyClass)
-    assertThat(myOtherClassType.isGeneric()).isTrue();
+    assertThat(myOtherClassType.isGeneric()).isFalse();
     PythonType xType = ((ExpressionStatement) fileInput.statements().statements().get(4)).expressions().get(0).typeV2();
-    assertThat(xType.unwrappedType()).isEqualTo(myOtherClassType);
+    assertThat(xType.unwrappedType()).isInstanceOf(UnknownType.class);
   }
 
   @Test