SONARPY-2147 fix rule S6973 (#1981)

* SONARPY-2147 fix message bug

* SONARPY-2147 fix nested searchcv bug

Author: Seppli11

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

@@ -19,6 +19,7 @@
  */
 package org.sonar.python.checks;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -74,19 +75,20 @@ private static void checkEstimator(SubscriptionContext ctx) {
   }
 
   private static void checkPyTorchOptimizer(String name, CallExpression callExpression, SubscriptionContext ctx) {
-    PyTorchCheck.getMissingParameters(name, callExpression)
-      .map(MissingHyperParameterCheck::toParameterNames)
-      .ifPresent(parameters -> ctx.addIssue(callExpression, formatMessage(parameters, PYTORCH_MESSAGE)));
-  }
+    List<String> missingParams = PyTorchCheck.getMissingParameters(name, callExpression).stream()
+      .map(Param::name)
+      .toList();
 
-  private static void checkSkLearnEstimator(String name, CallExpression callExpression, SubscriptionContext ctx) {
-    SkLearnCheck.getMissingParameters(name, callExpression)
-      .map(MissingHyperParameterCheck::toParameterNames)
-      .ifPresent(parameters -> ctx.addIssue(callExpression, formatMessage(parameters, SKLEARN_MESSAGE)));
+    if (!missingParams.isEmpty()) {
+      ctx.addIssue(callExpression, formatMessage(missingParams, PYTORCH_MESSAGE));
+    }
   }
 
-  private static List<String> toParameterNames(List<Param> parameters) {
-    return parameters.stream().map(Param::name).toList();
+  private static void checkSkLearnEstimator(String name, CallExpression callExpression, SubscriptionContext ctx) {
+    List<String> missingParams = SkLearnCheck.getMissingParameters(name, callExpression).stream().map(Param::name).toList();
+    if(!missingParams.isEmpty()) {
+      ctx.addIssue(callExpression, formatMessage(missingParams, SKLEARN_MESSAGE));
+    }
   }
 
   private static String formatMessage(List<String> missingArgs, String formatString) {
@@ -101,12 +103,12 @@ private static String formatMessage(List<String> missingArgs, String formatStrin
 
 
   // common method used by both the PyTorchCheck class and SkLearnCheck class
-  private static boolean isMissingAHyperparameter(CallExpression callExpression, List<Param> parametersToCheck) {
+  private static List<Param> filterUsedHyperparameter(CallExpression callExpression, List<Param> parametersToCheck) {
     return parametersToCheck.stream()
-      .map(param -> param.position()
+      .filter(param -> param.position()
         .map(position -> TreeUtils.nthArgumentOrKeyword(position, param.name, callExpression.arguments()))
-        .orElse(TreeUtils.argumentByKeyword(param.name, callExpression.arguments())))
-      .anyMatch(Objects::isNull);
+        .orElse(TreeUtils.argumentByKeyword(param.name, callExpression.arguments())) == null)
+      .toList();
   }
 
   private static class PyTorchCheck {
@@ -130,10 +132,11 @@ private static class PyTorchCheck {
       Map.entry("torch.optim.SGD", List.of(new Param(LR, 1), new Param("momentum", 2), new Param(WEIGHT_DECAY, 4)))
     );
 
-    public static Optional<List<Param>> getMissingParameters(String name, CallExpression callExpression) {
+    public static List<Param> getMissingParameters(String name, CallExpression callExpression) {
       return Optional.ofNullable(PY_TORCH_ESTIMATORS_AND_PARAMETERS_TO_CHECK.get(name))
         .filter(parameters -> !Expressions.containsSpreadOperator(callExpression.arguments()))
-        .filter(parameters -> isMissingAHyperparameter(callExpression, parameters));
+        .map(parameters -> filterUsedHyperparameter(callExpression, parameters))
+        .orElse(Collections.emptyList());
     }
   }
 
@@ -175,23 +178,24 @@ private static class SkLearnCheck {
       "sklearn.model_selection._search_successive_halving.HalvingRandomSearchCV",
       "sklearn.model_selection._search_successive_halving.HalvingGridSearchCV");
 
-    private static final Set<String> PIPELINE_FQNS = Set.of(
-      "sklearn.pipeline.make_pipeline",
-      "sklearn.pipeline.Pipeline");
-
-    public static Optional<List<Param>> getMissingParameters(String name, CallExpression callExpression) {
+    public static List<Param> getMissingParameters(String name, CallExpression callExpression) {
       return Optional.ofNullable(SK_LEARN_ESTIMATORS_AND_PARAMETERS_TO_CHECK.get(name))
         .filter(parameters -> !isDirectlyUsedInSearchCV(callExpression))
         .filter(parameters -> !isSetParamsCalled(callExpression))
         .filter(parameters -> !isPartOfPipelineAndSearchCV(callExpression))
-        .filter(parameters -> isMissingAHyperparameter(callExpression, parameters));
+        .map(parameters -> filterUsedHyperparameter(callExpression, parameters))
+        .orElse(Collections.emptyList());
     }
 
     private static boolean isDirectlyUsedInSearchCV(CallExpression callExpression) {
-      return Optional.ofNullable(TreeUtils.firstAncestorOfKind(callExpression, REGULAR_ARGUMENT))
-        .flatMap(TreeUtils.toOptionalInstanceOfMapper(RegularArgument.class))
-        .map(SkLearnCheck::isArgumentPartOfSearchCV)
-        .orElse(false);
+      Tree current = callExpression;
+      do{
+        current = TreeUtils.firstAncestorOfKind(current, REGULAR_ARGUMENT);
+        if(current instanceof RegularArgument arg && isArgumentPartOfSearchCV(arg)) {
+          return true;
+        }
+      } while(current != null);
+      return false;
     }
 
     private static boolean isSetParamsCalled(CallExpression callExpression) {
@@ -216,8 +220,6 @@ private static boolean isUsedWithSetParams(List<Usage> usages) {
     private static boolean isPartOfPipelineAndSearchCV(CallExpression callExpression) {
       return Expressions.getAssignedName(callExpression)
         .map(SkLearnCheck::isEstimatorUsedInSearchCV)
-        .or(() -> getPipelineAssignement(callExpression)
-          .map(SkLearnCheck::isEstimatorUsedInSearchCV))
         .orElse(false);
     }
 
@@ -241,15 +243,5 @@ private static boolean isArgumentPartOfSearchCV(RegularArgument arg) {
         .map(SEARCH_CV_FQNS::contains)
         .orElse(false);
     }
-
-    private static Optional<Name> getPipelineAssignement(CallExpression callExpression) {
-      return Optional.ofNullable(TreeUtils.firstAncestorOfKind(callExpression, CALL_EXPR))
-        .flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
-        .filter(callExp -> Optional.ofNullable(callExp.calleeSymbol())
-          .map(Symbol::fullyQualifiedName)
-          .map(PIPELINE_FQNS::contains)
-          .orElse(false))
-        .flatMap(Expressions::getAssignedName);
-    }
   }
 }

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

@@ -15,7 +15,7 @@ def adadelta(param_dict, param_list, rho, eps):
     from torch.optim import Adadelta
     noncompliant = Adadelta() # Noncompliant {{Add the missing hyperparameters lr and weight_decay for this PyTorch optimizer.}}
     noncompliant = Adadelta(some_extra_variable=3) # Noncompliant {{Add the missing hyperparameters lr and weight_decay for this PyTorch optimizer.}}
-    noncompliant = Adadelta(model.parameters(), lr = 0.001) # Noncompliant
+    noncompliant = Adadelta(model.parameters(), lr = 0.001) # Noncompliant {{Add the missing hyperparameter weight_decay for this PyTorch optimizer.}}
     noncompliant = Adadelta(model.parameters(), weight_decay = 0.32) # Noncompliant
 
     optimizer = Adadelta(model.parameters(), 0.001, rho, eps, 0.23)

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

@@ -5,7 +5,7 @@
         RandomForestClassifier, RandomForestRegressor,
         )
 from sklearn.linear_model import ElasticNet
-from sklearn.neighbors import NearestNeighbors, KNeighborsClassifier,KNeighborsRegressor 
+from sklearn.neighbors import NearestNeighbors, KNeighborsClassifier,KNeighborsRegressor
 from sklearn.svm import SVC, SVR, NuSVC, NuSVR
 from sklearn.tree import DecisionTreeClassifier, DecisionTreeRegressor
 from sklearn.neural_network import MLPClassifier, MLPRegressor
@@ -36,7 +36,7 @@ def non_compliant():
     SVC(random_state=42) # Noncompliant
     SVC(C=1) # Noncompliant
     SVR() # Noncompliant {{Add the missing hyperparameters C, kernel and gamma for this Scikit-learn estimator.}}
-    SVR(C=1.2, kernel="poly") # Noncompliant
+    SVR(C=1.2, kernel="poly") # Noncompliant {{Add the missing hyperparameter gamma for this Scikit-learn estimator.}}
     NuSVC() # Noncompliant
     NuSVR(gamma="scale", kernel="poly") # Noncompliant
 
@@ -95,7 +95,10 @@ def compliant():
 
     pipe2 = Pipeline([('svc'), SVC()]) # FN
 
-    grid2 = GridSearchCV(pipe2, param_grid={'svc__C': [1, 10, 100]}) 
+    grid2 = GridSearchCV(pipe2, param_grid={'svc__C': [1, 10, 100]})
+
+    grid3 = GridSearchCV(make_pipeline(SVC()), param_grid={'svc__C': [1, 10, 100]})
+    grid4 = GridSearchCV(some_method(some_other_method(make_pipeline(SVC()))), param_grid={'svc__C': [1, 10, 100]})
 
     GridSearchCV(s, param_grid={'svc__C': [1, 10, 100]}) # FN