Skip to content

Fix #5683: False positive used-before-assignment in loop else where the only non-break exit is through except #5684

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 27, 2022
Merged

Fix #5683: False positive used-before-assignment in loop else where the only non-break exit is through except #5684

merged 1 commit into from
Jan 27, 2022

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
✓ 🐛 Bug fix

Description

Closes #5683

@jacobtylerwalls jacobtylerwalls force-pushed the single-exit-into-loop-else branch from 4a6fdf8 to 89d54f0 Compare January 15, 2022 04:59
@coveralls
Copy link

coveralls commented Jan 15, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1756653853

  • 41 of 41 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 93.862%
Totals Coverage Status
Change from base Build 1751757499: 0.02%
Covered Lines: 14757
Relevant Lines: 15722
💛 - Coveralls

@jacobtylerwalls jacobtylerwalls force-pushed the single-exit-into-loop-else branch from 9056be0 to 78b3482 Compare January 15, 2022 05:30
@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jan 15, 2022
DanielNoord
DanielNoord requested changes Jan 15, 2022
View reviewed changes
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Thanks @jacobtylerwalls! That's quite a lot of code!

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Jan 15, 2022
View reviewed changes
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.

LGTM, the typing issue is minor even if we might want to dig into it before merging (fix it then rebase on it to remove the typing that became uneeded?). Thank you @jacobtylerwalls , great addition to 2.13 !

pylint/checkers/variables.py
):
break
else:
# No continue found, so we arrived at our special case!
Copy link
Member

Choose a reason for hiding this comment

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

That was a whole journey 😄 !

tests/functional/u/use/used_before_assignment_issue4761.py
print('The time is:')
break
else:
raise error
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we're going to still be surprised by the code that exists in the wild, but I'm also pretty confident we won't be surprised often. Those tests are thorough !

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Jan 15, 2022
@jacobtylerwalls jacobtylerwalls mentioned this pull request Jan 15, 2022
Merged
@jacobtylerwalls jacobtylerwalls force-pushed the single-exit-into-loop-else branch from 5a105a1 to 453c00a Compare January 15, 2022 17:48
DanielNoord
DanielNoord approved these changes Jan 23, 2022
View reviewed changes
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Sorry! Should have changed my review to approved after the last comments were integrated!

@DanielNoord
Copy link
Collaborator

@jacobtylerwalls Just something I thought of, I see the confidence of used-before-assignment is UNDEFINED. I believe this message will likely always require some form of inference (right?). Should we update the confidence of the message to be INFERENCE?

@jacobtylerwalls
Copy link
Member Author

What about a new level CONTROL_FLOW? It's not really node-inference dependent.

@DanielNoord
Copy link
Collaborator

I like that a lot!

@jacobtylerwalls
Copy link
Member Author

Cool, I'll PR it unless you've already started 🏃🏻‍♂️

@DanielNoord
Copy link
Collaborator

No I haven't, would certainly welcome a PR 😄

@jacobtylerwalls jacobtylerwalls mentioned this pull request Jan 23, 2022
Merged
@DanielNoord
Copy link
Collaborator

@jacobtylerwalls This seems to have landed in stale PR heaven 😄 If you could update the test output I will merge this asap!

@jacobtylerwalls
Fix pylint-dev#5683: False positive used-before-assignment in loo…
ec6f373 
…p `else` where the only non-break exit is via `except`
@jacobtylerwalls jacobtylerwalls force-pushed the single-exit-into-loop-else branch from c02c8ae to ec6f373 Compare January 27, 2022 14:16
@DanielNoord DanielNoord merged commit a8f81db into pylint-dev:main Jan 27, 2022
@DanielNoord
Copy link
Collaborator

Thanks!

@jacobtylerwalls jacobtylerwalls deleted the single-exit-into-loop-else branch January 27, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@DanielNoord DanielNoord DanielNoord approved these changes

Assignees
No one assigned
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

False positive for used-before-assignment in else clause when the only non-break exit from a loop is an except block
4 participants
@jacobtylerwalls @coveralls @DanielNoord @Pierre-Sassoulas