Fix false negative for used-before-assignment when an Except intervenes between Try/Finally
#5583
Conversation
Pierre-Sassoulas
added
the
False Negative 🦋
Pull Request Test Coverage Report for Build 1700068503
💛 - Coveralls |
jacobtylerwalls
force-pushed
the
try-except-finally
branch
from
50815a9
to
8f2bd0b
Compare
|
try:
pass
finally:
try:
times = 1
except TypeError:
pass
print(times) # emits. shouldn't. and won't if the outer try goes away. |
|
@jacobtylerwalls I haven't look at this but in #5582 we discussed doing some of the changes for |
|
Hi @DanielNoord thanks for looking into this. I think this is the same category ("no reason to wait; fixes false negatives for assumptions we've already doc'd and merged to main") as #5582, see #5582 (comment). Happy to hear your views on this. |
There was a problem hiding this comment.
Looks good, but the rebase for this is non trivial, and there's a strange conflict in the changelog and whatsnew. Could you rebase yourself @jacobtylerwalls ?
…intervenes between try/finally
jacobtylerwalls
force-pushed
the
try-except-finally
branch
from
10e6a50
to
979a330
Compare
| def try_except_finally_nested_try_finally_in_finally(): | ||
| """Don't confuse assignments in different finally statements where | ||
| one is nested inside a finally. | ||
| """ | ||
| try: | ||
| pass | ||
| finally: | ||
| try: | ||
| times = 1 | ||
| except TypeError: | ||
| pass | ||
| # Don't emit: only assume the outer try failed | ||
| print(times) |
There was a problem hiding this comment.
I don't understand Don't confuse assignments in different finally statements. Shouldn't we raise or times be defined somewhere in the other finally statement?
There was a problem hiding this comment.
The docstring looks like it was lazily copied from above and not really updated. I'll push again and explain in the docstring how I see what is being tested and where to go from here.
There was a problem hiding this comment.
In rewriting the tests I discovered I could handle nesting better. So there are more tests now :-)
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
for more information, see https://pre-commit.ci
Type of Changes
Description
Fixes false negative for
used-before-assignmentwhen evaluating statements in a finally block, there is a possibly failing statement in the try block, and an except handler intervenes between.Found while working on #5582. Unhandled case from #5384 -- I had thought of this case, but didn't know how to solve it: a finally that is not the direct descendant of a try handler because we have try/except/finally. (Since we were just incrementally solving false negatives, that wasn't such a problem at the time.) After #5582, I now know how to handle it!