SONARPY-1771 Rule S6972: Nested estimator parameters adjustment in a Pipeline should refer to valid parameters (#1789)

Author: ghislainpiot

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

@@ -350,9 +350,10 @@ public static Iterable<Class> getChecks() {
       StringReplaceCheck.class,
       StrongCryptographicKeysCheck.class,
       SklearnCachedPipelineDontAccessTransformersCheck.class,
-      SuperfluousCurlyBraceCheck.class,
-      SklearnPipelineSpecifyMemoryArgumentCheck.class,
       SklearnEstimatorHyperparametersCheck.class,
+      SklearnPipelineSpecifyMemoryArgumentCheck.class,
+      SklearnPipelineParameterAreCorrectCheck.class,
+      SuperfluousCurlyBraceCheck.class,
       TempFileCreationCheck.class,
       ImplicitlySkippedTestCheck.class,
       ToDoCommentCheck.class,

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

@@ -0,0 +1,330 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2024 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Collector;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.ClassSymbol;
+import org.sonar.plugins.python.api.symbols.FunctionSymbol;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.DictionaryLiteral;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.ExpressionList;
+import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.QualifiedExpression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.StringLiteral;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.python.checks.utils.Expressions;
+import org.sonar.python.tree.DictionaryLiteralImpl;
+import org.sonar.python.tree.KeyValuePairImpl;
+import org.sonar.python.tree.ListLiteralImpl;
+import org.sonar.python.tree.TreeUtils;
+import org.sonar.python.tree.TupleImpl;
+
+@Rule(key = "S6972")
+public class SklearnPipelineParameterAreCorrectCheck extends PythonSubscriptionCheck {
+
+  public static final String MESSAGE = "Provide valid parameters to the estimator.";
+  private static final Set<String> SKLEARN_SEARCH_FQNS = Set.of(
+    "sklearn.model_selection._search.GridSearchCV",
+    "sklearn.model_selection._search_successive_halving.HalvingGridSearchCV",
+    "sklearn.model_selection._search.RandomizedSearchCV",
+    "sklearn.model_selection._search_successive_halving.HalvingRandomSearchCV");
+
+  private record PipelineNameAndParsedParameters(Name pipelineName, Map<String, Set<ParameterNameAndLocation>> parsedParameters) {
+  }
+  private record ExpressionAndPrefix(List<Expression> tuple, String prefix, int depth) {
+  }
+  private record ParameterNameAndLocation(String string, Tree tree) {
+  }
+  private record StepAndClassifier(String stepName, ClassSymbol classifierName) {
+  }
+  private record StepAndParameter(String step, String parameter, Tree location) {
+  }
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, SklearnPipelineParameterAreCorrectCheck::checkCallExpression);
+  }
+
+  private static void checkCallExpression(SubscriptionContext subscriptionContext) {
+    CallExpression callExpression = (CallExpression) subscriptionContext.syntaxNode();
+
+    var parsedFunctionOptional = Optional.ofNullable(callExpression.calleeSymbol())
+      .map(Symbol::fullyQualifiedName)
+      .filter(SKLEARN_SEARCH_FQNS::contains)
+      .map(callExpr -> getStepAndParametersFromDict(callExpression))
+      .flatMap(parsedParameters -> getPipelineNameAndParsedParametersFromSearchFunctions(parsedParameters, callExpression))
+      .or(() -> Optional.ofNullable(callExpression.calleeSymbol())
+        .map(Symbol::fullyQualifiedName)
+        .filter("sklearn.pipeline.Pipeline.set_params"::equals)
+        .map(callExpr -> getStepAndParametersFromArguments(callExpression))
+        .flatMap(parsedParameters -> getPipelineNameAndParsedParametersFromPipelineSetParamsFunction(parsedParameters, callExpression)));
+
+    parsedFunctionOptional.ifPresent(
+      pipelineNameAndParsedParameters -> {
+        var parsedFunction = pipelineNameAndParsedParameters.parsedParameters;
+        var pipelineName = pipelineNameAndParsedParameters.pipelineName;
+        Expressions.singleAssignedNonNameValue(pipelineName)
+          .flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
+          .ifPresent(pipelineCallExpr -> findProblems(parsedFunction, parsePipeline(pipelineCallExpr), subscriptionContext));
+      });
+  }
+
+  private static Optional<PipelineNameAndParsedParameters> getPipelineNameAndParsedParametersFromPipelineSetParamsFunction(
+    Map<String, Set<ParameterNameAndLocation>> parsedParameters,
+    CallExpression callExpression) {
+    return newPipelineNameAndParsedParameters(
+      Optional.of(callExpression).map(CallExpression::callee)
+        .flatMap(TreeUtils.toOptionalInstanceOfMapper(QualifiedExpression.class))
+        .map(QualifiedExpression::qualifier)
+        .flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
+        .orElse(null),
+      parsedParameters);
+  }
+
+  private static Optional<PipelineNameAndParsedParameters> getPipelineNameAndParsedParametersFromSearchFunctions(Map<String, Set<ParameterNameAndLocation>> parsedParameters,
+    CallExpression callExpression) {
+    return newPipelineNameAndParsedParameters(
+      Optional.ofNullable(TreeUtils.nthArgumentOrKeyword(0, "estimator", callExpression.arguments()))
+        .flatMap(TreeUtils.toOptionalInstanceOfMapper(RegularArgument.class))
+        .map(RegularArgument::expression)
+        .flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
+        .orElse(null),
+      parsedParameters);
+  }
+
+  private static Optional<PipelineNameAndParsedParameters> newPipelineNameAndParsedParameters(@Nullable Name pipelineName,
+    Map<String, Set<ParameterNameAndLocation>> parsedParameters) {
+    return Optional.ofNullable(pipelineName)
+      .map(pipelineName1 -> new PipelineNameAndParsedParameters(pipelineName1, parsedParameters));
+  }
+
+  private static void findProblems(Map<String, Set<ParameterNameAndLocation>> setParameters, Map<String, ClassSymbol> pipelineDefinition, SubscriptionContext subscriptionContext) {
+    for (var entry : setParameters.entrySet()) {
+      var step = entry.getKey();
+      var stringAndTree = entry.getValue();
+      var parameters = stringAndTree.stream().map(ParameterNameAndLocation::string).collect(Collectors.toSet());
+
+      var classifier = pipelineDefinition.get(step);
+      if (classifier == null) {
+        continue;
+      }
+      var possibleParameters = getInitFunctionSymbol(classifier).map(FunctionSymbol::parameters).orElse(List.of());
+
+      parameters.forEach(parameter -> {
+        if (isNotAValidParameter(parameter, possibleParameters)) {
+          createIssue(subscriptionContext, parameter, stringAndTree);
+        }
+      });
+    }
+  }
+
+  private static void createIssue(SubscriptionContext subscriptionContext, String parameter, Set<ParameterNameAndLocation> parameterNameAndLocation) {
+    parameterNameAndLocation
+      .stream()
+      .filter(parameterNameAndLocation1 -> parameterNameAndLocation1.string()
+        .equals(parameter))
+      .findFirst()
+      .ifPresent(location -> subscriptionContext.addIssue(location.tree, MESSAGE));
+  }
+
+  private static boolean isNotAValidParameter(String parameter, List<FunctionSymbol.Parameter> possibleParameters) {
+    return possibleParameters.stream().noneMatch(symbol -> Objects.equals(symbol.name(), parameter));
+  }
+
+  private static Optional<FunctionSymbol> getInitFunctionSymbol(ClassSymbol classSymbol) {
+    return classSymbol.declaredMembers().stream().filter(
+      memberSymbol -> "__init__".equals(memberSymbol.name())).findFirst().map(FunctionSymbol.class::cast);
+  }
+
+  private static Stream<Expression> getExpressionsFromArgument(@Nullable RegularArgument argument) {
+    return Optional.ofNullable(argument)
+      .map(RegularArgument::expression)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(ListLiteralImpl.class))
+      .map(ListLiteralImpl::elements)
+      .map(ExpressionList::expressions)
+      .stream()
+      .flatMap(Collection::stream);
+  }
+
+  private static Map<String, ClassSymbol> parsePipeline(CallExpression callExpression) {
+    var stepsArgument = TreeUtils.nthArgumentOrKeyword(0, "steps", callExpression.arguments());
+    var out = new HashMap<String, ClassSymbol>();
+
+    getExpressionsFromArgument(stepsArgument)
+      .map(
+        TreeUtils.toInstanceOfMapper(TupleImpl.class))
+      .filter(Objects::nonNull)
+      .map(TupleImpl::elements)
+      .filter(SklearnPipelineParameterAreCorrectCheck::isTwoElementTuple)
+      // If we find a pipeline inside the pipeline, we need to parse it recursively
+      .map(SklearnPipelineParameterAreCorrectCheck::createEmptyExpressionAndPrefix)
+      .flatMap(expandRecursivePipelines())
+      .forEach(
+        expressionAndPrefix -> getResult(expressionAndPrefix.tuple())
+          .ifPresent(stepAndClassifier1 -> out.put(expressionAndPrefix.prefix() + stepAndClassifier1.stepName(), stepAndClassifier1.classifierName())));
+    return out;
+  }
+
+  private static boolean isTwoElementTuple(List<Expression> elements) {
+    return elements.size() == 2;
+  }
+
+  private static ExpressionAndPrefix createEmptyExpressionAndPrefix(List<Expression> tuple) {
+    return new ExpressionAndPrefix(tuple, "", 0);
+  }
+
+  private static Function<ExpressionAndPrefix, Stream<ExpressionAndPrefix>> expandRecursivePipelines() {
+    return expressionAndPrefix -> {
+      var tuple = expressionAndPrefix.tuple();
+      var step = tuple.get(0);
+      var classifier = tuple.get(1);
+
+      if (!step.is(Tree.Kind.STRING_LITERAL) || !classifier.is(Tree.Kind.NAME)) {
+        return Stream.of(expressionAndPrefix);
+      }
+      if (expressionAndPrefix.depth > 10) {
+        return Stream.of(expressionAndPrefix);
+      }
+
+      return classifierIsANestedPipeline((Name) classifier)
+        .map(callExpression -> TreeUtils.nthArgumentOrKeyword(0, "steps", callExpression.arguments()))
+        .map(SklearnPipelineParameterAreCorrectCheck::getExpressionsFromArgument)
+        .orElse(Stream.empty())
+        .map(TreeUtils.toInstanceOfMapper(TupleImpl.class))
+        .filter(Objects::nonNull)
+        .map(TupleImpl::elements)
+        .filter(SklearnPipelineParameterAreCorrectCheck::isTwoElementTuple)
+        .map(elements -> new ExpressionAndPrefix(elements,
+          expressionAndPrefix.prefix() + ((StringLiteral) step).trimmedQuotesValue() + "__", expressionAndPrefix.depth + 1))
+        .flatMap(expandRecursivePipelines());
+    };
+  }
+
+  private static Optional<CallExpression> classifierIsANestedPipeline(Name classifier) {
+    return Expressions.singleAssignedNonNameValue(classifier)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
+      .filter(callExpression -> Optional.of(callExpression)
+        .map(CallExpression::calleeSymbol).map(Symbol::fullyQualifiedName)
+        .filter("sklearn.pipeline.Pipeline"::equals)
+        .isPresent());
+  }
+
+  private static Optional<StepAndClassifier> getResult(List<Expression> tuple) {
+    var step = tuple.get(0);
+    var classifier = tuple.get(1);
+    var stepName = Optional.ofNullable(step)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(StringLiteral.class))
+      .map(StringLiteral::trimmedQuotesValue);
+    var classifierName = Optional.ofNullable(classifier)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
+      .map(CallExpression::calleeSymbol)
+      .filter(symbol -> symbol.is(Symbol.Kind.CLASS) && !"sklearn.pipeline.Pipeline".equals(symbol.fullyQualifiedName()))
+      .map(ClassSymbol.class::cast);
+
+    return stepName.flatMap(stepName1 -> classifierName.map(classifierName1 -> new StepAndClassifier(stepName1, classifierName1)));
+  }
+
+  private static Map<String, Set<ParameterNameAndLocation>> getStepAndParametersFromArguments(CallExpression callExpression) {
+    return callExpression.arguments()
+      .stream()
+      .filter(argument -> argument.is(Tree.Kind.REGULAR_ARGUMENT))
+      .map(RegularArgument.class::cast)
+      .map(RegularArgument::keywordArgument)
+      .filter(Objects::nonNull)
+      .map(SklearnPipelineParameterAreCorrectCheck::getStepAndParameterFromName)
+      .<StepAndParameter>mapMulti(Optional::ifPresent)
+      .collect(mergeStringAndTreeToMapCollector());
+  }
+
+  private static Map<String, Set<ParameterNameAndLocation>> getStepAndParametersFromDict(CallExpression callExpression) {
+    return Optional.ofNullable(TreeUtils.nthArgumentOrKeyword(1, "param_grid", callExpression.arguments()))
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(RegularArgument.class))
+      .map(RegularArgument::expression)
+      .stream()
+      .flatMap(SklearnPipelineParameterAreCorrectCheck::extractKeyValuePairFromDictLiteral)
+      .map(KeyValuePairImpl::key)
+      .map(TreeUtils.toInstanceOfMapper(StringLiteral.class))
+      .filter(Objects::nonNull)
+      .map(stringLiteral -> getStepAndParameterFromString(stringLiteral.trimmedQuotesValue(), stringLiteral))
+      .<StepAndParameter>mapMulti(Optional::ifPresent)
+      .collect(
+        mergeStringAndTreeToMapCollector());
+  }
+
+  private static Stream<KeyValuePairImpl> extractKeyValuePairFromDictLiteral(Expression expression) {
+    return Optional.of(expression)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
+      .flatMap(Expressions::singleAssignedNonNameValue)
+      .flatMap(TreeUtils.toOptionalInstanceOfMapper(DictionaryLiteralImpl.class))
+      .map(DictionaryLiteral::elements)
+      .stream()
+      .flatMap(Collection::stream)
+      .map(TreeUtils.toInstanceOfMapper(KeyValuePairImpl.class))
+      .filter(Objects::nonNull);
+  }
+
+  private static Collector<StepAndParameter, ?, Map<String, Set<ParameterNameAndLocation>>> mergeStringAndTreeToMapCollector() {
+    return Collectors.toMap(StepAndParameter::step, stepAndParameter -> Set.of(new ParameterNameAndLocation(stepAndParameter.parameter, stepAndParameter.location)),
+      (set1, set2) -> {
+        var set = new HashSet<>(set1);
+        set.addAll(set2);
+        return set;
+      });
+  }
+
+  private static Optional<StepAndParameter> getStepAndParameterFromName(Name name) {
+    return splitStepString(name.name()).map(split -> {
+      var splitsNotLast = Arrays.stream(split).limit(split.length - 1L).collect(Collectors.joining("__"));
+      return new StepAndParameter(splitsNotLast, split[split.length - 1], name);
+    });
+  }
+
+  private static Optional<StepAndParameter> getStepAndParameterFromString(String string, Tree location) {
+    return splitStepString(string).map(split -> new StepAndParameter(split[0], split[1], location));
+  }
+
+  private static Optional<String[]> splitStepString(String string) {
+    var split = string.split("__");
+    if (split.length < 2 || string.endsWith("__")) {
+      return Optional.empty();
+    }
+    return Optional.of(split);
+  }
+
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6972.html

