Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

used-before-assignment false positive with list comprehension and try/except #6035

Closed
The-Compiler opened this issue Mar 29, 2022 · 7 comments · Fixed by #6040
Closed

used-before-assignment false positive with list comprehension and try/except #6035

The-Compiler opened this issue Mar 29, 2022 · 7 comments · Fixed by #6040
Assignees
Labels
C: used-before-assignment Issues related to 'used-before-assignment' check False Positive 🦟 A message is emitted but nothing is wrong with the code Regression
Milestone

Comments

@The-Compiler
Copy link
Contributor

The-Compiler commented Mar 29, 2022

Bug description

Somewhat similarly to #5965,

def github_upload(filename, all_assets):
    assets = [asset for asset in all_assets if asset.name == filename]

    try:
        pass
    except ValueError:
        asset = assets[0]
        asset.delete()

leads to an used-before-assignment false-positive for the second line, yet asset is always defined there.

I bisected this to bd55b27, part of #5402 ("Fix #4761: Emit used-before-assignment where single assignment only made in except blocks")

Configuration

No response

Command used

pylint -E repro1.py

Pylint output

************* Module repro1
repro1.py:2:47: E0601: Using variable 'asset' before assignment (used-before-assignment)

Expected behavior

No error.

Pylint version

pylint 2.13.2
astroid 2.11.2
Python 3.8.12 (default, Mar  3 2022, 18:19:28) 
[GCC 11.2.0]

(environment in which I originally encountered the bug - output above is from a git install of both astroid and pylint)

Also happens with latest git main (astroid b18c7828, pylint 0bc45e9). Does not happen on pylint 2.12.2 and astroid 2.9.3 which I used previously.

OS / Environment

Archlinux

Additional dependencies

No response

@The-Compiler The-Compiler added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Mar 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code C: used-before-assignment Issues related to 'used-before-assignment' check and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Mar 29, 2022
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this issue Mar 29, 2022
@jacobtylerwalls jacobtylerwalls self-assigned this Mar 29, 2022
@jacobtylerwalls
Copy link
Member

Thanks for the well-written report! Extremely similar to #5586 and #5817, but still missed it before the release.

@jacobtylerwalls jacobtylerwalls added this to the 2.13.4 milestone Mar 29, 2022
skshetry added a commit to iterative/dvc that referenced this issue Mar 30, 2022
Waiting for next release for some false-positives to be fixed.
Especially, `used-before-assignment` and `unnecessary-ellipsis`.

See pylint-dev/pylint#6035.
@SteveHespelt
Copy link

in the spirit of possibly being helpful:

somewhat similar list comprehension statement, but not in a try-except block

if isinstance(d, dict):
    dict_keys =  [c for c in d if isinstance(d[c], dict)]

E:191,59: Using variable 'c' before assignment (used-before-assignment)

pylint 2.13.3, pytest-pylint 0.18.0, python 3.8.13, pytest-7.1.1

@jacobtylerwalls
Copy link
Member

I can't reproduce with that example:

$ cat a.py
if isinstance(d, dict):
    dict_keys =  [c for c in d if isinstance(d[c], dict)]


$ python3 -m pylint a.py
************* Module a
a.py:1:14: E0602: Undefined variable 'd' (undefined-variable)
a.py:2:29: E0602: Undefined variable 'd' (undefined-variable)
a.py:2:45: E0602: Undefined variable 'd' (undefined-variable)

@SteveHespelt
Copy link

my apologies @jacobtylerwalls - my example wasn't meant to be complete, just the minimal snippet showing the statement.
Extremely interesting: the error was reported in a CI pipeline using Python 3.8.13, the other versions I reported earlier.
Not using pytest-pylint, I get the following.
Not being a Python expert/guru, I'm concerned over the difference between the full codebase scan via pytest-pylint and pylint standalone on this snippet. I need to do some more digging.

My immediate take-away: any pytest-pylint reported errors, rerun with standalone pylint to see if any differences.
Interesting...

/tmp/a.py:

d = { 'a': {}, 'b':1 }
dict_keys = []
if isinstance(d, dict):
    dict_keys =  [c for c in d if isinstance(d[c], dict)]
print("dict_keys: " + str(dict_keys))
python3.8 /tmp/a.py

dict_keys: ['a']

python3.7 -m pylint /tmp/a.py

************* Module a
/tmp/a.py:1:0: C0114: Missing module docstring (missing-module-docstring)
/tmp/a.py:7:0: C0206: Consider iterating with .items() (consider-using-dict-items)

With python3.8:

python3.8 -m pylint --version

pylint 2.13.3
astroid 2.11.2
Python 3.8.13 (default, Mar 29 2022, 19:52:48)
[GCC 8.3.0]

root@abb086d6fbf6:/usr/src/app# python3.8 -m pylint /tmp/a.py

************* Module a
/tmp/a.py:1:0: C0114: Missing module docstring (missing-module-docstring)
/tmp/a.py:4:0: C0206: Consider iterating with .items() (consider-using-dict-items)

@jacobtylerwalls
Copy link
Member

Thanks, I misunderstood -- thought you were saying you found a case reproducible without a try/except block. I think it's worth adding this case to the tests.

@skshetry
Copy link
Contributor

skshetry commented Mar 31, 2022

I can still reproduce this in 2.13.4:

# test.py
def select_dict(data, keys):
    try:
        d = {key: data[key] for key in keys}
    except KeyError as e:
        key, *_ = e.args
        raise Exception(f"{key} not found")

    return d
$ pip install -U pylint==2.13.4 -q && pylint test.py
************* Module test
test.py:3:23: E0601: Using variable 'key' before assignment (used-before-assignment)

------------------------------------------------------------------
Your code has been rated at 2.86/10 (previous run: 2.86/10, +0.00)

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Mar 31, 2022

Thanks for the report: this one does not involve an if test in the comprehension, so I'm going to open a new issue for it. I have a patch ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: used-before-assignment Issues related to 'used-before-assignment' check False Positive 🦟 A message is emitted but nothing is wrong with the code Regression
Projects
None yet
5 participants