SONARPY-4138 Avoid raising S8514 when the class variable is likely intentional (#1096)

GitOrigin-RevId: 25997a4ff0b42970deaafdad60f7f69a1f1a4f2f

Author: guillaume-dequenne

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

@@ -16,6 +16,7 @@
  */
 package org.sonar.python.checks;
 
+import java.util.List;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
@@ -24,6 +25,8 @@
 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.Name;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
 import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
@@ -63,8 +66,37 @@ private static boolean isDataclassDecorator(Decorator decorator, SubscriptionCon
   }
 
   private static void checkStatement(SubscriptionContext ctx, Tree statement) {
-    if (statement instanceof AssignmentStatement assignmentStatement) {
+    if (statement instanceof AssignmentStatement assignmentStatement && !isLikelyIntentionalClassVar(assignmentStatement)) {
       ctx.addIssue(assignmentStatement, MESSAGE_MISSING_ANNOTATION);
     }
   }
+
+  private static boolean isLikelyIntentionalClassVar(AssignmentStatement assignment) {
+    List<ExpressionList> lhsList = assignment.lhsExpressions();
+    if (lhsList.size() != 1) {
+      return false;
+    }
+    List<Expression> expressions = lhsList.get(0).expressions();
+    if (expressions.size() != 1 || !expressions.get(0).is(Tree.Kind.NAME)) {
+      return false;
+    }
+    String name = ((Name) expressions.get(0)).name();
+    return name.startsWith("_") || isAllCaps(name);
+  }
+
+  // ALL_CAPS names follow the Python constant convention — they are intentional class-level attributes.
+  private static boolean isAllCaps(String name) {
+    if (name.isEmpty()) {
+      return false;
+    }
+    boolean hasUpperCase = false;
+    for (char c : name.toCharArray()) {
+      if (Character.isUpperCase(c)) {
+        hasUpperCase = true;
+      } else if (!Character.isDigit(c) && c != '_') {
+        return false;
+      }
+    }
+    return hasUpperCase;
+  }
 }

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

@@ -1,4 +1,5 @@
-<p>This rule raises an issue when a dataclass contains attributes without type annotations.</p>
+<p>This rule raises an issue when a dataclass attribute is assigned a value without a type annotation, suggesting the developer likely intended an
+instance field but silently created a class variable instead.</p>
 <h2>Why is this an issue?</h2>
 <p>The <code>@dataclass</code> decorator only processes attributes that have type annotations. Attributes without annotations are silently excluded
 from dataclass processing and become regular class variables instead of instance attributes. This means:</p>
@@ -12,8 +13,10 @@ <h3>What is the potential impact?</h3>
 dataclass contract. These bugs are difficult to trace because the class appears to work normally, but the affected attributes do not participate in
 any of the generated methods.</p>
 <h2>How to fix it</h2>
-<p>Add type annotations to all attributes that should be dataclass fields. If you intend an attribute to be a class variable shared across instances,
-move it outside the dataclass or document this intention clearly.</p>
+<p>Add type annotations to all attributes that should be dataclass fields.</p>
+<p>If you intentionally want a class variable shared across all instances, annotate it with <code>typing.ClassVar[T]</code>. This is the idiomatic
+Python way to declare class variables inside a dataclass — it makes the intent explicit and ensures the attribute is excluded from all generated
+dataclass methods.</p>
 <h3>Code examples</h3>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="1" data-diff-type="noncompliant">
@@ -33,10 +36,28 @@ <h4>Compliant solution</h4>
     timeout: int = 30
     retries: int = 3
 </pre>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="2" data-diff-type="noncompliant">
+from dataclasses import dataclass
+
+@dataclass
+class Registry:
+    instance_count = 0  # Noncompliant: intended as a class variable, but not explicit
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="2" data-diff-type="compliant">
+from dataclasses import dataclass
+from typing import ClassVar
+
+@dataclass
+class Registry:
+    instance_count: ClassVar[int] = 0  # Explicit class variable, excluded from __init__
+</pre>
 <h2>Resources</h2>
 <h3>Documentation</h3>
 <ul>
   <li>Python Documentation - <a href="https://docs.python.org/3/library/dataclasses.html">dataclasses — Data Classes</a></li>
   <li>PEP 557 - <a href="https://peps.python.org/pep-0557/">Data Classes</a></li>
+  <li>Python Documentation - <a href="https://docs.python.org/3/library/typing.html#typing.ClassVar">typing.ClassVar</a></li>
 </ul>
 

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

@@ -193,3 +193,21 @@ class ParentDataclass:
 
 class NonDataclassChild(ParentDataclass):
     unannotated = 99
+
+
+# ALL_CAPS names follow the Python constant convention and are intentional class-level attributes
+@dataclass
+class AllCapsConstants:
+    MAX_RETRIES = 5
+    DEFAULT_TIMEOUT = 30
+    BASE_URL = "https://example.com"
+
+
+# Names starting with _ are considered intentional private/special attributes
+@dataclass
+class UnderscorePrefixedAttributes:
+    _private = "value"
+    __slots__ = ['x', 'y']
+    __tablename__ = "users"
+    __abstract__ = True
+    _PRIVATE_CONST = 42