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 false positive used-before-assignment in pattern matching with a guard #7793

Closed
wants to merge 10 commits into from

Conversation

i-likecheese
Copy link

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fix false positive used-before-assignment in pattern matching with a guard #5327

Closes #5327

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/2.15.x labels Nov 18, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.7 milestone Nov 18, 2022
@github-actions

This comment has been minimized.

@i-likecheese
Copy link
Author

Any guidance on what would be a proper way of testing this?

@coveralls
Copy link

coveralls commented Nov 18, 2022

Pull Request Test Coverage Report for Build 3521229696

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0002%) to 95.428%

Totals Coverage Status
Change from base Build 3521160535: 0.0002%
Covered Lines: 17472
Relevant Lines: 18309

πŸ’› - Coveralls

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Nov 18, 2022

Hey @i-likecheese , thank you for contributing. You can create a functional test using a configuration file like this so it's executed only for python 3.10. See the functional tests documentation

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this false positive!

I'd prefer to solve it closer to the source instead of at the very last minute, so that the variables checker, which is already pretty hard to reason about, stays somewhat workable.

I suggest handling this case in _maybe_used_and_assigned_at_once, perhaps like:

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index 627e82ef0..7ad0c0913 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -1963,6 +1963,7 @@ class VariablesChecker(BaseChecker):
                             nodes.AugAssign,
                             nodes.Expr,
                             nodes.Return,
+                            nodes.Match,
                         ),
                     )
                     and VariablesChecker._maybe_used_and_assigned_at_once(defstmt)
@@ -2053,6 +2054,8 @@ class VariablesChecker(BaseChecker):
         """Check if `defstmt` has the potential to use and assign a name in the
         same statement.
         """
+        if isinstance(defstmt, nodes.Match):
+            return any(case.guard for case in defstmt.cases)
         if isinstance(defstmt.value, nodes.BaseContainer) and defstmt.value.elts:
             # The assignment must happen as part of the first element
             # e.g. "assert (x:= True), x"

Comment on lines +1645 to +1648
) and not (
PY310_PLUS
and isinstance(stmt, nodes.Match)
and isinstance(node.parent, nodes.Compare)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't solve this false positive, because the .parent is nodes.Name:

match ("example", "one"):
    case (x, y) if x:  # <-- pylint complains about 'x' here
        print("woo!")
    case _:
        print("boo...")

@github-actions
Copy link
Contributor

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

This comment was generated for commit be2bffe

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.7, 2.15.8 Nov 24, 2022
@jacobtylerwalls jacobtylerwalls changed the title Fix false positive used-before-assignment in pattern matching with a guard #5327 Fix false positive used-before-assignment in pattern matching with a guard Nov 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.15.8 milestone Dec 4, 2022
@jacobtylerwalls
Copy link
Member

Superseded by #7922.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive used-before-assignment in pattern matching with a guard.
4 participants