SONARPY-1780 Rule S6794: Subclasses of Scikit-Learn's "BaseEstimator" should not set attributes ending with "_" in the "__init__" method (#1768)

Author: ghislainpiot

its/ruling/src/test/resources/expected/python-S6974.json

@@ -0,0 +1,5 @@
+{
+"project:pecos/examples/qp2q/models/pecosq2q.py": [
+148
+]
+}

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

@@ -338,6 +338,7 @@ public static Iterable<Class> getChecks() {
       SingleCharacterAlternationCheck.class,
       SingleCharCharacterClassCheck.class,
       SkippedTestNoReasonCheck.class,
+      SkLearnEstimatorDontInitializeEstimatedValuesCheck.class,
       SpecialMethodParamListCheck.class,
       SpecialMethodReturnTypeCheck.class,
       SQLQueriesCheck.class,

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

@@ -0,0 +1,159 @@
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+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.quickfix.PythonQuickFix;
+import org.sonar.plugins.python.api.symbols.ClassSymbol;
+import org.sonar.plugins.python.api.tree.AssignmentStatement;
+import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
+import org.sonar.plugins.python.api.tree.ClassDef;
+import org.sonar.plugins.python.api.tree.QualifiedExpression;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.tree.Tuple;
+import org.sonar.python.quickfix.TextEditUtils;
+import org.sonar.python.tree.FunctionDefImpl;
+import org.sonar.python.tree.TreeUtils;
+
+@Rule(key = "S6974")
+public class SkLearnEstimatorDontInitializeEstimatedValuesCheck extends PythonSubscriptionCheck {
+
+  private static final String BASE_ESTIMATOR_FULLY_QUALIFIED_NAME = "sklearn.base.BaseEstimator";
+  private static final Set<String> MIXINS_FULLY_QUALIFIED_NAME = Set.of(
+    "sklearn.base.BiclusterMixin",
+    "sklearn.base.ClassifierMixin",
+    "sklearn.base.ClusterMixin",
+    "sklearn.base.DensityMixin",
+    "sklearn.base.MetaEstimatorMixin",
+    "sklearn.base.OneToOneFeatureMixin",
+    "sklearn.base.OutlierMixin",
+    "sklearn.base.RegressorMixin",
+    "sklearn.base.TransformerMixin");
+
+  private static final String MESSAGE = "Move this estimated attribute in the `fit` method.";
+  private static final String MESSAGE_SECONDARY = "The attribute is used in this estimator";
+  public static final String QUICK_FIX_MESSAGE = "Remove the statement";
+  public static final String QUICK_FIX_RENAME_MESSAGE = "Remove all trailing underscores from the variable name";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, SkLearnEstimatorDontInitializeEstimatedValuesCheck::checkFunction);
+  }
+
+  private static boolean inheritsMixin(ClassSymbol classSymbol) {
+    return MIXINS_FULLY_QUALIFIED_NAME.stream().anyMatch(classSymbol::isOrExtends);
+  }
+
+  private static void checkFunction(SubscriptionContext subscriptionContext) {
+    FunctionDefImpl functionDef = (FunctionDefImpl) subscriptionContext.syntaxNode();
+    if (!"__init__".equals(functionDef.name().name())) {
+      return;
+    }
+
+    var classDef = (ClassDef) TreeUtils.firstAncestorOfKind(functionDef, Tree.Kind.CLASSDEF);
+    if (classDef == null) {
+      return;
+    }
+    var classSymbol = TreeUtils.getClassSymbolFromDef(classDef);
+    if (classSymbol == null) {
+      return;
+    }
+    boolean inheritsBaseEstimator = Optional.of(classSymbol)
+      .map(classSymbol1 -> classSymbol1.isOrExtends(BASE_ESTIMATOR_FULLY_QUALIFIED_NAME))
+      .orElse(false);
+
+    if (!inheritsBaseEstimator && !inheritsMixin(classSymbol)) {
+      return;
+    }
+
+    var visitor = new VariableDeclarationEndingWithUnderscoreVisitor();
+    functionDef.body().accept(visitor);
+    var offendingVariables = visitor.qualifiedExpressions;
+    var secondaryLocation = classDef.name();
+    offendingVariables
+      .forEach((qualifiedExpression, assignmentStatement) -> {
+        var issue = subscriptionContext.addIssue(qualifiedExpression.name(), MESSAGE).secondary(secondaryLocation, MESSAGE_SECONDARY);
+
+        createQuickFix(assignmentStatement).ifPresent(issue::addQuickFix);
+        issue.addQuickFix(createQuickFixRename(qualifiedExpression));
+      });
+  }
+
+  private static PythonQuickFix createQuickFixRename(QualifiedExpression qualifiedExpression) {
+    var quickFix = PythonQuickFix.newQuickFix(QUICK_FIX_RENAME_MESSAGE);
+    var newName = qualifiedExpression.name().name().replaceAll("_+$", "");
+    return quickFix.addTextEdit(TextEditUtils.renameAllUsages(qualifiedExpression.name(), newName)).build();
+  }
+
+  private static Optional<PythonQuickFix> createQuickFix(AssignmentStatement assignmentStatement) {
+
+    var builder = PythonQuickFix.newQuickFix(QUICK_FIX_MESSAGE);
+
+    if (assignmentStatement.lhsExpressions().size() != 1 || assignmentStatement.lhsExpressions().stream().anyMatch(expressions -> expressions.expressions().size() != 1)) {
+      return Optional.empty();
+    }
+    builder.addTextEdit(TextEditUtils.removeStatement(assignmentStatement));
+    if (assignmentStatement.assignedValue().is(Tree.Kind.NONE)) {
+      return Optional.of(builder.build());
+    }
+    return Optional.empty();
+  }
+
+  private static class VariableDeclarationEndingWithUnderscoreVisitor extends BaseTreeVisitor {
+
+    private final Map<QualifiedExpression, AssignmentStatement> qualifiedExpressions = new HashMap<>();
+
+    private static boolean isOffendingQualifiedExpression(QualifiedExpression qualifiedExpression) {
+      return !qualifiedExpression.name().name().startsWith("__") && qualifiedExpression.name().name().endsWith("_") && qualifiedExpression.qualifier().is(Tree.Kind.NAME)
+        && "self".equals(((org.sonar.plugins.python.api.tree.Name) qualifiedExpression.qualifier()).name());
+    }
+
+    @Override
+    public void visitAssignmentStatement(AssignmentStatement pyAssignmentStatementTree) {
+      var offendingQualifiedExpressions = pyAssignmentStatementTree.lhsExpressions()
+        .stream()
+        .flatMap(expressionList -> expressionList.expressions().stream())
+        .filter(expression -> expression.is(Tree.Kind.QUALIFIED_EXPR))
+        .map(QualifiedExpression.class::cast);
+
+      var offendingTuples = pyAssignmentStatementTree.lhsExpressions()
+        .stream()
+        .flatMap(expressionList -> expressionList.expressions().stream())
+        .filter(expression -> expression.is(Tree.Kind.TUPLE))
+        .map(Tuple.class::cast)
+        .flatMap(tuple -> tuple.elements().stream())
+        .filter(expression -> expression.is(Tree.Kind.QUALIFIED_EXPR))
+        .map(QualifiedExpression.class::cast);
+
+      Stream.concat(
+        offendingQualifiedExpressions, offendingTuples)
+        .filter(VariableDeclarationEndingWithUnderscoreVisitor::isOffendingQualifiedExpression)
+        .forEach(qualifiedExpression -> qualifiedExpressions.put(qualifiedExpression, pyAssignmentStatementTree));
+      super.visitAssignmentStatement(pyAssignmentStatementTree);
+    }
+  }
+}

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

