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

Fix used-before-assignment false positive for walrus operators in ifs #8029

Merged
merged 8 commits into from
Jan 8, 2023
Merged

Fix used-before-assignment false positive for walrus operators in ifs #8029

merged 8 commits into from
Jan 8, 2023

Conversation

zenlyj
Copy link
Contributor

@zenlyj zenlyj commented Jan 7, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

After investigation, there seems to be two main cases that trigger used-before-assignment false positives:

  1. Ternary expression with walrus operator within attribute calls. Eg. (foo if (foo:='foo') else 'bar').upper()
  2. Ternary expression with walrus operator nested in function calls. Eg. str(str(foo if (foo:='foo') else 'bar'))

This fix enforces a stricter check on whether a name may be assigned and used in the same statement.

Closes #7779

@zenlyj zenlyj changed the title Fix used-before-assignment false positive Fix used-before-assignment false positive Jan 7, 2023
@github-actions

This comment has been minimized.

pylint/checkers/variables.py Outdated Show resolved Hide resolved
pylint/checkers/utils.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jan 7, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.10 milestone Jan 7, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the C: used-before-assignment Issues related to 'used-before-assignment' check label Jan 7, 2023
@github-actions

This comment has been minimized.

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.

Maybe a simplification with early return ?

pylint/checkers/variables.py Show resolved Hide resolved
pylint/checkers/variables.py Outdated Show resolved Hide resolved
@mbyrnepr2
Copy link
Member

@zenlyj the changelog file is required. There are a number here if you need to check how that is done

@github-actions

This comment has been minimized.

@mbyrnepr2
Copy link
Member

@zenlyj The Python 3.7 test in the CI is failing because Walrus is a syntax-error. It's possible to run this test for Python versions greater than 3.7. There are many examples in the existing functional tests where a .rc file, similar to this one, is added with the same file-name as the functional test module, for the tests to be run for the specification in the .rc file.

@codecov
Copy link

codecov bot commented Jan 7, 2023

Codecov Report

Merging #8029 (cad42be) into main (89a20e2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8029   +/-   ##
=======================================
  Coverage   95.42%   95.42%           
=======================================
  Files         176      176           
  Lines       18525    18527    +2     
=======================================
+ Hits        17678    17680    +2     
  Misses        847      847           
Impacted Files Coverage Ξ”
pylint/checkers/variables.py 97.38% <100.00%> (+<0.01%) ⬆️

@zenlyj
Copy link
Contributor Author

zenlyj commented Jan 7, 2023

@mbyrnepr2 Thank you for helping with the CI fails, very helpful advice.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2023

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit cad42be

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.

Thank you for the fix, the test cases are pretty nice too. This will be available in 2.15.10 :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 6ac908f into pylint-dev:main Jan 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas changed the title Fix used-before-assignment false positive Fix used-before-assignment false positive for walrus operators in ifs Jan 8, 2023
github-actions bot pushed a commit that referenced this pull request Jan 8, 2023
…fs (#8029)

Co-authored-by: Pierre Sassoulas <[email protected]>
(cherry picked from commit 6ac908f)
@zenlyj zenlyj deleted the bug-used-before-assign branch January 8, 2023 13:48
Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2023
…fs (#8029) (#8033)

Co-authored-by: Pierre Sassoulas <[email protected]>
(cherry picked from commit 6ac908f)

Co-authored-by: Zen Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported C: used-before-assignment Issues related to 'used-before-assignment' check 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 used-before-assignment with inline if
3 participants