@@ -0,0 +1,47 @@
+<p>This rule raises an issue when an invalid nested estimator parameter is set on a Pipeline.</p>
+<h2>Why is this an issue?</h2>
+<p>In the sklearn library, when using the <code>Pipeline</code> class, it is possible to modify the parameters of the nested estimators. This
+modification can be done by using the <code>Pipeline</code> method <code>set_params</code> and specifying the name of the estimator and the parameter
+to update separated by a double underscore <code>__</code>.</p>
+<pre>
+from sklearn.pipeline import Pipeline
+from sklearn.svm import SVC
+
+pipe = Pipeline(steps=[("clf", SVC())])
+pipe.set_params(clf__C=10)
+</pre>
+<p>In the example above, the regularization parameter <code>C</code> is set to the value <code>10</code> for the classifier called <code>clf</code>.
+Setting such parameters can be done as well with the help of the <code>param_grid</code> parameter for example when using
+<code>GridSearchCV</code>.</p>
+<p>Providing invalid parameters that do not exist on the estimator can lead to unexpected behavior or runtime errors.</p>
+<p>This rule checks that the parameters provided to the <code>set_params</code> method of a Pipeline instance or through the <code>param_grid</code>
+parameters of a <code>GridSearchCV</code> are valid for the nested estimators.</p>
+<h2>How to fix it</h2>
+<p>To fix this issue provide valid parameters to the nested estimators.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+from sklearn.pipeline import Pipeline
+from sklearn.decomposition import PCA
+
+pipe = Pipeline(steps=[('reduce_dim', PCA())])
+pipe.set_params(reduce_dim__C=2) # Noncompliant: the parameter C does not exists for the PCA estimator
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+from sklearn.pipeline import Pipeline
+from sklearn.decomposition import PCA
+
+pipe = Pipeline(steps=[('reduce_dim', PCA())])
+pipe.set_params(reduce_dim__n_components=2) # Compliant
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Scikit-Learn documentation - <a href="https://scikit-learn.org/stable/modules/compose.html#access-to-nested-parameters">Access to nested
+  parameters</a> </li>
+  <li> Scikit-Learn documentation - <a
+  href="https://scikit-learn.org/stable/modules/generated/sklearn.model_selection.GridSearchCV.html#sklearn-model-selection-gridsearchcv">GridSearchCV
+  reference</a> </li>
+</ul>
+