Python: Do not report unreachable "catch-all" cases in elif-chains.#2221
Merged
RasmusWL merged 1 commit intogithub:masterfrom Oct 29, 2019
Merged
Conversation
RasmusWL
reviewed
Oct 29, 2019
Member
RasmusWL
left a comment
RasmusWL
left a comment
There was a problem hiding this comment.
LGTM.
I'm not too big a fan of the name catch_all_exception though. It sounds too much like catching all exceptions (as in the code below). Could we name it just catch_all?
try:
...
except Exception:
...
Contributor
Author
|
Perhaps something like |
Member
|
😄 |
Contributor
Author
|
Very well, |
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.
taus-semmle
force-pushed
the
python-unreachable-catch-all-assert
branch
from
dd0be70 to
5e62da7
Compare
RasmusWL
approved these changes
Oct 29, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
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, ...orraise ..., and only if said statement is the first in theelseblock. Any other statements will still be reported.(Also applied a bit of autoformatting, which led to a few changes to existing code. It seems this hadn't been done to the entire file previously.)