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

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

Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Daniël van Noord <[email protected]>

Author: 3 people

ChangeLog

@@ -20,6 +20,12 @@ What's New in Pylint 2.13.5?
 ============================
 Release date: TBA
 
+* Fix false positive regression in 2.13.0 for ``used-before-assignment`` for
+  homonyms between variable assignments in try/except blocks and variables in
+  subscripts in comprehensions.
+
+  Closes #6069
+
 * Narrow the scope of the ``unnecessary-ellipsis`` checker to:
   * functions & classes which contain both a docstring and an ellipsis.
   * A body which contains an ellipsis ``nodes.Expr`` node & at least one other statement.

pylint/checkers/variables.py

@@ -623,8 +623,8 @@ def get_next_to_consume(self, node: nodes.Name) -> Optional[List[nodes.NodeNG]]:
         ):
             return found_nodes
 
-        # And is not part of a test in a filtered comprehension
-        if VariablesChecker._has_homonym_in_comprehension_test(node):
+        # And no comprehension is under the node's frame
+        if VariablesChecker._comprehension_between_frame_and_node(node):
             return found_nodes
 
         # Filter out assignments in ExceptHandlers that node is not contained in
@@ -1276,8 +1276,23 @@ def leave_functiondef(self, node: nodes.FunctionDef) -> None:
 
         global_names = _flattened_scope_names(node.nodes_of_class(nodes.Global))
         nonlocal_names = _flattened_scope_names(node.nodes_of_class(nodes.Nonlocal))
+        comprehension_target_names: List[str] = []
+
+        for comprehension_scope in node.nodes_of_class(nodes.ComprehensionScope):
+            for generator in comprehension_scope.generators:
+                self._find_assigned_names_recursive(
+                    generator.target, comprehension_target_names
+                )
+
         for name, stmts in not_consumed.items():
-            self._check_is_unused(name, node, stmts[0], global_names, nonlocal_names)
+            self._check_is_unused(
+                name,
+                node,
+                stmts[0],
+                global_names,
+                nonlocal_names,
+                comprehension_target_names,
+            )
 
     visit_asyncfunctiondef = visit_functiondef
     leave_asyncfunctiondef = leave_functiondef
