Skip to content

Fix #5326: Fix false positive for unused-variable for a comprehension variable matching a type annotation #5651

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

Merged

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jan 8, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fixes the regression described in #5326 where undefined-variable is emitted when a comprehension variable matches a type annotation.

One test result is edited to show that after these changes we will need better detection of walrus operators.

Fixes #5326

@coveralls
Copy link

coveralls commented Jan 8, 2022

Pull Request Test Coverage Report for Build 1672085133

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.704%

Totals Coverage Status
Change from base Build 1663696938: 0.0%
Covered Lines: 14363
Relevant Lines: 15328

πŸ’› - Coveralls

@DanielNoord DanielNoord self-requested a review January 8, 2022 18:50
@DanielNoord
Copy link
Collaborator

Knowing you you have probably already investigated this, but it was impossible to fix this without creating the false negative?

@jacobtylerwalls
Copy link
Member Author

Knowing you you have probably already investigated this, but it was impossible to fix this without creating the false negative?

Ha. So my thought there was that it was wrong to make _is_only_type_assignment() responsible for finding undefined variables in self-referencing walrus operator expressions. Pylint already has trouble with that, and it has nothing to do with with type annotations at all. I'll open a new issue.

$ cat test.py
'''Should raise undefined-variable'''
if (var := var):
    print(var)
$ pylint test.py

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

I'm assuming that's the false negative you meant, since it was "created" instead of "left alone." (But "created" only insofar as we were getting lucky 🎲 .)

If you meant the false negative I started to describe in #5326 (comment), then to keep things sane I'll just break that out into a new issue. I think they can be addressed separately.

@jacobtylerwalls jacobtylerwalls changed the title Fix false positive for unused-variable for a comprehension variable matching a type annotation Fix #5326: Fix false positive for unused-variable for a comprehension variable matching a type annotation Jan 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jan 9, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Jan 9, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jacobtylerwalls, very elegant fix !

@@ -1803,7 +1803,7 @@ def _is_only_type_assignment(node: nodes.Name, defstmt: nodes.Statement) -> bool
# Local refs are ordered, so we break.
# print(var)
# var = 1 # <- irrelevant
if defstmt_frame == node_frame and not ref_node.lineno < node.lineno:
if defstmt_frame == node_frame and ref_node.lineno > node.lineno:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the small optimization to save a "not" on top of the clever fix !

@Pierre-Sassoulas Pierre-Sassoulas merged commit b376395 into pylint-dev:main Jan 10, 2022
@jacobtylerwalls jacobtylerwalls deleted the type-annotation-unused branch January 10, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive undefined-variable for variable in inner scope matching outer scope type annotation
4 participants