Emit used-before-assignment for variables only defined under always false tests

[jacobtylerwalls]

Type of Changes

	Type
✓	✨ New feature

Description

Closes #4913

Determine if a variable is defined only under a code path guarded by an IF statement that astroid can determine is always falsy. Emit used-before-assignment if that variable is later accessed.

[DanielNoord]

@jacobtylerwalls This seems like it could benefit from pylint-dev/astroid#1189. Perhaps we should try and get that landed first?

[jacobtylerwalls]

I agree that's a good use of effort to get that merged. Not sure I immediately see the utility here, but it's certainly possible after looking more closely! :D

[DanielNoord]

I agree that's a good use of effort to get that merged. Not sure I immediately see the utility here, but it's certainly possible after looking more closely! :D

Well, if you don't see the usability than that's a clear point of review. The system introduced there should be "easily" extendable to also include is False, is sys.version etc. checks.
I am eagerly waiting for the PR to get merged/reviewed to start exploring some of the other constraints we could start checking for.

[jacobtylerwalls]

I guess it just comes down to how much to use inference in the VariablesChecker. This draft only infers the variables in the if tests, not the variables in the expressions under the tests, which is what I think that astroid PR would be useful for, no? (The tests themselves are executed unconditionally.) It would be a pretty large rewrite of used-before-assignment to simply infer all the variable usages and then if we get an Uninferable assume used-before-assignment. So that was my thinking -- but like I said, maybe my imagination will spark when looking more closely :D

[coveralls]

Pull Request Test Coverage Report for Build 3508970379

• 51 of 53 (96.23%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.003%) to 95.412%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/variables.py	51	53	96.23%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/variables.py	1	97.31%

Totals
Change from base Build 3505356469:	-0.003%
Covered Lines:	17449
Relevant Lines:	18288

---

💛 - Coveralls

[jacobtylerwalls]

Leaving notes for when I come back: many of the messages in the primer differ refer to this pattern:

if magic:
    magic_num = 1
...
if magic:
   print(magic_num)  # likely that nothing happened to `magic` in the meantime, so safe(ish)

Maybe a loose comparison of if-node tests or the pending astroid constraints PR could help.

[Pierre-Sassoulas]

I'm still in shock about how great the primer bot is. It's like developing pylint in easy mode.

[jacobtylerwalls]

Along the lines of #2835, this should probably be a new message possibly-used-before-assignment or possibly-unbound. The two patterns that the primer projects and pylint itself are failing on are:

1

if magic:
    magic_num = 1
...
if magic:
    print(magic_num)  # likely but not certain that nothing happened to `magic` in the meantime

2

# handle two possible values
if flag == "expected_val1":
    magic_num = 1
elif flag == "expected_val2":
    magic_num = 2
# no else ... so magic_num may not be defined ... but does this ever realistically happen?
print(magic_num)

[jacobtylerwalls]

Let me know if you think I should follow the idea below to reduce the size of the PR.

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are now emitted:

1. redefined-variable-type:
   Redefinition of indexer type from .ndarray to slice
   https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/multi.py#L3150

This comment was generated for commit 11e9b28

[Pierre-Sassoulas]

The tests cases are on point, but the code is complicated (because the problem we're trying to solve is complicated). I feel like we need a generic solution in astroid, something like infer raising a different exception when the variable can be unassigned, maybe ? Or an assigned boolean attribute in nodes ? There is an a lot of check that require control flow, implementing control flow in each checkers is going to be a lot more work than doing that in astroid directly. What do you think ?

[jacobtylerwalls]

I feel like we need a generic solution in astroid, something like infer raising a different exception when the variable can be unassigned, maybe ? Or an assigned boolean attribute in nodes ?

I just want to relate this to @DanielNoord's comment about the astroid constraint PR.

I think we might be conflating two things. The current proposal in pylint-dev/astroid#1189 is to add constraints on inferred values. Here, we're not using inference. We don't care what values undefined_var takes: if it's never defined, it's undefined! We don't need to infer it, and it would be a waste of time to do so.

There is an a lot of check that require control flow, implementing control flow in each checkers is going to be a lot more work than doing that in astroid directly.

I'm sympathetic to that, but another way to look at it would be this is a "soft-launch" of control flow, using just one message, where the pylint primer results look promising, and then as we iron out kinks, we can implement a similar approach in astroid later, and extend it for del and the rest.

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21:
The following messages are now emitted:

1. redefined-variable-type:
   Redefinition of fundamental type from django.forms.widgets.Widget to music21.pitch.Pitch
   https://github.com/cuthbertLab/music21/blob/6c1e5705b2140443b27cebfaa75826fde9dfe870/music21/pitch.py#L3688

Effect on pandas:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'mask' before assignment
   https://github.com/pandas-dev/pandas/blob/e660f2cf774c78d0d16b4af153b502a8abe49992/pandas/core/arrays/timedeltas.py#L954

Effect on psycopg:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'ports_out' before assignment
   https://github.com/psycopg/psycopg/blob/a8168869b762c821e7f07d1fc52d3642f899711e/psycopg/psycopg/conninfo.py#L333

This comment was generated for commit be571d1

[Pierre-Sassoulas]

The code in psycopg looks very suspicious, it might be a genuine bug. But in pandas it seems like a false positive.

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21:
The following messages are now emitted:

1. redefined-variable-type:
   Redefinition of fundamental type from django.forms.widgets.Widget to music21.pitch.Pitch
   https://github.com/cuthbertLab/music21/blob/6c1e5705b2140443b27cebfaa75826fde9dfe870/music21/pitch.py#L3688

Effect on psycopg:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'ports_out' before assignment
   https://github.com/psycopg/psycopg/blob/a8168869b762c821e7f07d1fc52d3642f899711e/psycopg/psycopg/conninfo.py#L333

This comment was generated for commit 2ae32ec

[Pierre-Sassoulas]

LGTM 👌 Feel free to merge, I'm not super comfortable with the variables checker and I don't understand everything, But the tests and the primer make me pretty confident it won't create too much issues.

[cdce8p]

This looks like a new false-positive. Assignment expressions are bound before the conditional check. Thus this executes just fine and will print False.

[jacobtylerwalls]

Good catch. Fixed in #7847.