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 FP used-before-assignment for statements guarded under same test #8581

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes #8167

@jacobtylerwalls jacobtylerwalls 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 backport maintenance/3.3.x labels Apr 16, 2023
@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Merging #8581 (e968a12) into main (2db55f6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8581   +/-   ##
=======================================
  Coverage   95.91%   95.92%           
=======================================
  Files         174      174           
  Lines       18371    18411   +40     
=======================================
+ Hits        17620    17660   +40     
  Misses        751      751           
Impacted Files Coverage Ξ”
pylint/checkers/variables.py 97.25% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.3 milestone Apr 16, 2023
@github-actions
Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on music21:
The following messages are no longer emitted:

  1. used-before-assignment:
    Using variable 'stream' before assignment
    https://github.com/cuthbertLab/music21/blob/f05ec0af047977eaa8a2c09f7cf99892a5983eb7/music21/stream/iterator.py#L1844
  2. used-before-assignment:
    Using variable 'Stream' before assignment
    https://github.com/cuthbertLab/music21/blob/f05ec0af047977eaa8a2c09f7cf99892a5983eb7/music21/stream/core.py#L554

Effect on pandas:
The following messages are now emitted:

  1. useless-suppression:
    Useless suppression of 'used-before-assignment'
    https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L117

The following messages are no longer emitted:

  1. used-before-assignment:
    Using variable 'groupby' before assignment
    https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L232
  2. used-before-assignment:
    Using variable 'groupby' before assignment
    https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L241
  3. suppressed-message:
    Suppressed 'used-before-assignment' (from line 117)
    https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L117

This comment was generated for commit e968a12

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 primer result bring joy to my heart.

return None

if give_me_none():
print(ALL_DONE) # [used-before-assignment]
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 those tests πŸ‘

@@ -819,6 +822,38 @@ def _uncertain_nodes_in_false_tests(

return uncertain_nodes

@staticmethod
def _node_guarded_by_same_test(node: nodes.NodeNG, other_if: nodes.If) -> bool:
"""Identify if `node` is guarded by an equivalent test as `other_if`.
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of inference here, but the primer time vary in the same range than it usually does, so the performance impact is probably acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm assuming that all of these nodes are inferred anyway elsewhere in the variables checker, so we're hitting the cache.

@jacobtylerwalls jacobtylerwalls merged commit f45bf09 into pylint-dev:main Apr 17, 2023
@jacobtylerwalls jacobtylerwalls deleted the same-test branch April 17, 2023 00:50
@github-actions
Copy link
Contributor

The backport to maintenance/2.17.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/2.17.x maintenance/2.17.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.17.x
# Create a new branch
git switch --create backport-8581-to-maintenance/2.17.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f45bf090a8e20c9fb09d61ed67d7f885ad354f85
# Push it to GitHub
git push --set-upstream origin backport-8581-to-maintenance/2.17.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.17.x

Then, create a pull request where the base branch is maintenance/2.17.x and the compare/head branch is backport-8581-to-maintenance/2.17.x.

@Pierre-Sassoulas Pierre-Sassoulas added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer and removed backport maintenance/3.3.x labels Apr 17, 2023
@Pierre-Sassoulas
Copy link
Member

I think this is going very hard to backport (I tried but there's a lot of concurrent changes), what do you think @jacobtylerwalls ?

@jacobtylerwalls
Copy link
Member Author

I tried also. I'm okay waiting for the 3.0 beta for this. music21 can always use bleeding-edge pylint πŸ—‘οΈ

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Apr 18, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.17.3, 3.0.0b1 Apr 18, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a7 May 6, 2023
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
Projects
None yet
2 participants