SONARPY-267 Rule S1172: Unused function parameters should be removed (#1091)

* SONARPY-267 Rule S1172: Unused function parameters should be removed

* Fix bug and code smell and increase coverage

* Address review suggestions

Author: nils-werner-sonarsource

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

@@ -242,6 +242,7 @@ public static Iterable<Class> getChecks() {
       UnreadPrivateMethodsCheck.class,
       UnsafeHttpMethodsCheck.class,
       UndefinedSymbolsCheck.class,
+      UnusedFunctionParameterCheck.class,
       UnusedLocalVariableCheck.class,
       UnusedNestedDefinitionCheck.class,
       UnverifiedHostnameCheck.class,

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

@@ -20,12 +20,17 @@
 package org.sonar.python.checks;
 
 import java.util.List;
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.sonar.plugins.python.api.tree.ArgList;
 import org.sonar.plugins.python.api.tree.Argument;
+import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ClassDef;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.Name;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.api.PythonTokenType;
+import org.sonar.python.tree.TreeUtils;
 
 public class CheckUtils {
 
@@ -65,6 +70,7 @@ private static boolean areLeavesEquivalent(Tree leftLeaf, Tree rightLeaf) {
       leftLeaf.firstToken().value().equals(rightLeaf.firstToken().value());
   }
 
+  @CheckForNull
   public static ClassDef getParentClassDef(Tree tree) {
     Tree current = tree.parent();
     while (current != null) {
@@ -89,4 +95,13 @@ public static boolean classHasInheritance(ClassDef classDef) {
     }
     return arguments.size() != 1 || !"object".equals(arguments.get(0).firstToken().value());
   }
+
+  public static boolean containsCallToLocalsFunction(Tree tree) {
+    return TreeUtils.hasDescendant(tree, t -> t.is(Tree.Kind.CALL_EXPR) && calleeHasNameLocals(((CallExpression) t)));
+  }
+
+  private static boolean calleeHasNameLocals(CallExpression callExpression) {
+    Expression callee = callExpression.callee();
+    return callee.is(Tree.Kind.NAME) && "locals".equals(((Name) callee).name());
+  }
 }

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

@@ -0,0 +1,73 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 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.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.ClassDef;
+import org.sonar.plugins.python.api.tree.ExpressionStatement;
+import org.sonar.plugins.python.api.tree.FunctionDef;
+import org.sonar.plugins.python.api.tree.Parameter;
+import org.sonar.plugins.python.api.tree.Tree.Kind;
+import org.sonar.python.semantic.SymbolUtils;
+
+@Rule(key = "S1172")
+public class UnusedFunctionParameterCheck extends PythonSubscriptionCheck {
+
+  private static final String MESSAGE = "Remove the unused function parameter \"%s\".";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Kind.FUNCDEF, ctx -> checkFunctionParameter(ctx, ((FunctionDef) ctx.syntaxNode())));
+  }
+
+  private static void checkFunctionParameter(SubscriptionContext ctx, FunctionDef functionDef) {
+    if (CheckUtils.containsCallToLocalsFunction(functionDef) || maybeOverridingMethod(functionDef) || isInterfaceMethod(functionDef)) {
+      return;
+    }
+
+    functionDef.localVariables().stream()
+      .filter(symbol -> !"self".equals(symbol.name()))
+      .map(Symbol::usages)
+      .filter(usages -> usages.size() == 1 && usages.get(0).tree().parent().is(Kind.PARAMETER))
+      .map(usages -> (Parameter) usages.get(0).tree().parent())
+      .forEach(param -> ctx.addIssue(param, String.format(MESSAGE, param.name().name())));
+  }
+
+  private static boolean isInterfaceMethod(FunctionDef functionDef) {
+    return functionDef.body().statements().stream()
+      .allMatch(statement -> statement.is(Kind.PASS_STMT, Kind.RAISE_STMT)
+        || (statement.is(Kind.EXPRESSION_STMT) && isStringExpression((ExpressionStatement) statement)));
+  }
+
+  private static boolean isStringExpression(ExpressionStatement stmt) {
+    return stmt.expressions().stream().allMatch(expr -> expr.is(Kind.STRING_LITERAL));
+  }
+
+  private static boolean maybeOverridingMethod(FunctionDef functionDef) {
+    if (functionDef.isMethodDefinition() && !SymbolUtils.isPrivateName(functionDef.name().name())) {
+      ClassDef classDef = CheckUtils.getParentClassDef(functionDef);
+      return classDef != null && classDef.args() != null;
+    }
+    return false;
+  }
+}

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

@@ -24,19 +24,16 @@
 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.Symbol;
+import org.sonar.plugins.python.api.symbols.Usage;
 import org.sonar.plugins.python.api.tree.AnnotatedAssignment;
-import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ComprehensionExpression;
 import org.sonar.plugins.python.api.tree.DictCompExpression;
-import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.ExpressionList;
 import org.sonar.plugins.python.api.tree.ForStatement;
 import org.sonar.plugins.python.api.tree.FunctionDef;
-import org.sonar.plugins.python.api.tree.Name;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.tree.Tree.Kind;
-import org.sonar.plugins.python.api.symbols.Symbol;
-import org.sonar.plugins.python.api.symbols.Usage;
 import org.sonar.python.tree.TreeUtils;
 
 @Rule(key = "S1481")
@@ -55,7 +52,7 @@ public void initialize(Context context) {
 
   private static void checkLocalVars(SubscriptionContext ctx, Tree functionTree, Set<Symbol> symbols) {
     // https://docs.python.org/3/library/functions.html#locals
-    if (isCallingLocalsFunction(functionTree)) {
+    if (CheckUtils.containsCallToLocalsFunction(functionTree)) {
       return;
     }
     symbols.stream()
@@ -90,13 +87,4 @@ private static boolean isTupleDeclaration(Tree tree) {
         || (t.is(Kind.EXPRESSION_LIST) && ((ExpressionList) t).expressions().size() > 1)
         || (t.is(Kind.FOR_STMT) && ((ForStatement) t).expressions().size() > 1 && ((ForStatement) t).expressions().contains(tree))) != null;
   }
-
-  private static boolean isCallingLocalsFunction(Tree tree) {
-    return TreeUtils.hasDescendant(tree, t -> t.is(Kind.CALL_EXPR) && calleeHasNameLocals(((CallExpression) t)));
-  }
-
-  private static boolean calleeHasNameLocals(CallExpression callExpression) {
-    Expression callee = callExpression.callee();
-    return callee.is(Kind.NAME) && "locals".equals(((Name) callee).name());
-  }
 }

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

@@ -0,0 +1,23 @@
+<p>Unused parameters are misleading. Whatever the values passed to such parameters, the behavior will be the same.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+def do_something(a, b):  # `a` is unused
+  return compute(b)
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+def do_something(b):
+  return compute(b)
+</pre>
+<h2>Exceptions</h2>
+<p>Functions in classes that override a class or implement interfaces are ignored.</p>
+<pre>
+class Parent(object):
+  def get_value(self, a, b):
+    return compute(a + b);
+
+class Child(Patent):
+  def get_value(self, a, b):
+    return compute(b)
+</pre>
+

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

@@ -0,0 +1,17 @@
+{
+  "title": "Unused function parameters should be removed",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "unused"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-1172",
+  "sqKey": "S1172",
+  "scope": "All",
+  "quickfix": "unknown"
+}

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

@@ -23,6 +23,7 @@
     "S1135",
     "S1143",
     "S1144",
+    "S1172",
     "S1186",
     "S1192",
     "S1226",

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

@@ -0,0 +1,32 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 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.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+public class UnusedFunctionParameterCheckTest {
+
+  @Test
+  public void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/unusedFunctionParameter.py", new UnusedFunctionParameterCheck());
+  }
+
+}

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

@@ -0,0 +1,54 @@
+def f(a):  # Noncompliant {{Remove the unused function parameter "a".}}
+#     ^
+    print("foo")
+
+
+def g(a):
+    print(a)
+
+
+def f(a):
+    return locals()
+
+
+def g(a):  # Noncompliant
+    b = 1
+    c = 2
+    compute(b)
+
+class MyInterface:
+
+    def write_alignment(self, a):
+        """This method should be replaced by any derived class to do something
+        useful.
+        """
+        raise NotImplementedError("This object should be subclassed")
+
+class Parent:
+    def do_something(self, a, b):  # Noncompliant {{Remove the unused function parameter "a".}}
+        #                  ^
+        return compute(b)
+
+    def do_something_else(self, a, b):
+        return compute(a + b)
+
+
+class Child(Parent):
+    def do_something_else(self, a, b):
+        return compute(a)
+
+
+class AnotherChild(UnknownParent):
+    def _private_method(self, a, b):  # Noncompliant
+        return compute(b)
+
+    def is_overriding_a_parent_method(self, a, b):
+        return compute(b)
+
+class ClassWithoutArgs:
+    def do_something(self, a, b):  # Noncompliant
+        return compute(b)
+
+class ClassWithoutArgs2():
+    def do_something(self, a, b):  # Noncompliant
+        return compute(b)