Fix false negative for used-before-assignment when some except handlers don't define a name
#5764
Conversation
…lers don't define a name
for more information, see https://pre-commit.ci
Pull Request Test Coverage Report for Build 1815244914
💛 - Coveralls |
Pierre-Sassoulas
added
False Negative 🦋
There was a problem hiding this comment.
Small changes, but this looks great!
@jacobtylerwalls just to get this right, what do you propose in terms of issue management. What should be opened/closed after this has been merged?
tests/functional/u/use/used_before_assignment_except_handler_for_try_with_return.py
Show resolved
Hide resolved
|
@areveny @DanielNoord thanks for the reviews and bugspotting. I'm glad we're getting a real v1.0 of control flow for try/except into something that should just work!
Let's pretend I never called the two issues duplicates! Specifically:
And then I can adjust this PR to say "closes" instead of "partially closes" #5524. Sorry for the extra work, but now I see from discussion we're likely to address the bigger-picture issue with a new message, so better to keep it all separate. |
|
I have opened and made the comments as you suggested. I'm going to ask for a review from Pierre as this is quite a large PR again. Just FYI: I think this should be the last new PR to go into |
There was a problem hiding this comment.
LGTM, the tests are really nice, and this will be a nice addition to 2.13. Also thank you for providing early feedback @areveny, :)
Small question, I noticed that there could be more tests if we do the permutation for "defined in one handler only" with unpacked iterable, or chained assignment. I suppose it's because it's not necessary to check this again as it's already checked in funcok_4 and func_ok5, and not doing all permutation would speed the functional tests up ?
| and other_node_statement.parent.parent.parent.parent_of( | ||
| node_statement | ||
| ) | ||
| and closest_try_except.parent.parent_of(node_statement) |
There was a problem hiding this comment.
Good job generalizing here, this is a lot cleaner.
|
I went ahead and added the tests. After all, just a few comments above I was very keen to tell the world that true positives and true negatives might hit different code paths! 😄 |
Type of Changes
Description
Fixes #5524.
In discussion I suggested closing #2835 as a duplicate, but if folks like we could "unlink" the two and reopen the older ticket.The older ticket is just suggesting "always assume try statements fail", and @cdce8p suggested a new message for that (Edit: done!possibly-unbound). Probably a 2.14 or later thing. By contrast this PR is just fixing a quick false negative. Sorry for not seeing that we were more likely to tackle these separately!