SONARPY-3847 Fix S8396 FP: do not raise on X | None = Field(...) patterns (#898)

Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
GitOrigin-RevId: 10a57f35a44eb8dab9eb8106fe1253f064583a64

Author: 2 people

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

@@ -22,7 +22,6 @@
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.tree.AnnotatedAssignment;
 import org.sonar.plugins.python.api.tree.Argument;
-import org.sonar.plugins.python.api.tree.BinaryExpression;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.ClassDef;
 import org.sonar.plugins.python.api.tree.Expression;
@@ -43,8 +42,6 @@ public class PydanticOptionalFieldDefaultCheck extends PythonSubscriptionCheck {
   private static final TypeMatcher IS_PYDANTIC_FIELD = TypeMatchers.isType("pydantic.Field");
 
   private static final TypeMatcher IS_TYPING_OPTIONAL = TypeMatchers.isType("typing.Optional");
-  private static final TypeMatcher IS_TYPING_UNION = TypeMatchers.isType("typing.Union");
-  private static final TypeMatcher IS_NONE_TYPE = TypeMatchers.isObjectOfType("NoneType");
 
   @Override
   public void initialize(Context context) {
@@ -67,16 +64,12 @@ private static void checkField(SubscriptionContext ctx, AnnotatedAssignment anno
     TypeAnnotation annotation = annotatedAssignment.annotation();
     Expression annotationExpr = annotation.expression();
 
+    if (!isTypingOptional(annotationExpr, ctx)) {
+      return;
+    }
+
     Expression assignedValue = annotatedAssignment.assignedValue();
-    boolean hasFieldWithEllipsis = assignedValue != null && isFieldCallWithEllipsis(ctx, assignedValue);
-
-    if (isTypingOptional(annotationExpr, ctx)) {
-      // Optional[X]: raise when no default, or when Field(...) with ellipsis
-      if (assignedValue == null || hasFieldWithEllipsis) {
-        ctx.addIssue(annotationExpr, MESSAGE);
-      }
-    } else if (isNullableNonOptional(annotationExpr, ctx) && hasFieldWithEllipsis) {
-      // X | None or Union[X, None]: only raise when Field(...) with ellipsis
+    if (assignedValue == null || isFieldCallWithEllipsis(ctx, assignedValue)) {
       ctx.addIssue(annotationExpr, MESSAGE);
     }
   }
@@ -88,36 +81,6 @@ private static boolean isTypingOptional(Expression annotationExpr, SubscriptionC
     return false;
   }
 
-  private static boolean isNullableNonOptional(Expression annotationExpr, SubscriptionContext ctx) {
-    // Case 1: T | None (BinaryExpression with BITWISE_OR)
-    if (annotationExpr.is(Tree.Kind.BITWISE_OR)) {
-      return containsNone((BinaryExpression) annotationExpr, ctx);
-    }
-
-    // Case 2: Union[T, None] (SubscriptionExpression)
-    if (annotationExpr instanceof SubscriptionExpression subscriptionExpr) {
-      return IS_TYPING_UNION.isTrueFor(subscriptionExpr.object(), ctx) && subscriptsContainNone(subscriptionExpr, ctx);
-    }
-
-    return false;
-  }
-
-  private static boolean containsNone(BinaryExpression binaryExpr, SubscriptionContext ctx) {
-    return isNoneExpression(binaryExpr.leftOperand(), ctx) ||
-      isNoneExpression(binaryExpr.rightOperand(), ctx) ||
-      (binaryExpr.leftOperand().is(Tree.Kind.BITWISE_OR) && containsNone((BinaryExpression) binaryExpr.leftOperand(), ctx)) ||
-      (binaryExpr.rightOperand().is(Tree.Kind.BITWISE_OR) && containsNone((BinaryExpression) binaryExpr.rightOperand(), ctx));
-  }
-
-  private static boolean isNoneExpression(Expression expr, SubscriptionContext ctx) {
-    return IS_NONE_TYPE.isTrueFor(expr, ctx);
-  }
-
-  private static boolean subscriptsContainNone(SubscriptionExpression subscriptionExpr, SubscriptionContext ctx) {
-    return subscriptionExpr.subscripts().expressions().stream()
-      .anyMatch(expr -> isNoneExpression(expr, ctx));
-  }
-
   private static boolean isFieldCallWithEllipsis(SubscriptionContext ctx, Expression assignedValue) {
     if (!(assignedValue instanceof CallExpression callExpr)) {
       return false;

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

@@ -21,20 +21,6 @@ class ModelWithMethods(BaseModel):
     def some_method(self):
         pass
 
-# Field(...) with ellipsis always raises, even for X | None and Union[X, None]
-
-class BitwiseOrWithFieldEllipsis(BaseModel):
-    value: str | None = Field(...)  # Noncompliant
-#          ^^^^^^^^^^
-
-class NoneLeftWithFieldEllipsis(BaseModel):
-    value: None | str = Field(...)  # Noncompliant
-#          ^^^^^^^^^^
-
-class UnionWithFieldEllipsis(BaseModel):
-    value: Union[str, None] = Field(...)  # Noncompliant
-#          ^^^^^^^^^^^^^^^^
-
 # =====================
 # COMPLIANT CASES
 # =====================
@@ -52,7 +38,7 @@ class SettingsModelCompliant(BaseModel):
 
 class RequiredModel(BaseModel):
     required_field: str
-    another_field: int = Field(...)  # not Optional, no issue
+    another_field: int = Field(...)
 
 class ConfigModel(BaseModel):
     timeout: Optional[int] = 30
@@ -61,22 +47,29 @@ class FactoryModel(BaseModel):
     items: Optional[list] = Field(default_factory=list)
 
 class RegularClass:
-    value: Optional[str]  # not a BaseModel
-
-# X | None and Union[X, None] without default: not Optional[X], so no issue
+    value: Optional[str]
 
 class BitwiseOrNoneCompliant(BaseModel):
     reason: int | None
 
+class BitwiseOrNoneWithFieldEllipsisCompliant(BaseModel):
+    bio: str | None = Field(...)
+
 class BitwiseOrNoneWithFieldDefaultCompliant(BaseModel):
     title: str | None = Field(default=None)
 
 class NoneLeftCompliant(BaseModel):
     description: None | str
 
+class NoneLeftWithFieldEllipsisCompliant(BaseModel):
+    value: None | str = Field(...)
+
 class UnionWithNoneCompliant(BaseModel):
     data: Union[str, None]
 
+class UnionWithFieldEllipsisCompliant(BaseModel):
+    value: Union[str, None] = Field(...)
+
 # =====================
 # EDGE CASES
 # =====================