SONARPY-3124 S6709: Exclude SVC (#364)

Co-authored-by: Thomas Serre <thomas.serre@sonarsource.com>
GitOrigin-RevId: e5684d58f5f1f2af154caeb7ae31db9786247dac

Author: 2 people

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

@@ -33,9 +33,13 @@
 import org.sonar.plugins.python.api.tree.RegularArgument;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.cfg.fixpoint.ReachingDefinitionsAnalysis;
+import org.sonar.python.checks.cdk.CdkPredicate;
 import org.sonar.python.semantic.ClassSymbolImpl;
 import org.sonar.python.semantic.SymbolUtils;
 import org.sonar.python.tree.TreeUtils;
+import org.sonar.python.types.v2.TypeCheckMap;
+
+import static org.sonar.python.checks.hotspots.CommonValidationUtils.singleAssignedString;
 
 @Rule(key = "S6709")
 public class RandomSeedCheck extends PythonSubscriptionCheck {
@@ -61,10 +65,44 @@ public class RandomSeedCheck extends PythonSubscriptionCheck {
 
   private ReachingDefinitionsAnalysis reachingDefinitionsAnalysis;
 
+  private static Predicate<CallExpression> keywordAbsentOrNotIn(String keyword, String... restrictedValues) {
+    Set<String> restrictedValueSet = Set.of(restrictedValues);
+    return call -> {
+      var arg = TreeUtils.argumentByKeyword(keyword, call.arguments());
+      if (arg == null) {
+        return true;
+      }
+      String expressionString = singleAssignedString(arg.expression());
+      return restrictedValueSet.stream().noneMatch(expressionString::equals);
+    };
+  }
+  private static Predicate<CallExpression> probabilityArgAbsent() {
+    return call -> {
+      var probabilityArg = TreeUtils.argumentByKeyword("probability", call.arguments());
+      return probabilityArg == null || CdkPredicate.isFalse().test(probabilityArg.expression());
+    };
+  }
+
+  private static final Predicate<CallExpression> SOLVER_NOT_SAG_SAGA = keywordAbsentOrNotIn("solver", "sag", "saga");
+  private static final Predicate<CallExpression> SELECTION_NOT_RANDOM = keywordAbsentOrNotIn("selection", "random");
+
+  private static final Map<String, Predicate<CallExpression>> SKLEARN_EXCEPTIONS = Map.ofEntries(
+    Map.entry("sklearn.svm._classes.SVC", probabilityArgAbsent()),
+    Map.entry("sklearn.linear_model._logistic.LogisticRegression", SOLVER_NOT_SAG_SAGA),
+    Map.entry("sklearn.linear_model._ridge.Ridge", SOLVER_NOT_SAG_SAGA),
+    Map.entry("sklearn.linear_model._coordinate_descent.Lasso", SELECTION_NOT_RANDOM),
+    Map.entry("sklearn.linear_model._coordinate_descent.ElasticNet", SELECTION_NOT_RANDOM));
+
+  private TypeCheckMap<Predicate<CallExpression>> typeCheckMap;
+
   @Override
   public void initialize(Context context) {
     context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT,
-      ctx -> this.reachingDefinitionsAnalysis = new ReachingDefinitionsAnalysis(ctx.pythonFile()));
+      ctx -> {
+        this.reachingDefinitionsAnalysis = new ReachingDefinitionsAnalysis(ctx.pythonFile());
+        this.typeCheckMap = new TypeCheckMap<>();
+        SKLEARN_EXCEPTIONS.forEach((fqn, predicate) -> this.typeCheckMap.put(ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(fqn), predicate));
+      });
     context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::checkEmptySeedCall);
   }
 
@@ -82,6 +120,7 @@ private void checkEmptySeedCall(SubscriptionContext ctx) {
         .filter(symbol -> symbol.fullyQualifiedName() != null && symbol.fullyQualifiedName().startsWith(SKLEARN_FQN))
         .filter(RandomSeedCheck::hasRandomStateParameter)
         .filter(symbol -> isArgumentAbsentOrNone(TreeUtils.argumentByKeyword(SKLEARN_ARG_NAME, call.arguments())))
+        .filter(symbol -> !isSKLearnException(call))
         .map(symbol -> SKLEARN_MESSAGE))
       .ifPresent(message -> ctx.addIssue(call.callee(), message));
   }
@@ -128,4 +167,11 @@ private boolean isAssignedNone(Expression exp) {
       .filter(Predicate.not(Set::isEmpty))
       .filter(values -> values.stream().allMatch(value -> value.is(Tree.Kind.NONE))).isPresent();
   }
+
+  private boolean isSKLearnException(CallExpression call) {
+    var calleeType = call.callee().typeV2();
+    return typeCheckMap.getOptionalForType(calleeType)
+      .map(predicate -> predicate.test(call))
+      .orElse(false);
+  }
 }

python-checks/src/main/java/org/sonar/python/checks/hotspots/CommonValidationUtils.java

@@ -17,6 +17,7 @@
 package org.sonar.python.checks.hotspots;
 
 import java.util.function.BiConsumer;
+
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.tree.BinaryExpression;
 import org.sonar.plugins.python.api.tree.CallExpression;
@@ -99,7 +100,7 @@ private static boolean isNumericLiteralEqualToDouble(NumericLiteral numericLiter
     }
   }
 
