Skip to content

Commit 5156192

Browse files
sonar-nigel[bot]Vibe Botclaude
authored and
sonartech
committed
SONARPY-3834 Fix S8396 FP: do not raise on X | None without default value (#891)
Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> GitOrigin-RevId: 2bcd9301c3bbe254a2e20176a68f05c356b83c16
1 parent 465bbee commit 5156192

2 files changed

Lines changed: 73 additions & 84 deletions

File tree

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

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -67,30 +67,36 @@ private static void checkField(SubscriptionContext ctx, AnnotatedAssignment anno
6767
TypeAnnotation annotation = annotatedAssignment.annotation();
6868
Expression annotationExpr = annotation.expression();
6969

70-
if (!isOptionalType(annotationExpr, ctx)) {
71-
return;
72-
}
73-
7470
Expression assignedValue = annotatedAssignment.assignedValue();
75-
76-
boolean hasNoDefault = assignedValue == null;
7771
boolean hasFieldWithEllipsis = assignedValue != null && isFieldCallWithEllipsis(ctx, assignedValue);
7872

79-
if (hasNoDefault || hasFieldWithEllipsis) {
73+
if (isTypingOptional(annotationExpr, ctx)) {
74+
// Optional[X]: raise when no default, or when Field(...) with ellipsis
75+
if (assignedValue == null || hasFieldWithEllipsis) {
76+
ctx.addIssue(annotationExpr, MESSAGE);
77+
}
78+
} else if (isNullableNonOptional(annotationExpr, ctx) && hasFieldWithEllipsis) {
79+
// X | None or Union[X, None]: only raise when Field(...) with ellipsis
8080
ctx.addIssue(annotationExpr, MESSAGE);
8181
}
8282
}
8383

84-
private static boolean isOptionalType(Expression annotationExpr, SubscriptionContext ctx) {
84+
private static boolean isTypingOptional(Expression annotationExpr, SubscriptionContext ctx) {
85+
if (annotationExpr instanceof SubscriptionExpression subscriptionExpr) {
86+
return IS_TYPING_OPTIONAL.isTrueFor(subscriptionExpr.object(), ctx);
87+
}
88+
return false;
89+
}
90+
91+
private static boolean isNullableNonOptional(Expression annotationExpr, SubscriptionContext ctx) {
8592
// Case 1: T | None (BinaryExpression with BITWISE_OR)
8693
if (annotationExpr.is(Tree.Kind.BITWISE_OR)) {
87-
BinaryExpression binaryExpr = (BinaryExpression) annotationExpr;
88-
return containsNone(binaryExpr, ctx);
94+
return containsNone((BinaryExpression) annotationExpr, ctx);
8995
}
9096

91-
// Case 2: Optional[T] or Union[T, None] (SubscriptionExpression)
97+
// Case 2: Union[T, None] (SubscriptionExpression)
9298
if (annotationExpr instanceof SubscriptionExpression subscriptionExpr) {
93-
return isOptionalOrUnionWithNone(subscriptionExpr, ctx);
99+
return IS_TYPING_UNION.isTrueFor(subscriptionExpr.object(), ctx) && subscriptsContainNone(subscriptionExpr, ctx);
94100
}
95101

96102
return false;
@@ -107,20 +113,6 @@ private static boolean isNoneExpression(Expression expr, SubscriptionContext ctx
107113
return IS_NONE_TYPE.isTrueFor(expr, ctx);
108114
}
109115

110-
private static boolean isOptionalOrUnionWithNone(SubscriptionExpression subscriptionExpr, SubscriptionContext ctx) {
111-
Expression subscriptedObj = subscriptionExpr.object();
112-
113-
if (IS_TYPING_OPTIONAL.isTrueFor(subscriptedObj, ctx)) {
114-
return true;
115-
}
116-
117-
if (IS_TYPING_UNION.isTrueFor(subscriptedObj, ctx)) {
118-
return subscriptsContainNone(subscriptionExpr, ctx);
119-
}
120-
121-
return false;
122-
}
123-
124116
private static boolean subscriptsContainNone(SubscriptionExpression subscriptionExpr, SubscriptionContext ctx) {
125117
return subscriptionExpr.subscripts().expressions().stream()
126118
.anyMatch(expr -> isNoneExpression(expr, ctx));

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

Lines changed: 55 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -10,115 +10,112 @@ class UserModel(BaseModel):
1010
email: Optional[str] # Noncompliant {{Add an explicit default value to this optional field.}}
1111
# ^^^^^^^^^^^^^
1212

13-
class ProfileModel(BaseModel):
14-
bio: str | None # Noncompliant
15-
# ^^^^^^^^^^
16-
1713
class SettingsModel(BaseModel):
1814
theme: Optional[str] = Field(...) # Noncompliant
1915
# ^^^^^^^^^^^^^
2016

21-
class ArticleModel(BaseModel):
22-
title: str
23-
subtitle: Optional[str] # Noncompliant
24-
tags: list[str] | None # Noncompliant
17+
class ModelWithMethods(BaseModel):
18+
optional_field: Optional[str] # Noncompliant
19+
required_field: str
20+
21+
def some_method(self):
22+
pass
23+
24+
# Field(...) with ellipsis always raises, even for X | None and Union[X, None]
2525

26-
class DataModel(BaseModel):
27-
value: Union[str, None] # Noncompliant
26+
class BitwiseOrWithFieldEllipsis(BaseModel):
27+
value: str | None = Field(...) # Noncompliant
28+
# ^^^^^^^^^^
2829

29-
class ComplexModel(BaseModel):
30-
data: Union[str, int, None] # Noncompliant
30+
class NoneLeftWithFieldEllipsis(BaseModel):
31+
value: None | str = Field(...) # Noncompliant
32+
# ^^^^^^^^^^
33+
34+
class UnionWithFieldEllipsis(BaseModel):
35+
value: Union[str, None] = Field(...) # Noncompliant
36+
# ^^^^^^^^^^^^^^^^
3137

3238
# =====================
3339
# COMPLIANT CASES
3440
# =====================
3541

3642
class UserModelCompliant(BaseModel):
3743
name: str
38-
email: Optional[str] = None # Compliant
44+
email: Optional[str] = None
3945

4046
class ProfileModelCompliant(BaseModel):
41-
bio: str | None = None # Compliant
47+
bio: str | None = None
4248

4349
class SettingsModelCompliant(BaseModel):
44-
theme: Optional[str] = Field(default=None) # Compliant
45-
priority: Optional[int] = Field(default=0) # Compliant
50+
theme: Optional[str] = Field(default=None)
51+
priority: Optional[int] = Field(default=0)
4652

4753
class RequiredModel(BaseModel):
48-
required_field: str # Compliant - not Optional
49-
another_field: int = Field(...) # Compliant - not Optional
54+
required_field: str
55+
another_field: int = Field(...) # not Optional, no issue
5056

5157
class ConfigModel(BaseModel):
52-
timeout: Optional[int] = 30 # Compliant - has default
58+
timeout: Optional[int] = 30
5359

5460
class FactoryModel(BaseModel):
55-
items: Optional[list] = Field(default_factory=list) # Compliant
61+
items: Optional[list] = Field(default_factory=list)
5662

5763
class RegularClass:
58-
value: Optional[str] # Compliant - not a BaseModel
64+
value: Optional[str] # not a BaseModel
65+
66+
# X | None and Union[X, None] without default: not Optional[X], so no issue
67+
68+
class BitwiseOrNoneCompliant(BaseModel):
69+
reason: int | None
70+
71+
class BitwiseOrNoneWithFieldDefaultCompliant(BaseModel):
72+
title: str | None = Field(default=None)
73+
74+
class NoneLeftCompliant(BaseModel):
75+
description: None | str
76+
77+
class UnionWithNoneCompliant(BaseModel):
78+
data: Union[str, None]
5979

6080
# =====================
6181
# EDGE CASES
6282
# =====================
6383

6484
class ComplexModelCompliant(BaseModel):
65-
data: Union[str, int, None] = None # Compliant
85+
data: Union[str, int, None] = None
6686

6787
class EmptyModel(BaseModel):
68-
pass # Compliant - no fields
88+
pass
6989

7090
class OnlyRequiredModel(BaseModel):
7191
id: int
72-
name: str
73-
74-
# =====================
75-
# ADDITIONAL EDGE CASES FOR COVERAGE
76-
# =====================
77-
78-
class NoneLeftModel(BaseModel):
79-
value: None | str # Noncompliant
80-
81-
class NestedUnionLeftModel(BaseModel):
82-
value: None | str | int # Noncompliant
92+
name: str
8393

8494
class EmptyFieldModel(BaseModel):
85-
value: Optional[str] = Field() # Compliant - Field() with no ellipsis
95+
value: Optional[str] = Field()
8696

8797
class FieldWithValueModel(BaseModel):
88-
value: Optional[str] = Field(42) # Compliant - first arg is not ellipsis
98+
value: Optional[str] = Field(42)
8999

90100
class FieldWithKeywordFirstModel(BaseModel):
91-
value: Optional[str] = Field(default=None) # Compliant - default is specified
101+
value: Optional[str] = Field(default=None)
92102

93103
class FieldEllipsisWithDefaultModel(BaseModel):
94-
value: Optional[str] = Field(..., default=None) # Compliant
104+
value: Optional[str] = Field(..., default=None)
95105

96106
class FieldEllipsisWithFactoryModel(BaseModel):
97-
value: Optional[list] = Field(..., default_factory=list) # Compliant
98-
99-
class ModelWithMethods(BaseModel):
100-
optional_field: Optional[str] # Noncompliant
101-
required_field: str
102-
103-
def some_method(self):
104-
pass
105-
106-
@classmethod
107-
def class_method(cls):
108-
pass
107+
value: Optional[list] = Field(..., default_factory=list)
109108

110-
class MultiNestedModel(BaseModel):
111-
left_none: None | str # Noncompliant
112-
right_none: str | None # Noncompliant
113-
deep_left: None | int | str # Noncompliant
114-
deep_right1: int | str | None # Noncompliant
115-
deep_right2: int | str | str | str # Compliant
109+
class MultiNoneFieldCompliant(BaseModel):
110+
deep_left: None | int | str
111+
deep_right: int | str | None
112+
no_none: int | str | str
116113

117114
def custom_field():
118115
return None
119116

120117
class CustomFieldModel(BaseModel):
121-
value: Optional[str] = custom_field() # Compliant - not pydantic.Field
118+
value: Optional[str] = custom_field()
122119

123120
class OtherSubscriptionModel(BaseModel):
124-
value: list[str] # Compliant
121+
value: list[str]

0 commit comments

Comments
 (0)