SONARPY-2009 Fix FP on S5644 when a subscription expression refers to a class with a generic type set (#388)

GitOrigin-RevId: 9e74be1df2c70f591063da4978faa3d9ac3e9d98

Author: Seppli11

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

@@ -16,7 +16,7 @@
  */
 package org.sonar.python.checks;
 
-import java.util.Collection;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import javax.annotation.Nullable;
@@ -26,6 +26,7 @@
 import org.sonar.plugins.python.api.symbols.FunctionSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.tree.ArgList;
+import org.sonar.plugins.python.api.tree.Argument;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ClassDef;
 import org.sonar.plugins.python.api.tree.Expression;
@@ -77,7 +78,25 @@ private static boolean isValidSubscriptionSymbol(Symbol symbol, Expression subsc
     @Nullable String classRequiredMethod) {
     LocationInFile locationInFile = symbol.is(FUNCTION) ? ((FunctionSymbol) symbol).definitionLocation() : ((ClassSymbol) symbol).definitionLocation();
     secondaries.put(locationInFile, SECONDARY_MESSAGE.formatted(symbol.name()));
-    return isSubscriptionInClassArg(subscriptionObject) || canHaveMethod(symbol, requiredMethod, classRequiredMethod);
+    return isSubscriptionInClassArg(subscriptionObject) 
+      || canHaveMethod(symbol, requiredMethod, classRequiredMethod) 
+      || isValidGenericUsage(symbol, subscriptionObject, requiredMethod);
+  }
+
+  private static boolean isValidGenericUsage(Symbol symbol, Expression subscriptionObject, String requiredMethod) {
+    return "__getitem__".equals(requiredMethod) && symbol.is(CLASS) && !areSomeSubscriptsSuspicious(subscriptionObject);
+  }
+
+  private static boolean areSomeSubscriptsSuspicious(Expression subscriptionObject) {
+    var subscriptionExprTree = TreeUtils.firstAncestorOfKind(subscriptionObject, Tree.Kind.SUBSCRIPTION);
+    return subscriptionExprTree instanceof SubscriptionExpression subscriptionExpr 
+      && subscriptionExpr.subscripts().expressions().stream()
+        .allMatch(ItemOperationsTypeCheck::isSubscriptSuspicious);
+  }
+
+  private static boolean isSubscriptSuspicious(Expression expr) {
+    // a subscript used as a generic should be a name of a class, alias, or a class as a string literal; Anything else is suspicious
+    return !expr.is(Tree.Kind.NAME, Tree.Kind.STRING_LITERAL);
   }
 
   private static boolean isInvalidSubscriptionCallExpr(Expression expression, Map<LocationInFile, String> secondaries) {
@@ -91,14 +110,23 @@ private static boolean isInvalidSubscriptionCallExpr(Expression expression, Map<
   }
 
   private static boolean isSubscriptionInClassArg(Expression subscriptionObject) {
-    return Optional.ofNullable(((ClassDef) TreeUtils.firstAncestorOfKind(subscriptionObject, Tree.Kind.CLASSDEF))).map(ClassDef::args).map(ArgList::arguments)
-      .stream()
-      .flatMap(Collection::stream)
+    var classDefOptional = Optional.ofNullable(TreeUtils.firstAncestorOfKind(subscriptionObject, Tree.Kind.CLASSDEF))
+      .map(ClassDef.class::cast);
+
+    List<Argument> classArguments = classDefOptional
+      .map(ClassDef::args)
+      .map(ArgList::arguments)
+      .orElse(List.of());
+
+    var onlyRegularArgumentExpressions = classArguments.stream()
       .flatMap(TreeUtils.toStreamInstanceOfMapper(RegularArgument.class))
-      .map(RegularArgument::expression)
+      .map(RegularArgument::expression);
+
+    var subscriptionObjectStream = onlyRegularArgumentExpressions
       .flatMap(TreeUtils.toStreamInstanceOfMapper(SubscriptionExpression.class))
-      .map(SubscriptionExpression::object)
-      .anyMatch(subscriptionObject::equals);
+      .map(SubscriptionExpression::object);
+
+    return subscriptionObjectStream.anyMatch(subscriptionObject::equals);
   }
 
   @Override

python-checks/src/test/resources/checks/itemOperationsTypeCheck/itemOperations_getitem.py

@@ -107,14 +107,16 @@ def __init__(self, values):
           self._values = values
 
   a = A([0,1,2])
-
   a[0]  # Noncompliant
 # ^^^^
 
   class B: ...
-
   B[0]  # Noncompliant
 
+  @some_decorator
+  class ClassWithDecorator: ...
+  ClassWithDecorator[0]  # FN: decorator
+
 
   class C:
       def __init__(self, values):
@@ -134,7 +136,7 @@ def __class_getitem__(cls, key):
 
 
 def getitem(self, key):
-    print(f"getting {key}")
+  print(f"getting {key}")
 
 def meta_classes():
   class MyMetaClassWithGet(type):
@@ -218,6 +220,37 @@ def custom_mock():
         a = ExtendedMock()[42]
 
 
-class MyGenericClass[T]: ...
+def generic_cases(unknown_type):
+  class MyGenericClass[T]: ...
+
+  class MyGenericSubType(MyGenericClass[str]): ...
+
+  class SomeOtherClass: ...
+  SomeOtherClassAlias = SomeOtherClass
+
+  T = TypeVar('T')
+  GenericAlias = MyGenericClass[T]  # OK
+  IntAlias = MyGenericClass[int]  # OK
+  IntLiteralAlias = MyGenericClass[0]  # Noncompliant
+  StrAlias = MyGenericClass[str]  # OK
+  StrLiteralAlias = MyGenericClass["str"]  # OK
+
+  a = MyGenericClass[int]()
+  a = MyGenericClass[SomeOtherClass]()
+  a = MyGenericClass[SomeOtherClassAlias]()
+  b = MyGenericClass[unknown_type]()
+  b = MyGenericClass[0]() # Noncompliant
+  b = MyGenericClass["str"]() # OK
+
+  c = SomeOtherClass()
+
+  c[0] # Noncompliant
+  c[int] # Noncompliant
+  c["int"] # Noncompliant
 
-class MyGenericSubType(MyGenericClass[str]): ...
+  class Loader:
+    def __getitem__(self, item):
+      return self.loader[item] # Noncompliant
+    
+    def loader(self):
+      pass

python-checks/src/test/resources/checks/itemOperationsTypeCheck/itemOperations_setitem.py

@@ -115,10 +115,6 @@ def __init__(self, values):
   a[0] = 42  # Noncompliant {{Fix this code; "a" does not have a "__setitem__" method.}}
 # ^^^^
 
-  class B: ...
-
-  B[0]  # Noncompliant
-
   class C:
       def __init__(self, values):
           self._values = values