-  static String singleAssignedString(Expression expression) {
+  public static String singleAssignedString(Expression expression) {
     if (expression instanceof Name name) {
       return Expressions.singleAssignedNonNameValue(name)
         .map(CommonValidationUtils::singleAssignedString)
@@ -108,6 +109,18 @@ static String singleAssignedString(Expression expression) {
     return expression.is(Tree.Kind.STRING_LITERAL) ? ((StringLiteral) expression).trimmedQuotesValue() : "";
   }
 
+  public static boolean isStringEqualTo(Expression expression, String expected) {
+    if (expression instanceof Name name) {
+      return Expressions.singleAssignedNonNameValue(name)
+        .map(value -> isStringEqualTo(value, expected))
+        .orElse(false);
+    }
+    if (expression instanceof StringLiteral stringLiteral) {
+      return expected.equals(stringLiteral.trimmedQuotesValue());
+    }
+    return false;
+  }
+
   interface CallValidator {
     void validate(SubscriptionContext ctx, CallExpression callExpression);
   }

python-checks/src/test/java/org/sonar/python/checks/hotspots/CommonValidationUtilsTest.java

@@ -53,8 +53,30 @@ private static void checkCallExpr(SubscriptionContext ctx) {
     }
   }
 
+  static class isStringEqualToTestCheck extends PythonSubscriptionCheck {
+    @Override
+    public void initialize(Context context) {
+      context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, isStringEqualToTestCheck::checkCallExpr);
+    }
+
+    private static void checkCallExpr(SubscriptionContext ctx) {
+      CallExpression callExpression = (CallExpression) ctx.syntaxNode();
+      TreeUtils.nthArgumentOrKeywordOptional(0, "arg", callExpression.arguments())
+        .ifPresent(argument -> {
+          if (CommonValidationUtils.isStringEqualTo(argument.expression(), "abc")) {
+            ctx.addIssue(argument, "Argument is abc");
+          }
+        });
+    }
+  }
+
   @Test
   void isLessThan() {
     PythonCheckVerifier.verify("src/test/resources/checks/commonValidationUtils.py", new isLessThanMoreThanTestCheck());
   }
+
+  @Test
+  void isStringEqualTo() {
+    PythonCheckVerifier.verify("src/test/resources/checks/commonValidationUtilsString.py", new isStringEqualToTestCheck());
+  }
 }

python-checks/src/test/resources/checks/commonValidationUtilsString.py

@@ -0,0 +1,18 @@
+callExpr("abc")  # Noncompliant {{Argument is abc}}
+callExpr("def")
+
+abc_var = "abc"
+callExpr(abc_var)  # Noncompliant {{Argument is abc}}
+
+def_var = "def"
+callExpr(def_var)
+
+callExpr(arg="abc")  # Noncompliant {{Argument is abc}}
+callExpr(arg="def")
+
+callExpr(arg=f"{abc_var}")  # FN f-string are not supported
+callExpr(arg="def")
+
+callExpr(arg=3)
+not_a_string = 3
+callExpr(arg=not_a_string)

python-checks/src/test/resources/checks/randomSeedSKlearn.py

@@ -1,23 +1,58 @@
 from sklearn.model_selection import train_test_split
 from sklearn.svm import SVC
+from sklearn.neural_network import MLPClassifier
 from sklearn.datasets import load_iris, make_blobs
+from sklearn.linear_model import LogisticRegression
+from sklearn.linear_model import Ridge, Lasso, ElasticNet
 
 def failure():
     X, y = load_iris(return_X_y=True)
     X_train, X_test, y_train, y_test = train_test_split(X, y) # Noncompliant {{Provide a seed for the random_state parameter.}}
     #                                  ^^^^^^^^^^^^^^^^
-    svc = SVC() # Noncompliant
-    #     ^^^
+    svc = SVC() # Compliant
+
+    svc2 = SVC(probability=True) # Noncompliant
+    #      ^^^
+
+    mlp = MLPClassifier() # Noncompliant
 
     X, y = make_blobs(n_samples=1300, random_state=None) # Noncompliant
     #      ^^^^^^^^^^
 
+    lr_default = LogisticRegression() # Compliant
+
+    lr_sag = LogisticRegression(solver="sag") # Noncompliant
+    #        ^^^^^^^^^^^^^^^^^^
+
+    lr_saga = LogisticRegression(solver='saga') # Noncompliant
+    #         ^^^^^^^^^^^^^^^^^^
+
+    lr_unknown_string = LogisticRegression(solver=some_unknown_string) # Compliant
+
+    ridge_default = Ridge() # Compliant
+
+    ridge_sag = Ridge(solver="sag") # Noncompliant
+    #           ^^^^^
+
+    ridge_saga = Ridge(solver='saga') # Noncompliant
+    #            ^^^^^
+
+    lasso_default = Lasso() # Compliant
+
+    lasso_random = Lasso(selection="random") # Noncompliant
+    #              ^^^^^
+
+    elastic_default = ElasticNet() # Compliant
+
+    elastic_random = ElasticNet(selection='random') # Noncompliant
+    #                ^^^^^^^^^^
+
 def success():
     from sklearn.ensemble import RandomForestClassifier
     rfc = RandomForestClassifier(random_state=0) # Compliant
-    from sklearn.linear_model import SGDClassifier 
+    from sklearn.linear_model import SGDClassifier
     sgd = SGDClassifier(random_state=foo()) # Compliant
-    
+
     def sklearn_seed(rng):
         svc = SVC(random_state=rng) # Compliant
 
@@ -30,5 +65,5 @@ def ambiguous():
     from sklearn.svm import SVC as something
     from sklearn.datasets import make_blobs as something
 
-    something = something() # FN 
+    something = something() # FN