Skip to content

Commit e4658ec

Browse files
sonar-nigel[bot]sonartech
authored andcommitted
SONARPY-2299 Fix S3801 false positive for match statements with exhaustive coverage (#1020)
GitOrigin-RevId: 88a3b5e788bbe41167ebd49be451ee2a7f3c7f37
1 parent 05cdab9 commit e4658ec

File tree

7 files changed

+396
-16
lines changed

7 files changed

+396
-16
lines changed

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package org.sonar.python.checks;
1818

1919
import java.util.List;
20-
import java.util.stream.Collectors;
2120
import org.sonar.check.Rule;
2221
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2322
import org.sonar.plugins.python.api.SubscriptionContext;
@@ -44,12 +43,12 @@ public void initialize(Context context) {
4443
if (cfg == null || hasExceptOrFinally(cfg)) {
4544
return;
4645
}
47-
List<Tree> endStatements = cfg.end().predecessors().stream()
46+
List<Statement> endStatements = cfg.end().predecessors().stream()
4847
.map(block -> parentStatement(block.elements().get(block.elements().size() - 1)))
4948
.filter(s -> !s.is(Kind.RAISE_STMT, Kind.ASSERT_STMT, Kind.WITH_STMT) && !isWhileTrue(s))
50-
.collect(Collectors.toList());
49+
.toList();
5150

52-
List<Tree> returnsWithValue = endStatements.stream()
51+
List<Statement> returnsWithValue = endStatements.stream()
5352
.filter(s -> s.is(Kind.RETURN_STMT) && hasValue((ReturnStatement) s))
5453
.toList();
5554

@@ -68,9 +67,9 @@ private static boolean isWhileTrue(Statement statement) {
6867
return statement.is(Kind.WHILE_STMT) && Expressions.isTruthy(((WhileStatement) statement).condition());
6968
}
7069

71-
private static void addIssue(SubscriptionContext ctx, FunctionDef functionDef, List<Tree> endStatements) {
70+
private static void addIssue(SubscriptionContext ctx, FunctionDef functionDef, List<Statement> endStatements) {
7271
PreciseIssue issue = ctx.addIssue(functionDef.name(), MESSAGE);
73-
for (Tree statement : endStatements) {
72+
for (Statement statement : endStatements) {
7473
if (statement.is(Kind.RETURN_STMT)) {
7574
ReturnStatement returnStatement = (ReturnStatement) statement;
7675
boolean hasValue = hasValue(returnStatement);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,4 +223,4 @@ def match_statement_fn(value):
223223
match value:
224224
case "1": return
225225
case x: return
226-
print("unreachable") # FN, "case x" will match anything
226+
print("unreachable") # Noncompliant

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

Lines changed: 232 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,13 +106,52 @@ def implicit_return_on_match_statement(x): # Noncompliant
106106
return x
107107
# ^^^^^^^^ < {{Return with value}}
108108

109-
def match_statement_with_default(x): # Noncompliant
109+
def match_statement_with_default(x):
110110
match(x):
111111
case 0:
112112
return x
113113
case _:
114114
return 42
115115

116+
def match_wildcard_return_or_raise(x):
117+
match x:
118+
case 1:
119+
return 2
120+
case _:
121+
raise ValueError("error")
122+
123+
def match_capture_pattern_all_return(x):
124+
match x:
125+
case 1:
126+
return 2
127+
case 2:
128+
return 3
129+
case other:
130+
return other + 10
131+
132+
def match_wildcard_with_guard(x): # Noncompliant
133+
match x:
134+
case 1:
135+
return 2
136+
case _ if x > 0:
137+
return 3
138+
139+
def match_irrefutable_not_last(x):
140+
match x:
141+
case _:
142+
return 1
143+
case 1:
144+
return 2
145+
146+
def match_irrefutable_in_middle(x):
147+
match x:
148+
case 0:
149+
return 1
150+
case _:
151+
return 2
152+
case 1:
153+
pass
154+
116155
# raise / assert
117156

118157
def return_value_or_raise(x):
@@ -182,3 +221,195 @@ def fp_on_undetected_raise(x): # Noncompliant
182221

183222
def raise_error():
184223
raise "error"
224+
225+
226+
# Patterns from ruling analysis
227+
228+
def find_match_in_loop(texts, frag): # Noncompliant
229+
# ^^^^^^^^^^^^^^^^^^
230+
for t in texts:
231+
# ^^^ < {{Implicit return without value if the condition is false}}
232+
if t is not frag and t.left > frag.right:
233+
return t
234+
# ^^^^^^^^ < {{Return with value}}
235+
236+
def multiple_value_returns_with_bare_return(aug_tok1, params): # Noncompliant
237+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
238+
if not aug_tok1:
239+
return
240+
# ^^^^^^ < {{Return without value}}
241+
if aug_tok1.period_final:
242+
return "REASON_KNOWN_COLLOCATION"
243+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
244+
if aug_tok1.abbr:
245+
return "REASON_ABBR"
246+
# ^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
247+
return
248+
# ^^^^^^ < {{Return without value}}
249+
250+
def return_only_in_else_branch(run_mode): # Noncompliant
251+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^
252+
if run_mode == 'v1':
253+
do_v1_test()
254+
# ^^^^^^^^^^^^ < {{Implicit return without value}}
255+
elif run_mode == 'v2':
256+
do_v2_test()
257+
# ^^^^^^^^^^^^ < {{Implicit return without value}}
258+
else:
259+
return ValueError('Unknown run mode %s' % run_mode)
260+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
261+
262+
263+
class MyClass:
264+
def value_or_none(self): # Noncompliant
265+
# ^^^^^^^^^^^^^
266+
if self.condition:
267+
# ^^ < {{Implicit return without value if the condition is false}}
268+
return self._value
269+
# ^^^^^^^^^^^^^^^^^^ < {{Return with value}}
270+
271+
def always_returns_none_or_value(self):
272+
if self.condition:
273+
return self._value
274+
return None
275+
276+
@property
277+
def linked_object(self): # Noncompliant
278+
# ^^^^^^^^^^^^^
279+
if self.link_type == "a":
280+
return self.a
281+
# ^^^^^^^^^^^^^ < {{Return with value}}
282+
elif self.link_type == "b":
283+
return self.b
284+
# ^^^^^^^^^^^^^ < {{Return with value}}
285+
elif self.link_type == "c":
286+
return self.c
287+
# ^^^^^^^^^^^^^ < {{Return with value}}
288+
elif self.link_type == "d":
289+
# ^^^^ < {{Implicit return without value if the condition is false}}
290+
return self.d
291+
# ^^^^^^^^^^^^^ < {{Return with value}}
292+
293+
294+
def outer_with_nested(params):
295+
def handler(cursor): # Noncompliant
296+
# ^^^^^^^
297+
if cursor.bindvars is None:
298+
return
299+
# ^^^^^^ < {{Return without value}}
300+
if isinstance(cursor.bindvars, list):
301+
return [v.getvalue() for v in cursor.bindvars]
302+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
303+
if isinstance(cursor.bindvars, dict):
304+
return {n: v.getvalue() for n, v in cursor.bindvars.items()}
305+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
306+
raise TypeError("Unexpected bindvars")
307+
308+
result = run(handler)
309+
return result
310+
311+
312+
def boundaries(start, end): # Noncompliant
313+
# ^^^^^^^^^^
314+
if not isinstance(start, int):
315+
raise TypeError("expected int")
316+
if not isinstance(end, int):
317+
raise TypeError("expected int")
318+
if start > end:
319+
start, end = end, start
320+
if start < end:
321+
# ^^ < {{Implicit return without value if the condition is false}}
322+
return start, end
323+
# ^^^^^^^^^^^^^^^^^ < {{Return with value}}
324+
325+
326+
class MiddlewareMixin:
327+
@staticmethod
328+
def process_request(request): # Noncompliant
329+
# ^^^^^^^^^^^^^^^
330+
if not request.user.is_authenticated:
331+
# ^^ < {{Implicit return without value if the condition is false}}
332+
if request.path not in IGNORE_URL:
333+
# ^^ < {{Implicit return without value if the condition is false}}
334+
return redirect('/login/')
335+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
336+
337+
338+
class WebhookPlugin:
339+
def order_fully_paid(self, order, previous_value): # Noncompliant
340+
# ^^^^^^^^^^^^^^^^
341+
if not self.active:
342+
return previous_value
343+
# ^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
344+
event_type = "ORDER_FULLY_PAID"
345+
if webhooks := get_webhooks_for_event(event_type):
346+
# ^^ < {{Implicit return without value if the condition is false}}
347+
trigger_webhooks_async(webhooks)
348+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Implicit return without value}}
349+
350+
351+
class CacheFactory:
352+
@classmethod
353+
def factory(cls, backend, ttl): # Noncompliant
354+
# ^^^^^^^
355+
if backend == "memory":
356+
return CacheDict(ttl)
357+
# ^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
358+
elif backend == "disk":
359+
return CacheDisk(ttl)
360+
# ^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
361+
else:
362+
log.error("unrecognized cache type")
363+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Implicit return without value}}
364+
365+
366+
@lru_cache()
367+
def cached_lookup(locale): # Noncompliant
368+
# ^^^^^^^^^^^^^
369+
name = get_name(locale)
370+
if name is not None:
371+
# ^^ < {{Implicit return without value if the condition is false}}
372+
return load_resource(name)
373+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
374+
375+
376+
@some_hook(flag=True)
377+
def announce_header(**kwargs): # Noncompliant
378+
# ^^^^^^^^^^^^^^^
379+
if not session.user or config.DISABLE_CHECK:
380+
return
381+
# ^^^^^^ < {{Return without value}}
382+
if down:
383+
text = "scheduler not running"
384+
elif mismatch:
385+
text = "version mismatch"
386+
else:
387+
return
388+
# ^^^^^^ < {{Return without value}}
389+
return 'error', text, True
390+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^ < {{Return with value}}
391+
392+
393+
def early_sentinel_value_return(path, conn=None): # Noncompliant
394+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^
395+
if conn is None:
396+
conn = get_conn()
397+
if conn is False:
398+
return False
399+
# ^^^^^^^^^^^^ < {{Return with value}}
400+
for comp in path.split("/"):
401+
# ^^^ < {{Implicit return without value if the condition is false}}
402+
process(comp)
403+
404+
405+
class SearchHelper:
406+
def dup_check(self, sequence): # Noncompliant
407+
# ^^^^^^^^^
408+
for sc in self.all_shortcuts:
409+
# ^^^ < {{Implicit return without value if the condition is false}}
410+
if sc is self.shortcut:
411+
continue
412+
for k in sc['keys']:
413+
if k == sequence:
414+
return sc['name']
415+
# ^^^^^^^^^^^^^^^^^ < {{Return with value}}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ def match_statement_no_fp(value):
353353
return no_fp_with_match(value + 1)
354354

355355

356-
def match_statement_fn(value): # FN, "case x" will always match
356+
def match_statement_fn(value): # Noncompliant
357357
match value:
358358
case x:
359359
return match_statement_fn(value + 1)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ def match_statement_no_fp_with_implicit_return(value): # OK, implicit "None" re
529529
return 42
530530

531531

532-
def match_statement_fn_with_capture(value): # FN, implicit "None" return is unreachable
532+
def match_statement_fn_with_capture(value): # Noncompliant
533533
match value:
534534
case 1:
535535
return 42

python-frontend/src/main/java/org/sonar/python/cfg/ControlFlowGraphBuilder.java

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -204,26 +204,56 @@ private static PythonCfgBlock buildFuncDefStatement(FunctionDef functionDef, Pyt
204204

205205
private PythonCfgBlock buildMatchStatement(MatchStatement statement, PythonCfgBlock successor) {
206206
List<CaseBlock> caseBlocks = statement.caseBlocks();
207-
PythonCfgBlock matchingBlock = null;
208-
PythonCfgBlock falseSuccessor = successor;
207+
int irrefutableIdx = findIrrefutableCaseIndex(caseBlocks);
208+
209+
PythonCfgBlock irrefutableBlock = null;
210+
if (irrefutableIdx >= 0) {
211+
CaseBlock irrefutableCase = caseBlocks.get(irrefutableIdx);
212+
PythonCfgBlock irrefutableBodyBlock = build(irrefutableCase.body().statements(), createSimpleBlock(successor));
213+
irrefutableBlock = createSimpleBlock(irrefutableBodyBlock);
214+
irrefutableBlock.addElement(irrefutableCase.pattern());
215+
irrefutableBlock.addElement(statement.subjectExpression());
216+
}
217+
218+
PythonCfgBlock matchingBlock = irrefutableBlock;
219+
PythonCfgBlock falseSuccessor = irrefutableBlock != null ? irrefutableBlock : successor;
209220
for (int i = caseBlocks.size() - 1; i >= 0; i--) {
210-
PythonCfgBlock caseBodyBlock = createSimpleBlock(successor);
221+
if (irrefutableIdx >= 0 && i >= irrefutableIdx) {
222+
continue;
223+
}
224+
if (i == irrefutableIdx - 1) {
225+
falseSuccessor = irrefutableBlock;
226+
}
211227
CaseBlock caseBlock = caseBlocks.get(i);
212228
Pattern pattern = caseBlock.pattern();
213229
Guard guard = caseBlock.guard();
214-
caseBodyBlock = build(caseBlock.body().statements(), caseBodyBlock);
230+
PythonCfgBlock caseBodyBlock = build(caseBlock.body().statements(), createSimpleBlock(successor));
215231
matchingBlock = createBranchingBlock(pattern, caseBodyBlock, falseSuccessor);
216232
if (guard != null) {
217233
matchingBlock.addElement(guard.condition());
218234
}
219235
matchingBlock.addElement(pattern);
220236
matchingBlock.addElement(statement.subjectExpression());
221-
blocks.add(matchingBlock);
222-
falseSuccessor = matchingBlock;
237+
if (i < irrefutableIdx || irrefutableIdx < 0) {
238+
falseSuccessor = matchingBlock;
239+
}
223240
}
224241
return matchingBlock;
225242
}
226243

244+
private static int findIrrefutableCaseIndex(List<CaseBlock> caseBlocks) {
245+
for (int i = 0; i < caseBlocks.size(); i++) {
246+
if (isIrrefutablePattern(caseBlocks.get(i))) {
247+
return i;
248+
}
249+
}
250+
return -1;
251+
}
252+
253+
private static boolean isIrrefutablePattern(CaseBlock caseBlock) {
254+
return caseBlock.guard() == null && caseBlock.pattern().is(Tree.Kind.WILDCARD_PATTERN, Tree.Kind.CAPTURE_PATTERN);
255+
}
256+
227257
private PythonCfgBlock buildWithStatement(WithStatement withStatement, PythonCfgBlock successor) {
228258
PythonCfgBlock withBodyBlock = build(withStatement.statements().statements(), createSimpleBlock(successor));
229259
// exceptions may be raised inside with block and be caught by context manager

0 commit comments

Comments
 (0)