@@ -0,0 +1,46 @@
+<p>This rule raises an issue when an attributes ending with <code>_</code> is set in the <code>__init__</code> method of a class inheriting from
+Scikit-Learn <code>BaseEstimator</code></p>
+<h2>Why is this an issue?</h2>
+<p>On a Scikit-Learn estimator, attributes that have a trailing underscore represents attributes that are estimated. These attributes have to be set
+in the fit method. Their presence are used to verify if an estimator has been fitted.</p>
+<pre>
+from sklearn.neighbors import KNeighborsClassifier
+
+X = [[0], [1], [2], [3]]
+y = [0, 0, 1, 1]
+knn = KNeighborsClassifier(n_neighbors=1)
+knn.fit(X, y)
+knn.n_samples_fit_
+</pre>
+<p>In the example above the attributes of the <code>KNeighborsClassifier</code>, <code>n_samples_fit_</code> is set only after the estimator’s
+<code>fit</code> method is called. Calling <code>n_samples_fit_</code> before the estimator is fitted would raise an <code>AttributeError</code>
+exception.</p>
+<p>When implementing a custom estimator by subclassing Scikit-Learn’s <code>BaseEstimator</code>, it is important to follow the above convention and
+not set attributes with a trailing underscore inside the <code>__init__</code> method.</p>
+<h2>How to fix it</h2>
+<p>To fix this issue, move the attributes with a trailing underscore from the <code>__init__</code> method to the <code>fit</code> method.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+from sklearn.base import BaseEstimator
+
+class MyEstimator(BaseEstimator):
+    def __init__(self):
+        self.estimated_attribute_ = None # Noncompliant: an estimated attribute is set in the __init__ method.
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+from sklearn.base import BaseEstimator
+
+class MyEstimator(BaseEstimator):
+    def fit(self, X, y):
+        self.estimated_attribute_ = some_estimation(X) # Compliant
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Scikit-Learn documentation - <a href="https://scikit-learn.org/stable/developers/develop.html#parameters-and-init">Parameters and init</a>
+  </li>
+  <li> {rule:python:S6970} - The Scikit-learn <code>fit</code> method should be called before methods yielding results </li>
+</ul>
+

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6974.json

