Fix #5500: Fix false-positive used-before-assignment for assignments in except blocks following try blocks that return

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Fixes false positive for used-before-assignment when evaluating statements after except handlers where the corresponding try returned. In this scenario, we should assume one of the except handlers did execute.

There is the potential for a false-negative where only ONE of the except handlers defines the variable in question, but to check that, we would need to permit scenarios where the other except handlers merely raise or return--that case shouldn't emit a warning. I think the feasibility of that should be evaluated separately.

Closes #5500

[coveralls]

Pull Request Test Coverage Report for Build 1573137423

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.654%

---

Totals
Change from base Build 1573037236:	0.0%
Covered Lines:	14183
Relevant Lines:	15144

---

💛 - Coveralls

[Pierre-Sassoulas]

LGTM, I think it's a clever fix to bring better control flow.

[DanielNoord]

Fix itself seems fine, but this should be documented in the description of the used-before-assignment message. I'm not sure how to do this properly, especially since the original issue is also discussing another exception to the general "assume tries don't execute" rule. I think whatever we end up assuming should be clearly documented for end-users without the need of looking at our actual code or previous changelogs.

[jacobtylerwalls]

Sorry for the extra force push, I pushed a comment to the wrong branch and removed it when I discovered it to avoid cluttering the branch history.

[cdce8p]

The change looks good and indeed fixes the false-positives.

One thing I noticed though is that the filtered_nodes list comprehension has gotten rather complicated with multiple nested ifs and comprehensions itself. This should probably be a function instead.

That would also allow n.statement() to be cached witch would improve performance at least a tiny bit.

Could you create a followup PR for it?

[jacobtylerwalls]

Could you create a followup PR for it?

Sure thing--I agree, the comprehension is quite ugly now.