Skip to content

Commit 5e62da7

Browse files
committed
Python: Do not report unreachable "catch-all" cases in elif-chains.
This was brought up on the LGTM.com forums here: https://discuss.lgtm.com/t/warn-when-always-failing-assert-is-reachable-rather-than-unreachable/2436 Essentially, in a complex chain of `elif` statements, like ```python if x < 0: ... elif x >= 0: ... else: ... ``` the `else` clause is redundant, since the preceding conditions completely exhaust the possible values for `x` (assuming `x` is an integer). Rather than promoting the final `elif` clause to an `else` clause, it is common to instead raise an explicit exception in the `else` clause. During execution, this exception will never actually be raised, but its presence indicates that the preceding conditions are intended to cover all possible cases. I think it's a fair point. This is a clear instance where the alert, even if it is technically correct, is not useful for the end user. Also, I decided to make the exclusion fairly restrictive: it only applies if the unreachable statement is an `assert False, ...` or `raise ...`, and only if said statement is the first in the `else` block. Any other statements will still be reported.
1 parent 6e6dab9 commit 5e62da7

2 files changed

Lines changed: 30 additions & 7 deletions

File tree

python/ql/src/Statements/UnreachableCode.ql

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,7 @@ import python
1616
predicate typing_import(ImportingStmt is) {
1717
exists(Module m |
1818
is.getScope() = m and
19-
exists(TypeHintComment tc |
20-
tc.getLocation().getFile() = m.getFile()
21-
)
19+
exists(TypeHintComment tc | tc.getLocation().getFile() = m.getFile())
2220
)
2321
}
2422

@@ -39,18 +37,26 @@ predicate suppression_in_scope(Stmt s) {
3937
)
4038
}
4139

40+
/** Holds if `s` is a statement that raises an exception at the end of an if-elif-else chain. */
41+
predicate marks_an_impossible_else_branch(Stmt s) {
42+
exists(If i | i.getOrelse().getItem(0) = s |
43+
s.(Assert).getTest() instanceof False
44+
or
45+
s instanceof Raise
46+
)
47+
}
48+
4249
predicate reportable_unreachable(Stmt s) {
4350
s.isUnreachable() and
4451
not typing_import(s) and
4552
not suppression_in_scope(s) and
4653
not exists(Stmt other | other.isUnreachable() |
4754
other.contains(s)
4855
or
49-
exists(StmtList l, int i, int j |
50-
l.getItem(i) = other and l.getItem(j) = s and i < j
51-
)
56+
exists(StmtList l, int i, int j | l.getItem(i) = other and l.getItem(j) = s and i < j)
5257
) and
53-
not unique_yield(s)
58+
not unique_yield(s) and
59+
not marks_an_impossible_else_branch(s)
5460
}
5561

5662
from Stmt s

python/ql/test/query-tests/Statements/unreachable/test.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,3 +120,20 @@ def foo(x):
120120
print(x, "== 0")
121121

122122

123+
# Unreachable catch-all case
124+
125+
def unreachable_catch_all_assert_false(x):
126+
if x < 0:
127+
return "negative"
128+
elif x >= 0:
129+
return "positive"
130+
else:
131+
assert False, x
132+
133+
def unreachable_catch_all_raise(x):
134+
if x < 0:
135+
pass
136+
elif x >= 0:
137+
pass
138+
else:
139+
raise ValueError(x)

0 commit comments

Comments
 (0)