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

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #5817

[Pierre-Sassoulas]

Amazing speedy fix ! ⚡

[Pierre-Sassoulas]

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

[jacobtylerwalls]

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

[jacobtylerwalls]

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.

[jacobtylerwalls]

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

[jacobtylerwalls]

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

[Pierre-Sassoulas]

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

[jacobtylerwalls]

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

[Pierre-Sassoulas]

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

[coveralls]

Pull Request Test Coverage Report for Build 1859227693

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.991%

---

Totals
Change from base Build 1840800912:	0.0%
Covered Lines:	14921
Relevant Lines:	15875

---

💛 - Coveralls