SONARPY-2690 Fix FP on S1172 to avoid raising issues on conventional method signatures (#442)

Co-authored-by: Sebastian Zumbrunn <sebastian.zumbrunn@sonarsource.com>
GitOrigin-RevId: 718636a1ffbca0445dd48283ed01e08abf22b71e

Author: 2 people

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

@@ -34,6 +34,7 @@
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.ExpressionStatement;
 import org.sonar.plugins.python.api.tree.FunctionDef;
+import org.sonar.plugins.python.api.tree.Name;
 import org.sonar.plugins.python.api.tree.Parameter;
 import org.sonar.plugins.python.api.tree.ReturnStatement;
 import org.sonar.plugins.python.api.tree.Statement;
@@ -68,6 +69,7 @@ private static void checkFunctionParameter(SubscriptionContext ctx, FunctionDef
       .filter(UnusedFunctionParameterCheck::isUnused)
       .filter(symbol -> !isUsedInStringLiteralOrComment(symbol.name(), functionDef))
       .map(symbol -> (Parameter) symbol.usages().get(0).tree().parent())
+      .filter(param -> !isSpecialArgument(param))
       .forEach(param -> ctx.addIssue(param, String.format(MESSAGE, param.name().name())));
   }
 
@@ -88,6 +90,20 @@ private static boolean isUsedInStringLiteralOrComment(String symbolName, Functio
       comments.stream().anyMatch(str -> p.matcher(str).find());
   }
 
+  private static boolean isSpecialArgument(Parameter parameter) {
+    String starTokenValue = Optional.ofNullable(parameter.starToken())
+      .map(Token::value)
+      .orElse("");
+    String paramName = Optional.ofNullable(parameter.name())
+      .map(Name::name)
+      .orElse("");
+
+    boolean isArgsParam = "*".equals(starTokenValue) && "args".equals(paramName);
+    boolean isKwArgsParam = "**".equals(starTokenValue) && "kwargs".equals(paramName);
+
+    return isArgsParam || isKwArgsParam;
+  }
+
   private static boolean isIgnoredSymbolName(String symbolName) {
     return "self".equals(symbolName) || symbolName.startsWith("_") || AWS_LAMBDA_PARAMETERS.contains(symbolName);
   }
@@ -102,6 +118,7 @@ private static boolean isException(SubscriptionContext ctx, FunctionDef function
       isSpecialMethod(functionDef) ||
       hasNonCallUsages(functionSymbol) ||
       isTestFunction(ctx, functionDef) ||
+      isDjangoView(functionDef) ||
       isAbstractClass(functionDef);
   }
 
@@ -178,4 +195,10 @@ private static List<String> collectComments(Tree element) {
     }
     return comments;
   }
+
+  private static boolean isDjangoView(FunctionDef functionDef) {
+    FunctionSymbol functionSymbol = ((FunctionDefImpl) functionDef).functionSymbol();
+    FunctionSymbolImpl functionSymbolImpl = (FunctionSymbolImpl) functionSymbol;
+    return functionSymbolImpl != null && functionSymbolImpl.isDjangoView();
+  }
 }

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

@@ -16,6 +16,7 @@
  */
 package org.sonar.python.checks;
 
+import java.util.Arrays;
 import java.util.List;
 import org.junit.jupiter.api.Test;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
@@ -48,4 +49,12 @@ void test_test_file() {
     PythonCheckVerifier.verifyNoIssue("src/test/resources/checks/unusedFunctionParameter/test_something.py", new UnusedFunctionParameterCheck());
   }
 
+  @Test
+  void test_django() {
+    PythonCheckVerifier.verify(Arrays.asList(
+        "src/test/resources/checks/unusedFunctionParameter/django/urls.py",
+        "src/test/resources/checks/unusedFunctionParameter/django/views.py"
+      ),
+      new UnusedFunctionParameterCheck());
+  }
 }

python-checks/src/test/resources/checks/unusedFunctionParameter/django/__init__.py

@@ -0,0 +1,9 @@
+from django.urls import path
+
+from . import views
+
+urlpatterns = [
+  path('view_func1', views.view_func1, name='view_func1'),
+  path('view_func2', views.view_func2, name='view_func2'),
+  path('view_func3', views.view_func3, name='view_func3'),
+]

python-checks/src/test/resources/checks/unusedFunctionParameter/django/urls.py

@@ -0,0 +1,15 @@
+from django.views.decorators.http import require_http_methods, require_POST, require_GET, require_safe, other_decorator
+
+def not_a_view(request): # Noncompliant
+    print("smth")
+
+def view_func1(request):  # Compliant
+  print("smth")
+
+@require_http_methods(["GET", "POST"])  # Compliant
+def view_func2(request):
+    print("smth")
+
+def view_func3(request, unused_parameter):  # Compliant FN, unused_parameter is not used in the function body and is not mandatory
+    print("smth")
+

python-checks/src/test/resources/checks/unusedFunctionParameter/django/views.py

@@ -18,6 +18,24 @@ def g2(a):  # Noncompliant
     c = 2
     compute(b)
 
+def function_with_args_and_no_star(a, args): # Noncompliant
+    print(a)
+
+def function_with_args_and_star(a, *args): # Compliant, *args is often unused and acts as a catch-all
+    print(a)
+
+def function_with_args_and_two_star(a, **args): # Noncompliant
+    print(a)
+
+def function_with_kwargs_and_no_star(a, kwargs): # Noncompliant
+    print(a)
+
+def function_with_kwargs_and_star(a, *kwargs): # Noncompliant
+    print(a)
+
+def function_with_kwargs_and_two_star(a, **kwargs): # Compliant, **kwargs is often unused and acts as a catch-all
+    print(a)
+
 class MyInterface:
 
     def write_alignment(self, a):

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

@@ -159,7 +159,7 @@ private void createAmbiguousSymbols() {
           if (bindingUsages.size() > 1 &&
             bindingUsages.stream().anyMatch(usage -> usage.kind() == Usage.Kind.FUNC_DECLARATION || usage.kind() == Usage.Kind.CLASS_DECLARATION)) {
 
-            Set<Symbol> alternativeDefinitions = getAlternativeDefinitions(symbol, bindingUsages);
+            Set<Symbol> alternativeDefinitions = createAlternativeDefinitions(symbol, bindingUsages);
             AmbiguousSymbol ambiguousSymbol = AmbiguousSymbolImpl.create(alternativeDefinitions);
             // update symbol and usage to newly created ambiguous symbol
             symbol.usages().forEach(usage -> ((SymbolImpl) ambiguousSymbol).addUsage(usage.tree(), usage.kind()));
@@ -171,7 +171,7 @@ private void createAmbiguousSymbols() {
     }
   }
 
-  private Set<Symbol> getAlternativeDefinitions(Symbol symbol, List<Usage> bindingUsages) {
+  private Set<Symbol> createAlternativeDefinitions(Symbol symbol, List<Usage> bindingUsages) {
     Set<Symbol> alternativeDefinitions = new HashSet<>();
     for (Usage bindingUsage : bindingUsages) {
       switch (bindingUsage.kind()) {