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.

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

Account for more node types in handling of except block homonyms with comprehensions #6073

Merged
merged 14 commits into from
Apr 2, 2022
Merged

Account for more node types in handling of except block homonyms with comprehensions #6073

merged 14 commits into from
Apr 2, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Mar 31, 2022 •
edited
Loading

Type of Changes

Type
✓ 🐛 Bug fix

Description

Several nodes types didn't meet the allow list for homynym handling b/w except blocks and comprehensions.

Closes #6069

@jacobtylerwalls
Add Subscript to homonym handling for except blocks and comprehensions
158f4a0 
Fixes a false positive for `used-before-assignment`.
@jacobtylerwalls jacobtylerwalls added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer C: used-before-assignment Issues related to 'used-before-assignment' check labels Mar 31, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.13.5 milestone Mar 31, 2022
@coveralls
Copy link

coveralls commented Mar 31, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2081793733

  • 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
  • 105 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.03%) to 94.245%
Files with Coverage Reduction New Missed Lines %
pylint/checkers/design_analysis.py 1 98.9%
pylint/config/config_initialization.py 1 93.18%
pylint/config/option_manager_mixin.py 1 88.55%
pylint/config/argument.py 2 94.59%
pylint/checkers/exceptions.py 3 97.72%
pylint/checkers/format.py 3 96.5%
pylint/checkers/base/basic_checker.py 8 97.94%
pylint/checkers/misc.py 9 83.95%
pylint/utils/utils.py 14 86.98%
pylint/checkers/imports.py 15 91.36%
Totals Coverage Status
Change from base Build 2079916425: -0.03%
Covered Lines: 15541
Relevant Lines: 16490
💛 - Coveralls

DanielNoord
DanielNoord previously approved these changes Mar 31, 2022
View reviewed changes
@jacobtylerwalls jacobtylerwalls changed the title Add Subscript to homonym handling for except blocks and comprehensions Add Subscript and Compare to homonym handling for except blocks and comprehensions Mar 31, 2022
@jacobtylerwalls jacobtylerwalls changed the title Add Subscript and Compare to homonym handling for except blocks and comprehensions Account for more node types in handling of except blocks homonyms with comprehensions Mar 31, 2022
@jacobtylerwalls jacobtylerwalls changed the title Account for more node types in handling of except blocks homonyms with comprehensions Account for more node types in handling of except block homonyms with comprehensions Mar 31, 2022
@jacobtylerwalls
Copy link
Member Author

I'm testing another approach. Less whackamole.

@jacobtylerwalls jacobtylerwalls marked this pull request as draft March 31, 2022 15:17
@jacobtylerwalls
Copy link
Member Author

Close, but couldn't avoid breaking this test. Back to whackamole.

https://github.com/PyCQA/pylint/blob/cb9dc791a0e83f90391cc234b03cb5e4769400b3/tests/functional/u/unused/unused_argument.py#L43-L48

@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review March 31, 2022 16:02
@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Mar 31, 2022 •
edited
Loading

Ah, okay, I do have another diff that works. But it's not battle-tested enough for a hotfix timeline. I can follow up for 2.14. Here it is!

@jacobtylerwalls
Copy link
Member Author

Hi @skshetry, I looked for an approach that didn't involve special-casing each type of NodeNG that might appear (subscript, etc.). Could I ask you to test this PR against your project? That would give me more confidence going into a quick patch release timeline. Thanks.

@jacobtylerwalls
Copy link
Member Author

anyone is welcome to review, just hitting the button to dismiss the now moot review!

Pierre-Sassoulas
Pierre-Sassoulas reviewed Mar 31, 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.

I don't see any problem here, but I'm really out of touch with used-before-assignment at this point 😄 I'm wondering if astroid could help with multiple imbricated scopes in a generic "simpler" way. How is it done in other linter ? Is it supposed to be that hard ? Is there an issue with the representation of the AST ? Maybe a state of the art could help ?

DanielNoord
DanielNoord approved these changes Apr 1, 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.

I like this approach! Well done figuring it out!

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

Is it supposed to be that hard ?

Thanks, you inspired me to delete more code in e35d1da.

DanielNoord
DanielNoord approved these changes Apr 2, 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.

Only the return type needs to be added 😄

pylint/checkers/variables.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas @DanielNoord
Update pylint/checkers/variables.py
ef98184 
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas merged commit d294c49 into pylint-dev:main Apr 2, 2022
@jacobtylerwalls jacobtylerwalls deleted the homonym-subscript branch April 2, 2022 12:32
This was referenced Apr 2, 2022
Closed
Merged
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Apr 4, 2022
Pierre-Sassoulas added a commit that referenced this pull request Apr 4, 2022
@jacobtylerwalls @Pierre-Sassoulas @DanielNoord
Account for more node types in handling of except block homonyms with…
22b5dc1 
… comprehensions (#6073)

Fixes a false positive for `used-before-assignment`.

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
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 left review comments

@DanielNoord DanielNoord DanielNoord approved these changes

Assignees
No one assigned
Labels
Backported C: used-before-assignment Issues related to 'used-before-assignment' check
Projects
None yet
Milestone
2.13.5
4 participants
@jacobtylerwalls @coveralls @Pierre-Sassoulas @DanielNoord