Fix #85, #2615: Emit used-before-assignment in final or except blocks where try statements could have failed

[jacobtylerwalls]

Type of Changes

	Type
✓	✨ New feature

Description

Previously
Pylint assumed assignments in the Try block of a Try/Finally or Try/Except pair would succeed. In cases of failure, though, attempting to use such names in a final or except block would raise UnboundLocalError.

Now
Pylint emits used-before-assignment to account for the fact that the statement might have been interrupted by an exception.

Closes #85, Closes #2615

[DanielNoord]

I actually had an interesting discussion about this with one of the maintainers of pylance:
microsoft/pylance-release#1958

Basically it comes down to whether we want to assume all try statement fail or all succeed.

Personally I'm inclined to say let's assume it is succeeds. As a user I will be more annoyed by having to add disable's everywhere because one of my tools is producing false positives compared to running into exceptions because my tools are too "dumb". I prefer dumbness over potentially incorrect strictness, but this might be personal preference.

[coveralls]

Pull Request Test Coverage Report for Build 1570790157

• 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
• 21 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.01%) to 93.714%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/refactoring/refactoring_checker.py	2	98.16%
pylint/extensions/_check_docs_utils.py	19	92.66%

Totals
Change from base Build 1566586190:	0.01%
Covered Lines:	14192
Relevant Lines:	15144

---

💛 - Coveralls

[jacobtylerwalls]

Thanks for the speedy reply!

As a user I will be more annoyed by having to add disable's everywhere because one of my tools is producing false positives

So my suggestion here for the user would be to just define the variables before the try rather than add disables.

Basically it comes down to whether we want to assume all try statement fail or all succeed.

I agree this is a design question. I think this approach splits the difference well. We assume the try statements fail for the sake of finally statements relying on names that might not exist, which is easy for users to fix by initializing their variables, but we assume the try statements succeed for the sake of evaluating unused-variable, which I would agree would be quite annoying to disable if the variable is used later in the try block. How does that strike you?

I know it's not surveying opinions on the other side very well, but there's 13 +1's on the ticket. I tend to agree with the Pylance maintainer that if you're depending on a name in the finally block, then that really is an error--either something should be moved out of the try, or else the variable should be initialized before the try.

[Pierre-Sassoulas]

If you are confident that a statement will never raise an exception, you can move it outside of the try block

I found this argument from Eric Traut pretty convincing, honestly. By adding a statement in a Try except the user tell us that the code can fail so we should probably treat the code as possibly failing. If something never fail it should be outside the try and the resulting code will be clearer and better.

[DanielNoord]

Does it make sense to create "possibly-used-before-assignment" for this? Like we have "possibly-unused-variable"? I think @Pierre-Sassoulas and I discussed at some point prior.

If not, I think we should go with the majority opinion and continue with this PR!

[Pierre-Sassoulas]

LGTM,! Thank you. I think that taking the same decision than pylance is the safe bet here. But I'm putting this in 2.13 so opinion can be heard about the controversial part of the issue if required.

[DanielNoord]

Small changes here and there! Rest looks good! Glad to see such an old issue finally resolved even if I did not initially agree with the solution. I'm must admit I do think this might be the best solution for this problem going forward. Thanks @jacobtylerwalls for putting this to our attention! 😄

[jacobtylerwalls]

Let's not merge until I solve #2615 with a very similar block. I think I can use the same branch.

[Pierre-Sassoulas]

@jacobtylerwalls , we merged #5402 (🎉), but I'm not sure how to resolve the conflict, could you rebase on the latest main, please ?

[DanielNoord]

Thanks! Most of this already looks good.

I thought about node.statement previously but never commented about it. Upon re-review I think it makes sense.

[DanielNoord]

Let's merge this in the current state and use #5506 to perfect this description 😄