CLP-1218 Fix S8514 false positives on generated dataclass methods

GitOrigin-RevId: 640918cfee5ef475e76cd7b76bd6b541a2ea0856

Author: guillaume-dequenne

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

@@ -20,16 +20,23 @@
 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.cfg.ControlFlowGraph;
+import org.sonar.plugins.python.api.symbols.v2.UsageV2;
 import org.sonar.plugins.python.api.tree.AssignmentStatement;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ClassDef;
 import org.sonar.plugins.python.api.tree.Decorator;
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.ExpressionList;
+import org.sonar.plugins.python.api.tree.FunctionDef;
+import org.sonar.plugins.python.api.tree.LambdaExpression;
 import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.ReturnStatement;
 import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.types.v2.FunctionType;
 import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
 import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
+import org.sonar.python.checks.utils.Expressions;
 
 @Rule(key = "S8514")
 public class DataclassFieldDefinitionCheck extends PythonSubscriptionCheck {
@@ -66,11 +73,66 @@ private static boolean isDataclassDecorator(Decorator decorator, SubscriptionCon
   }
 
   private static void checkStatement(SubscriptionContext ctx, Tree statement) {
-    if (statement instanceof AssignmentStatement assignmentStatement && !isLikelyIntentionalClassVar(assignmentStatement)) {
+    if (statement instanceof AssignmentStatement assignmentStatement && !isLikelyIntentionalClassVar(assignmentStatement)
+      && !isMethodAssignment(assignmentStatement.assignedValue(), ctx)) {
       ctx.addIssue(assignmentStatement, MESSAGE_MISSING_ANNOTATION);
     }
   }
 
+  private static boolean isMethodAssignment(Expression expression, SubscriptionContext ctx) {
+    Expression value = Expressions.removeParentheses(expression);
+    if (isFunction(value)) {
+      return true;
+    }
+    // Source function return types are not always inferred. Recognize local factories
+    // returning a function without exempting ordinary data-producing factory calls.
+    if (value instanceof CallExpression call && call.callee() instanceof Name name && name.symbolV2() != null) {
+      List<UsageV2> bindings = name.symbolV2().usages().stream().filter(UsageV2::isBindingUsage).toList();
+      return bindings.size() == 1
+        && bindings.get(0).kind() == UsageV2.Kind.FUNC_DECLARATION
+        && bindings.get(0).tree().parent() instanceof FunctionDef function
+        && isMethodFactory(function, ctx);
+    }
+    return false;
+  }
+
+  private static boolean isMethodFactory(FunctionDef function, SubscriptionContext ctx) {
+    if (function.asyncKeyword() != null || !function.decorators().isEmpty() || hasDataReturnOrYield(function.body())) {
+      return false;
+    }
+    // Checking explicit returns alone would miss paths that implicitly return None.
+    ControlFlowGraph cfg = ctx.cfg(function);
+    return cfg != null && !cfg.end().predecessors().isEmpty()
+      && cfg.end().predecessors().stream().allMatch(block -> {
+        List<Tree> elements = block.elements();
+        return !elements.isEmpty() && returnsFunction(elements.get(elements.size() - 1));
+      });
+  }
+
+  private static boolean returnsFunction(Tree tree) {
+    return tree instanceof ReturnStatement statement && statement.expressions().size() == 1
+      && isFunction(statement.expressions().get(0));
+  }
+
+  private static boolean hasDataReturnOrYield(Tree tree) {
+    if (tree.is(Tree.Kind.YIELD_EXPR, Tree.Kind.YIELD_STMT) || (tree instanceof ReturnStatement && !returnsFunction(tree))) {
+      return true;
+    }
+    Tree nestedBody = switch (tree) {
+      case FunctionDef nested -> nested.body();
+      case ClassDef nested -> nested.body();
+      case LambdaExpression lambda -> lambda.expression();
+      default -> null;
+    };
+    // Traverse arguments, return expressions and defaults, but not nested bodies.
+    return tree.children().stream().filter(child -> child != nestedBody).anyMatch(DataclassFieldDefinitionCheck::hasDataReturnOrYield);
+  }
+
+  private static boolean isFunction(Expression expression) {
+    Expression value = Expressions.removeParentheses(expression);
+    return value.is(Tree.Kind.LAMBDA) || value.typeV2() instanceof FunctionType;
+  }
+
   private static boolean isLikelyIntentionalClassVar(AssignmentStatement assignment) {
     List<ExpressionList> lhsList = assignment.lhsExpressions();
     if (lhsList.size() != 1) {

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

@@ -211,3 +211,190 @@ class UnderscorePrefixedAttributes:
     __tablename__ = "users"
     __abstract__ = True
     _PRIVATE_CONST = 42
+
+
+def make_method(operation):
+    def method(self, *args):
+        return operation(*args)
+    return method
+
+
+def method(self):
+    return 42
+
+
+@dataclass
+class GeneratedMethods:
+    direct = method
+    generated = make_method(sum)
+    anonymous = lambda self: 42
+    count = 42  # Noncompliant
+    items = list()  # Noncompliant
+
+
+def nested_generated_methods():
+    @dataclass
+    class Operations:
+        bitwise_and = make_method(sum)
+        bitwise_or = make_method(sum)
+        count = 42  # Noncompliant
+
+
+def make_data():
+    def method(self):
+        return 42
+    return 42
+
+
+def make_pair():
+    return method, 42
+
+
+async def make_coroutine():
+    return method
+
+
+def make_generator():
+    yield 42
+    return method
+
+
+class CallableData:
+    def __call__(self):
+        return 42
+
+
+@dataclass
+class FactoryDataStillRequiresAnnotations:
+    data = make_data()  # Noncompliant
+    pair = make_pair()  # Noncompliant
+    coroutine = make_coroutine()  # Noncompliant
+    generator = make_generator()  # Noncompliant
+    callable_data = CallableData()  # Noncompliant
+    field_default = field(default=42)  # Noncompliant
+
+
+def make_lambda():
+    return lambda self: 42
+
+
+@dataclass
+class LambdaFactory:
+    generated = make_lambda()
+    parenthesized = (lambda self: 42)
+
+
+def make_mixed_value(as_method=False):
+    if as_method:
+        return lambda self: 42
+    return 42
+
+
+def make_rebound_value():
+    return lambda self: 42
+
+
+make_rebound_value = int
+
+
+def make_generator_in_call():
+    print((yield 42))
+    return lambda self: 42
+
+
+@dataclass
+class ReviewRegressionControls:
+    mixed = make_mixed_value()  # Noncompliant
+    rebound = make_rebound_value()  # Noncompliant
+    generator = make_generator_in_call()  # Noncompliant
+
+
+def make_optional_method(as_method=False):
+    if as_method:
+        return method
+
+
+def make_method_or_none(as_method=False):
+    if as_method:
+        return method
+    return
+
+
+def make_no_value():
+    pass
+
+
+def make_redefined_value():
+    return method
+
+
+def make_redefined_value():
+    return 0
+
+
+def make_generator_with_yield_from():
+    print((yield from [42]))
+    return method
+
+
+def make_generator_in_default():
+    def nested(value=(yield 42)):
+        return value
+    return nested
+
+
+def make_generator_in_lambda_default():
+    return lambda self, value=(yield 42): value
+
+
+def replace_factory(factory):
+    return int
+
+
+@replace_factory
+def make_decorated_value():
+    return method
+
+
+@dataclass
+class AmbiguousFactoryResults:
+    implicit_none = make_optional_method()  # Noncompliant
+    bare_return = make_method_or_none()  # Noncompliant
+    no_return = make_no_value()  # Noncompliant
+    redefined = make_redefined_value()  # Noncompliant
+    delegated_generator = make_generator_with_yield_from()  # Noncompliant
+    generator_default = make_generator_in_default()  # Noncompliant
+    generator_lambda_default = make_generator_in_lambda_default()  # Noncompliant
+    decorated = make_decorated_value()  # Noncompliant
+
+
+def make_alternative_methods(first=True):
+    if first:
+        return method
+    else:
+        return lambda self: 42
+
+
+def make_generator_method():
+    def generated(self):
+        yield 42
+    return generated
+
+
+def make_lambda_generator_method():
+    return lambda self: (yield 42)
+
+
+def make_method_with_nested_class():
+    class Nested:
+        def generated(self):
+            yield 42
+    return method
+
+
+@dataclass
+class MoreGeneratedMethods:
+    alternatives = make_alternative_methods()
+    generator_method = make_generator_method()
+    lambda_generator_method = make_lambda_generator_method()
+    nested_class = make_method_with_nested_class()