SONARPY-2370 Fix FP on S5644 for generic types defined through type parameter syntax #2209

ghislainpiot · 2024-12-03T14:23:28Z

…arameter syntax

ghislainpiot · 2024-12-03T16:05:59Z

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

+    return false;
+  }
+
+  private static Optional<Expression> isSubscriptionInClassArg(Expression subscriptionObject) {


I went for the quick win to fix the FP, instead of looking at migrating to V2.

maksim-grebeniuk-sonarsource · 2024-12-04T08:46:30Z

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

+    return null;
+  }
+
+  private static boolean isValidSubscriptionCallExpr(CallExpression subscriptionObject, Map<LocationInFile, String> secondaries) {


could be simplified

private static boolean isValidSubscriptionCallExpr(Expression expression, Map<LocationInFile, String> secondaries) { if (expression instanceof CallExpression callExpression && callExpression.calleeSymbol() instanceof FunctionSymbol functionSymbol && functionSymbol.isAsynchronous()) { secondaries.put(functionSymbol.definitionLocation(), SECONDARY_MESSAGE.formatted(functionSymbol.name())); return true; } return false; }

and then no need to check for subscriptionObject.is(Tree.Kind.CALL_EXPR) on line 56

maksim-grebeniuk-sonarsource · 2024-12-04T08:47:35Z

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

+    Symbol subscriptionCalleeSymbol = subscriptionObject.calleeSymbol();
+    if (subscriptionCalleeSymbol != null && subscriptionCalleeSymbol.is(FUNCTION) && ((FunctionSymbol) subscriptionCalleeSymbol).isAsynchronous()) {
+      FunctionSymbol functionSymbol = (FunctionSymbol) subscriptionCalleeSymbol;
+      secondaries.put(functionSymbol.definitionLocation(), String.format(SECONDARY_MESSAGE, functionSymbol.name()));


String.format(SECONDARY_MESSAGE... could be replaced with:

SECONDARY_MESSAGE.formatted(functionSymbol.name())

maksim-grebeniuk-sonarsource · 2024-12-04T08:48:13Z

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

+    return false;
+  }
+
+  private static Optional<Expression> isSubscriptionInClassArg(Expression subscriptionObject) {


could simply return a boolean:

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) .flatMap(TreeUtils.toStreamInstanceOfMapper(RegularArgument.class)) .map(RegularArgument::expression) .flatMap(TreeUtils.toStreamInstanceOfMapper(SubscriptionExpression.class)) .map(SubscriptionExpression::object) .anyMatch(subscriptionObject::equals); }

maksim-grebeniuk-sonarsource · 2024-12-04T08:48:37Z

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

+
+class MyGenericClass[T]: ...
+
+class MyGenericSubType(MyGenericClass[str]): ... # FP S5644


minor: add empty line in the end of the file

maksim-grebeniuk-sonarsource · 2024-12-04T08:59:49Z

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

+      if (isSubscriptionInClassArg(subscriptionObject).isPresent()) {
+        return true;
+      }
+
+      return canHaveMethod(symbol, requiredMethod, classRequiredMethod);


could be simplified:

return isSubscriptionInClassArg(subscriptionObject) || canHaveMethod(symbol, requiredMethod, classRequiredMethod);

maksim-grebeniuk-sonarsource · 2024-12-04T09:13:48Z

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

+      return true;
+    }
+    if (symbol.is(FUNCTION, CLASS)) {
+      secondaries.put(symbol.is(FUNCTION) ? ((FunctionSymbol) symbol).definitionLocation() : ((ClassSymbol) symbol).definitionLocation(),


It will be easier to read if the location calculation will be extended and assigned to a local variable

maksim-grebeniuk-sonarsource · 2024-12-04T09:14:32Z

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

+    }
+    if (symbol.is(FUNCTION, CLASS)) {
+      secondaries.put(symbol.is(FUNCTION) ? ((FunctionSymbol) symbol).definitionLocation() : ((ClassSymbol) symbol).definitionLocation(),
+        String.format(SECONDARY_MESSAGE, symbol.name()));


could be replaced with

SECONDARY_MESSAGE.formatter(symbol.name())

maksim-grebeniuk-sonarsource · 2024-12-04T09:39:00Z

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

    }
+
    if (subscriptionObject instanceof HasSymbol hasSymbol) {


this if could be moved to be a part of isValidSubscriptionSymbol

maksim-grebeniuk-sonarsource · 2024-12-04T09:56:50Z

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

-        secondaries.put(functionSymbol.definitionLocation(), String.format(SECONDARY_MESSAGE, functionSymbol.name()));
-        return false;
-      }
+    if (subscriptionObject.is(Tree.Kind.CALL_EXPR) && isValidSubscriptionCallExpr((CallExpression) subscriptionObject, secondaries)) {


seems that methods isValidSubscriptionCallExpr and isValidSubscriptionSymbol both have inverted logic to the isValidSubscription, so I'd suggest to rename them to isInvalid...

maksim-grebeniuk-sonarsource · 2024-12-04T09:59:24Z

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

-        secondaries.put(symbol.is(FUNCTION) ?
-          ((FunctionSymbol) symbol).definitionLocation() : ((ClassSymbol) symbol).definitionLocation(), String.format(SECONDARY_MESSAGE, symbol.name()));
-        return canHaveMethod(symbol, requiredMethod, classRequiredMethod);
+      var isValidSubscriptionSymbol = isValidSubscriptionSymbol(hasSymbol, subscriptionObject, secondaries, requiredMethod, classRequiredMethod);


to get rid of the cases where we check boolead for a null I'd probably will suggest to split the method to two methods:

get a symbol to validate (do the filtering if validation is not needed here)

validation itself returning simple boolean

maksim-grebeniuk-sonarsource

Overall looks good, some comments need to be addressed

maksim-grebeniuk-sonarsource · 2024-12-05T08:18:02Z

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

-      }
+
+    var symbol = TreeUtils.getSymbolFromTree(subscriptionObject)
+      .filter(s -> s.is(FUNCTION, CLASS))


as discussed - the previous logic was to return true even if a symbol is not a function or a class but has an fqn starting from typing or collection, need to revisit this condition

maksim-grebeniuk-sonarsource

Looks good! Thanks for the changes. As discussed, the potential problem that needs to be solved is the validation of non-function and non-class symbols from typing or collections modules

sonarqube-next · 2024-12-05T10:39:36Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

maksim-grebeniuk-sonarsource

LGTM

ghislainpiot force-pushed the SONARPY-2370 branch from 60ac144 to c2023cc Compare December 3, 2024 14:40

SONARPY-2370 Fix FP on S5644 for generic types defined through type p…

d2a95c3

…arameter syntax

ghislainpiot force-pushed the SONARPY-2370 branch from c2023cc to d2a95c3 Compare December 3, 2024 15:19

ghislainpiot commented Dec 3, 2024

View reviewed changes

ghislainpiot requested a review from maksim-grebeniuk-sonarsource December 3, 2024 16:06

maksim-grebeniuk-sonarsource reviewed Dec 4, 2024

View reviewed changes

maksim-grebeniuk-sonarsource requested changes Dec 4, 2024

View reviewed changes

ghislainpiot force-pushed the SONARPY-2370 branch from 96d04ad to a5ad4a9 Compare December 4, 2024 15:41

Address review comments

5d17b41

ghislainpiot force-pushed the SONARPY-2370 branch from a5ad4a9 to 5d17b41 Compare December 4, 2024 15:42

ghislainpiot requested a review from maksim-grebeniuk-sonarsource December 4, 2024 16:02

maksim-grebeniuk-sonarsource reviewed Dec 5, 2024

View reviewed changes

maksim-grebeniuk-sonarsource requested changes Dec 5, 2024

View reviewed changes

Second round of review

0d8345c

ghislainpiot requested a review from maksim-grebeniuk-sonarsource December 5, 2024 09:51

Third round of review

babb9e8

ghislainpiot force-pushed the SONARPY-2370 branch from fee7634 to babb9e8 Compare December 5, 2024 10:34

maksim-grebeniuk-sonarsource approved these changes Dec 5, 2024

View reviewed changes

ghislainpiot merged commit 5390bf6 into master Dec 5, 2024
11 checks passed

ghislainpiot deleted the SONARPY-2370 branch December 5, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-2370 Fix FP on S5644 for generic types defined through type parameter syntax #2209

SONARPY-2370 Fix FP on S5644 for generic types defined through type parameter syntax #2209

ghislainpiot commented Dec 3, 2024 •

edited by jira bot

Loading

ghislainpiot Dec 3, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource Dec 4, 2024

maksim-grebeniuk-sonarsource left a comment

maksim-grebeniuk-sonarsource Dec 5, 2024

maksim-grebeniuk-sonarsource left a comment

sonarqube-next bot commented Dec 5, 2024

maksim-grebeniuk-sonarsource left a comment


		class MyGenericClass[T]: ...

		class MyGenericSubType(MyGenericClass[str]): ... # FP S5644

SONARPY-2370 Fix FP on S5644 for generic types defined through type parameter syntax #2209

SONARPY-2370 Fix FP on S5644 for generic types defined through type parameter syntax #2209

Conversation

ghislainpiot commented Dec 3, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksim-grebeniuk-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksim-grebeniuk-sonarsource left a comment

Choose a reason for hiding this comment

sonarqube-next bot commented Dec 5, 2024

Quality Gate passed

maksim-grebeniuk-sonarsource left a comment

Choose a reason for hiding this comment

ghislainpiot commented Dec 3, 2024 •

edited by jira bot

Loading