Skip to content

Fix used-before-assignment false positive for except handler names shared by comprehension test #5818

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

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Fix used-before-assignment false positive for except handler names shared by comprehension test #5818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,11 @@ Release date: TBA

Closes #4716

* Fix false positive for ``used-before-assignment`` when an except handler
shares a name with a test in a filtered comprehension.

Closes #5817

* Fix crash in ``unnecessary-dict-index-lookup`` checker if the output of
``items()`` is assigned to a 1-tuple.

Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.13.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ Other Changes

Closes #4716

* Fix false positive for ``used-before-assignment`` when an except handler
shares a name with a test in a filtered comprehension.

Closes #5817

* Fix a crash in ``unused-private-member`` checker when analyzing code using
``type(self)`` in bound methods.

Expand Down
7 changes: 6 additions & 1 deletion pylint/checkers/variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,12 @@ def get_next_to_consume(self, node: nodes.Name) -> Optional[List[nodes.NodeNG]]:
found_nodes = None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
found_nodes = None
found_nodes = None
if found_nodes and (
isinstance(parent_node, nodes.Comprehension)
or node in parent_node.ifs
):
# this is a test in a filtered comprehension
# Example: [e for e in range(3) if e] <--- followed by except e:
found_nodes = None

Nitpick but I think this would match the style of the previous code better

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to the idea of rewriting it somehow, but I don't see how this way would work -- it's different logically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see you're suggesting to make this into two cases rather than one. This might work, I'll push and see if you like it better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, unfortunately it fails to fix the original issue, changing used-before-assignment to undefined-variable instead of fixing it.

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index 836d1947..a6ace64c 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -645,13 +645,13 @@ scope_type : {self._atomic.scope_type}
         ):
             found_nodes = None
 
-        # Filter out assignments in ExceptHandlers that node is not contained in
-        # unless this is a test in a filtered comprehension
+        # This is a test in a filtered comprehension
         # Example: [e for e in range(3) if e] <--- followed by except e:
-        if found_nodes and (
-            not isinstance(parent_node, nodes.Comprehension)
-            or node not in parent_node.ifs
-        ):
+        if found_nodes and isinstance(parent_node, nodes.Comprehension) and node in parent_node.ifs:
+            return None
+
+        # Filter out assignments in ExceptHandlers that node is not contained in
+        if found_nodes:
             found_nodes = [
                 n
                 for n in found_nodes

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, since we don't want to set found_nodes to None, we just don't want to do the special filtering.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry my bad I did not understand what it did well apparentely. Thank you for testing it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand, it's not very clear from Filter out assignments in ExceptHandlers that node is not contained in what the expected state is afterward.

I'm starting to think that we're accumulating too many filtered comprehension / keywords in calls cases. A tricky but potentially useful refactor might be to detect them earlier someplace.

Two other places besides this one we just merged doing a similar dance:
https://github.com/PyCQA/pylint/blob/4f385643bc24483dee9aef8f124498dd676fab85/pylint/checkers/variables.py#L1319-L1325

And from #5812:
https://github.com/PyCQA/pylint/blob/4a8816ccdad457ffd874d5d9fb8efae691c2fe0e/pylint/checkers/variables.py#L2070-L2077

Copy link
Member

Choose a reason for hiding this comment

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

I trust you if you say we can do a refactor in the variables checker, you're clearly the world expert now with all the work you did on used-before-assignment


# Filter out assignments in ExceptHandlers that node is not contained in
if found_nodes:
# unless this is a test in a filtered comprehension
# Example: [e for e in range(3) if e] <--- followed by except e:
if found_nodes and (
not isinstance(parent_node, nodes.Comprehension)
or node not in parent_node.ifs
):
found_nodes = [
n
for n in found_nodes
Expand Down
7 changes: 7 additions & 0 deletions tests/functional/u/used/used_before_assignment_issue626.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,10 @@ def main4():
pass

print(e) # [used-before-assignment]


def main5():
try:
print([e for e in range(3) if e])
except ValueError as e:
print(e)