Don't assume direct parentage when emitting used-before-assignment
#5582
Conversation
Pierre-Sassoulas
added
the
False Negative 🦋
Pull Request Test Coverage Report for Build 1678621581
💛 - Coveralls |
|
Need to add a test for try:
pass
except:
if:
name=
else:
name=
print(name)EDIT: done! ✔️ |
… when emitting `used-before-assignment`
jacobtylerwalls
force-pushed
the
try-ancestor
branch
from
9f20e2a to
d16730b
Compare
jacobtylerwalls
changed the title
used-before-assignmentused-before-assignment
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
(no typing exists for result of statement()`
jacobtylerwalls
force-pushed
the
try-ancestor
branch
from
85c6149 to
4b409a2
Compare
for more information, see https://pre-commit.ci
| def consecutive_except_blocks(): | ||
| """An assignment assumed to execute in one TryExcept should continue to be | ||
| assumed to execute in a consecutive TryExcept. | ||
| """ | ||
| try: | ||
| res = 100 | ||
| except ZeroDivisionError: | ||
| pass | ||
| try: | ||
| pass | ||
| except ValueError: | ||
| print(res) |
There was a problem hiding this comment.
I don't think I understand everything here.
If the first except is raised and the function do not return, it means in the second one res won't be defined ?
| def consecutive_except_blocks(): | |
| """An assignment assumed to execute in one TryExcept should continue to be | |
| assumed to execute in a consecutive TryExcept. | |
| """ | |
| try: | |
| res = 100 | |
| except ZeroDivisionError: | |
| pass | |
| try: | |
| pass | |
| except ValueError: | |
| print(res) | |
| def consecutive_except_blocks(): | |
| try: | |
| res = 100 | |
| except ZeroDivisionError: | |
| pass | |
| try: | |
| pass | |
| except ValueError: | |
| print(res) # [used-before-assignment] | |
| def consecutive_except_blocks_with_return(): | |
| try: | |
| res = 100 | |
| except ZeroDivisionError: | |
| return | |
| try: | |
| pass | |
| except ValueError: | |
| print(res) |
There was a problem hiding this comment.
Same here, we want to avoid annoying users, and so outside a TryExcept we assume the try statements executed. By that same logic we assume that outside a TryExcept its except statements did not execute -- because it would be a bit backwards to assume the try statements never execute and thus that assignments in the except always happen.
There was a problem hiding this comment.
Didn't we say that like Pylance we consider that everything in a try/except can fail ? (#5384). Also, if we do not raise or return in the except it means we can reach line 43 from one branch or another and we would need to consider both branches in order to be correct.
There was a problem hiding this comment.
No, that wasn't my understanding, and that's not what Pylance does. The yellow underline is Pylance, showing that the checker we just worked on is in agreement with Pylance:
There was a problem hiding this comment.
I guess a good way of putting it is that's why there are different private methods in this checker now:
_uncertain_nodes_in_except_blocks
_uncertain_nodes_in_try_blocks_when_evaluating_except_blocks
_uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks
We make different control flow inferences based on where the name is being used (try, except, finally, or outside).
There was a problem hiding this comment.
I agree with the points you're making here. Especially the last example is something of which I can easily imagine complaints coming from. Code with requests and databases seem to often really on such patterns.
Perhaps you could add a summary of this explanation to the issue you mentioned? That way we don't lose this.
There was a problem hiding this comment.
Especially the last example is something of which I can easily imagine complaints coming from.
My last example was not quite right 😬 -- I neglected that subsequent handlers can't swallow exceptions raised in earlier handlers, so ValueError can't be passed down to a subsequent handler. I'll edit it.
There was a problem hiding this comment.
Perhaps you could add a summary of this explanation to the issue you mentioned? That way we don't lose this.
I think this discussion is getting at when an else clause should be used in a try/except/else. If you can depend on exhaustive handling by your except handlers, you don't need to guard your print(a) after the try/except. If it's not exhaustively handled, then your print(a) should be in an else.
I guess this could even be its own checker, so it could be enabled/disabled separately. (It would probably be easier to implement that way, given how much new logic would go into the exhaustiveness checking.)
There was a problem hiding this comment.
I think relying on everyone knowing that library_function_that_might_sysexit will implicitly exit all the time is an anti-pattern. Personally I would initialize the variable to be sure, or at least to add a comment stating explicitly that there's an exit and it's ok to do that if there's a good reason to do that like performance. (a pylint disable is not that much more work then),
But I get that this might be a little opinionated and that we'd get complaints from this, thank you for explaining.
Since we're talking about addressing false negatives, not false positives, that seems like a smart way of iterating to me.
👍
There was a problem hiding this comment.
This discussion helped clarify for me that beefing up the checker to assume try statements always fail is worthy of discussion, but would need to be solved in tandem with #5524, so I suggest we keep discussing there for 2.14 or later.
There was a problem hiding this comment.
Just did a quick review of things I noticed immediately. I'll do a full review once @Pierre-Sassoulas is finished as not to duplicate work/comments.
There was a problem hiding this comment.
I think this discussion must be resolved before we review the change in details.
|
Moving this to |
I guess that's not quite what I gathered from your comment at #5582 (comment). I thought the suggestion was to wait for 2.14 before potentially beefing up the checker to assume try statements always fail -- something that I think after more discussion we intend to track and discuss in #5524. On the contrary, this PR and its partner #5583 merely fix false negatives that one would currently assume would work given how the release notes for 2.13 already describe the partial assumptions we are making. So it's not a blocker -- we can ship with false negatives, but it's not either that we are consciously deciding to wait to ship fixes for false negatives regarding assumptions that have already been merged to main in other PRs. |
|
Not a problem @DanielNoord, if I could go back in time I would have done this in the first set of control flow PRs, but live and learn! |
We've come a long way since your initial PR and my refactor of that document. I think it's still a bit of a mess but it is so much better than previously and also catches a lot more! All thanks to you 😄 |
There was a problem hiding this comment.
Great work @jacobtylerwalls, thank you for the discussion too, doing false negative later makes a lot of sense.
Type of Changes
Description
Before
In Pylint 2.13.0 (unreleased) we are starting to be more confident in emitting
used-before-assignmentby making control flow inferences. (See e.g. #5384) We started cautiously, just checking direct parents of statements in try blocks to find out if that was the sameTryExceptancestor that was the parent of the node being visited in the except block. This left out possibly failing statements in the try block indented underif/with, etc.Now
@DanielNoord suggested using
get_node_first_ancestor_of_type()to be able to leapfrog overifandwithstatements to arrive at the relevant try ancestor. With this PR, we can now emitused-before-assignmenteven when the possibly failing statements in a try block are inside if/with statements.One refactor commit and one behavior change commit.
Closes #4045