@@ -0,0 +1,22 @@
+{
+  "title": "Subclasses of Scikit-Learn\u0027s \"BaseEstimator\" should not set attributes ending with \"_\" in the \"__init__\" method",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6974",
+  "sqKey": "S6974",
+  "scope": "All",
+  "quickfix": "partial",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "MEDIUM",
+      "RELIABILITY": "HIGH"
+    },
+    "attribute": "CONVENTIONAL"
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

@@ -240,6 +240,7 @@
     "S6928",
     "S6929",
     "S6969",
-    "S6971"
+    "S6971",
+    "S6974"
   ]
 }

python-checks/src/test/java/org/sonar/python/checks/SkLearnEstimatorDontInitializeEstimatedValuesCheckTest.java

@@ -0,0 +1,97 @@
+/*
+ * 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 org.junit.jupiter.api.Test;
+import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+class SkLearnEstimatorDontInitializeEstimatedValuesCheckTest {
+  @Test
+  void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/sklearn_estimator_underscore_initialization.py", new SkLearnEstimatorDontInitializeEstimatedValuesCheck());
+  }
+
+  @Test
+  void testQuickfix1() {
+    PythonQuickFixVerifier.verify(
+      new SkLearnEstimatorDontInitializeEstimatedValuesCheck(),
+      """
+        from sklearn.base import BaseEstimator
+        class InheritingEstimator(BaseEstimator):
+            def __init__(self) -> None:
+                self.a_ = None
+                ...""",
+      """
+        from sklearn.base import BaseEstimator
+        class InheritingEstimator(BaseEstimator):
+            def __init__(self) -> None:
+                ...""",
+      """
+  from sklearn.base import BaseEstimator
+  class InheritingEstimator(BaseEstimator):
+      def __init__(self) -> None:
+          self.a = None
+          ..."""
+          );
+  }
+  @Test
+  void testQuickfix2() {
+    PythonQuickFixVerifier.verify(
+      new SkLearnEstimatorDontInitializeEstimatedValuesCheck(),
+      """
+        from sklearn.base import BaseEstimator
+        class InheritingEstimator(BaseEstimator):
+            def __init__(self) -> None:
+                self._something_a_______ = None""",
+      """
+        from sklearn.base import BaseEstimator
+        class InheritingEstimator(BaseEstimator):
+            def __init__(self) -> None:
+                pass""",
+      """
+  from sklearn.base import BaseEstimator
+  class InheritingEstimator(BaseEstimator):
+      def __init__(self) -> None:
+          self._something_a = None"""
+    );
+  }
+  @Test
+  void testQuickfixEmptyFunc() {
+    PythonQuickFixVerifier.verify(
+      new SkLearnEstimatorDontInitializeEstimatedValuesCheck(),
+      """
+        from sklearn.base import ClassifierMixin
+        class InheritingEstimator(ClassifierMixin):
+            def __init__(self) -> None:
+                self.a_ = None""",
+      """
+        from sklearn.base import ClassifierMixin
+        class InheritingEstimator(ClassifierMixin):
+            def __init__(self) -> None:
+                pass""",
+      """
+  from sklearn.base import ClassifierMixin
+  class InheritingEstimator(ClassifierMixin):
+      def __init__(self) -> None:
+          self.a = None"""
+    );
+  }
+}