@@ -1405,7 +1420,7 @@ def _undefined_and_used_before_checker(
                 continue
 
             action, nodes_to_consume = self._check_consumer(
-                node, stmt, frame, current_consumer, i, base_scope_type
+                node, stmt, frame, current_consumer, base_scope_type
             )
             if nodes_to_consume:
                 # Any nodes added to consumed_uncertain by get_next_to_consume()
@@ -1476,37 +1491,37 @@ def _should_node_be_skipped(
 
         return False
 
+    def _find_assigned_names_recursive(
+        self,
+        target: Union[nodes.AssignName, nodes.BaseContainer],
+        target_names: List[str],
+    ) -> None:
+        """Update `target_names` in place with the names of assignment
+        targets, recursively (to account for nested assignments).
+        """
+        if isinstance(target, nodes.AssignName):
+            target_names.append(target.name)
+        elif isinstance(target, nodes.BaseContainer):
+            for elt in target.elts:
+                self._find_assigned_names_recursive(elt, target_names)
+
     # pylint: disable=too-many-return-statements
     def _check_consumer(
         self,
         node: nodes.Name,
         stmt: nodes.NodeNG,
         frame: nodes.LocalsDictNodeNG,
         current_consumer: NamesConsumer,
-        consumer_level: int,
         base_scope_type: Any,
     ) -> Tuple[VariableVisitConsumerAction, Optional[List[nodes.NodeNG]]]:
         """Checks a consumer for conditions that should trigger messages."""
         # If the name has already been consumed, only check it's not a loop
         # variable used outside the loop.
-        # Avoid the case where there are homonyms inside function scope and
-        # comprehension current scope (avoid bug #1731)
         if node.name in current_consumer.consumed:
-            if utils.is_func_decorator(current_consumer.node) or not (
-                current_consumer.scope_type == "comprehension"
-                and self._has_homonym_in_upper_function_scope(node, consumer_level)
-                # But don't catch homonyms against the filter of a comprehension,
-                # (like "if x" in "[x for x in expr() if x]")
-                # https://github.com/PyCQA/pylint/issues/5586
-                and not (
-                    self._has_homonym_in_comprehension_test(node)
-                    # Or homonyms against values to keyword arguments
-                    # (like "var" in "[func(arg=var) for var in expr()]")
-                    or (
-                        isinstance(node.scope(), nodes.ComprehensionScope)
-                        and isinstance(node.parent, (nodes.Call, nodes.Keyword))
-                    )
-                )
+            # Avoid the case where there are homonyms inside function scope and
+            # comprehension current scope (avoid bug #1731)
+            if utils.is_func_decorator(current_consumer.node) or not isinstance(
+                node, nodes.ComprehensionScope
             ):
                 self._check_late_binding_closure(node)
                 self._loopvar_name(node)
@@ -2259,8 +2274,14 @@ def _loopvar_name(self, node: astroid.Name) -> None:
                 self.add_message("undefined-loop-variable", args=node.name, node=node)
 
     def _check_is_unused(
-        self, name, node, stmt, global_names, nonlocal_names: Iterable[str]
-    ):
+        self,
+        name,
+        node,
+        stmt,
+        global_names,
+        nonlocal_names: Iterable[str],
+        comprehension_target_names: List[str],
+    ) -> None:
         # Ignore some special names specified by user configuration.
         if self._is_name_ignored(stmt, name):
             return
@@ -2282,7 +2303,8 @@ def _check_is_unused(
         argnames = node.argnames()
         # Care about functions with unknown argument (builtins)
         if name in argnames:
-            self._check_unused_arguments(name, node, stmt, argnames, nonlocal_names)
+            if name not in comprehension_target_names:
+                self._check_unused_arguments(name, node, stmt, argnames, nonlocal_names)
         else:
             if stmt.parent and isinstance(
                 stmt.parent, (nodes.Assign, nodes.AnnAssign, nodes.Tuple)
@@ -2455,46 +2477,15 @@ def _should_ignore_redefined_builtin(self, stmt):
     def _allowed_redefined_builtin(self, name):
         return name in self.config.allowed_redefined_builtins
 
-    def _has_homonym_in_upper_function_scope(
-        self, node: nodes.Name, index: int
-    ) -> bool:
-        """Return whether there is a node with the same name in the
-        to_consume dict of an upper scope and if that scope is a function
-
-        :param node: node to check for
-        :param index: index of the current consumer inside self._to_consume
-        :return: True if there is a node with the same name in the
-                 to_consume dict of an upper scope and if that scope
-                 is a function, False otherwise
-        """
-        return any(
-            _consumer.scope_type == "function" and node.name in _consumer.to_consume
-            for _consumer in self._to_consume[index - 1 :: -1]
-        )
-
     @staticmethod
-    def _has_homonym_in_comprehension_test(node: nodes.Name) -> bool:
-        """Return True if `node`'s frame contains a comprehension employing an
-        identical name in a test.
-
-        The name in the test could appear at varying depths:
-
-        Examples:
-            [x for x in range(3) if name]
-            [x for x in range(3) if name.num == 1]
-            [x for x in range(3)] if call(name.num)]
-        """
-        closest_comprehension = utils.get_node_first_ancestor_of_type(
-            node, nodes.Comprehension
-        )
-        return (
-            closest_comprehension is not None
-            and node.frame(future=True).parent_of(closest_comprehension)
-            and any(
-                test is node or test.parent_of(node)
-                for test in closest_comprehension.ifs
-            )
+    def _comprehension_between_frame_and_node(node: nodes.Name) -> bool:
+        """Return True if a ComprehensionScope intervenes between `node` and its frame."""
+        closest_comprehension_scope = utils.get_node_first_ancestor_of_type(
+            node, nodes.ComprehensionScope
         )
+        return closest_comprehension_scope is not None and node.frame(
+            future=True
+        ).parent_of(closest_comprehension_scope)
 
     def _store_type_annotation_node(self, type_annotation):
         """Given a type annotation, store all the name nodes it refers to."""

tests/functional/u/unused/unused_argument.py

@@ -47,6 +47,12 @@ def metadata_from_dict(key):
     """
     return {key: str(value) for key, value in key.items()}
 
+
+def metadata_from_dict_2(key):
+    """Similar, but with more nesting"""
+    return {key: (a, b) for key, (a, b) in key.items()}
+
+
 # pylint: disable=too-few-public-methods,  misplaced-future,wrong-import-position
 from __future__ import print_function
 

tests/functional/u/unused/unused_argument.txt

@@ -1,9 +1,9 @@
 unused-argument:3:16:3:21:test_unused:Unused argument 'first':HIGH
 unused-argument:3:23:3:29:test_unused:Unused argument 'second':HIGH
 unused-argument:32:29:32:32:Sub.newmethod:Unused argument 'aay':INFERENCE
-unused-argument:54:13:54:16:function:Unused argument 'arg':HIGH
-unused-argument:61:21:61:24:AAAA.method:Unused argument 'arg':INFERENCE
-unused-argument:68:0:None:None:AAAA.selected:Unused argument 'args':INFERENCE
-unused-argument:68:0:None:None:AAAA.selected:Unused argument 'kwargs':INFERENCE
-unused-argument:87:23:87:26:BBBB.__init__:Unused argument 'arg':INFERENCE
-unused-argument:98:34:98:39:Ancestor.set_thing:Unused argument 'other':INFERENCE
+unused-argument:60:13:60:16:function:Unused argument 'arg':HIGH
+unused-argument:67:21:67:24:AAAA.method:Unused argument 'arg':INFERENCE
+unused-argument:74:0:None:None:AAAA.selected:Unused argument 'args':INFERENCE
+unused-argument:74:0:None:None:AAAA.selected:Unused argument 'kwargs':INFERENCE
+unused-argument:93:23:93:26:BBBB.__init__:Unused argument 'arg':INFERENCE
+unused-argument:104:34:104:39:Ancestor.set_thing:Unused argument 'other':INFERENCE

tests/functional/u/used/used_before_assignment_filtered_comprehension.py renamed to tests/functional/u/used/used_before_assignment_comprehension_homonyms.py

@@ -49,3 +49,47 @@ def func5():
     except ZeroDivisionError:
         k = None
         print(k, filtered)
+
+
+def func6(data, keys):
+    """Similar, but with a subscript in a key-value pair rather than the test
+    See https://github.com/PyCQA/pylint/issues/6069"""
+    try:
+        results = {key: data[key] for key in keys}
+    except KeyError as exc:
+        key, *_ = exc.args
+        raise Exception(f"{key} not found") from exc
+
+    return results
+
+
+def func7():
+    """Similar, but with a comparison"""
+    bools = [str(i) == i for i in range(3)]
+
+    try:
+        1 / 0
+    except ZeroDivisionError:
+        i = None
+        print(i, bools)
+
+
+def func8():
+    """Similar, but with a container"""
+    pairs = [(i, i) for i in range(3)]
+
+    try:
+        1 / 0
+    except ZeroDivisionError:
+        i = None
+        print(i, pairs)
+
+
+# Module level cases
+
+module_ints = [j | j for j in range(3)]
+try:
+    1 / 0
+except ZeroDivisionError:
+    j = None
+    print(j, module_ints)