Enhance nonlocal check to handle nesting

Author: zenlyj

pylint/checkers/variables.py

@@ -303,6 +303,18 @@ def _assigned_locally(name_node: nodes.Name) -> bool:
     )
 
 
+def _is_before(node: nodes.NodeNG, reference_node: nodes.NodeNG) -> bool:
+    """Returns True if node appears before reference_node, False otherwise."""
+    if node.lineno < reference_node.lineno:
+        return True
+    if (
+        node.lineno == reference_node.lineno
+        and node.col_offset < reference_node.col_offset
+    ):
+        return True
+    return False
+
+
 def _has_locals_call_after_node(stmt: nodes.NodeNG, scope: nodes.FunctionDef) -> bool:
     skip_nodes = (
         nodes.FunctionDef,
@@ -503,6 +515,8 @@ def __init__(self, node: nodes.NodeNG, scope_type: str):
         self.names_under_always_false_test: set[str] = set()
         self.names_defined_under_one_branch_only: set[str] = set()
 
+        self.defined_by_nonlocal = False
+
     def __repr__(self) -> str:
         _to_consumes = [f"{k}->{v}" for k, v in self.to_consume.items()]
         _consumed = [f"{k}->{v}" for k, v in self.consumed.items()]
@@ -531,7 +545,9 @@ def mark_as_consumed(self, name: str, consumed_nodes: list[nodes.NodeNG]) -> Non
         else:
             del self.to_consume[name]
 
-    def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
+    def get_next_to_consume(
+        self, node: nodes.Name, visited_nonlocal_nodes: list[list[nodes.Nonlocal]]
+    ) -> list[nodes.NodeNG] | None:
         """Return a list of the nodes that define `node` from this scope.
 
         If it is uncertain whether a node will be consumed, such as for statements in
@@ -561,13 +577,6 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
         ):
             found_nodes = None
 
-        # Before filtering, check that this node's name is not a nonlocal
-        if any(
-            isinstance(child, nodes.Nonlocal) and node.name in child.names
-            for child in node.frame().get_children()
-        ):
-            return found_nodes
-
         # And no comprehension is under the node's frame
         if VariablesChecker._comprehension_between_frame_and_node(node):
             return found_nodes
@@ -581,6 +590,18 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
                 or n.statement().parent_of(node)
             ]
 
+        # Filter out definitions that are shadowed by an existing nonlocal definition
+        if found_nodes:
+            uncertain_nodes = self._uncertain_nodes_nonlocal(
+                found_nodes, node, visited_nonlocal_nodes
+            )
+            self.consumed_uncertain[node.name] += uncertain_nodes
+            uncertain_nodes_set = set(uncertain_nodes)
+            found_nodes = [n for n in found_nodes if n not in uncertain_nodes_set]
+            if not found_nodes:
+                self.defined_by_nonlocal = True
+                return []
+
         # Filter out assignments guarded by always false conditions
         if found_nodes:
             uncertain_nodes = self._uncertain_nodes_if_tests(found_nodes, node)
@@ -624,6 +645,42 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
 
         return found_nodes
 
+    def _uncertain_nodes_nonlocal(
+        self,
+        found_nodes: list[nodes.NodeNg],
+        node: nodes.Name,
+        visited_nonlocal_nodes: list[list[nodes.Nonlocal]],
+    ) -> list[nodes.NodeNg]:
+        """Return found nodes that can be excluded for checks for cases
+        where `node` is already defined by a nonlocal statement.
+        """
+        uncertain_nodes = []
+        node_frame = node.frame()
+
+        for found_node in found_nodes:
+            # Determine the frame for the current found node
+            found_node_frame = (
+                found_node.parent.frame()
+                if isinstance(found_node, nodes.FunctionDef)
+                else found_node.frame()
+            )
+
+            # If both nodes in same frame, check for nonlocal definition in their frame
+            # Else, check for nonlocal definitions in visited frames
+            if any(
+                node.name in nonlocal_node.names and _is_before(nonlocal_node, node)
+                for scope in visited_nonlocal_nodes
+                if (found_node_frame != node_frame)
+                or (
+                    found_node_frame == node_frame
+                    and (scope and scope[0].frame() == node_frame)
+                )
+                for nonlocal_node in scope
+            ):
+                uncertain_nodes.append(found_node)
+
+        return uncertain_nodes
+
     def _inferred_to_define_name_raise_or_return(
         self,
         name: str,
@@ -1268,6 +1325,7 @@ def __init__(self, linter: PyLinter) -> None:
         self._reported_type_checking_usage_scopes: dict[
             str, list[nodes.LocalsDictNodeNG]
         ] = {}
+        self._visited_nonlocal_nodes: list[list[nodes.Nonlocal]] = []
         self._postponed_evaluation_enabled = False
 
     @utils.only_required_for_messages(
@@ -1434,6 +1492,9 @@ def leave_setcomp(self, _: nodes.SetComp) -> None:
     def visit_functiondef(self, node: nodes.FunctionDef) -> None:
         """Visit function: update consumption analysis variable and check locals."""
         self._to_consume.append(NamesConsumer(node, "function"))
+        self._visited_nonlocal_nodes.append(
+            [n for n in node.body if isinstance(n, nodes.Nonlocal)]
+        )
         if not (
             self.linter.is_message_enabled("redefined-outer-name")
             or self.linter.is_message_enabled("redefined-builtin")
@@ -1483,6 +1544,7 @@ def visit_functiondef(self, node: nodes.FunctionDef) -> None:
     def leave_functiondef(self, node: nodes.FunctionDef) -> None:
         """Leave function: check function's locals are consumed."""
         self._check_metaclasses(node)
+        self._visited_nonlocal_nodes.pop()
 
         if node.type_comment_returns:
             self._store_type_annotation_node(node.type_comment_returns)
@@ -1761,7 +1823,9 @@ def _check_consumer(
                 self._check_late_binding_closure(node)
                 return (VariableVisitConsumerAction.RETURN, None)
 
-        found_nodes = current_consumer.get_next_to_consume(node)
+        found_nodes = current_consumer.get_next_to_consume(
+            node, self._visited_nonlocal_nodes
+        )
         if found_nodes is None:
             return (VariableVisitConsumerAction.CONTINUE, None)
         if not found_nodes:
@@ -1964,6 +2028,8 @@ def _report_unfound_name_definition(
             and node.scope() in self._reported_type_checking_usage_scopes[node.name]
         ):
             return False
+        if current_consumer.defined_by_nonlocal:
+            return False
 
         confidence = HIGH
         if node.name in current_consumer.names_under_always_false_test:
@@ -2291,19 +2357,11 @@ def _is_variable_violation(
                     # x = b if (b := True) else False
                     maybe_before_assign = False
                 elif (
-                    isinstance(  # pylint: disable=too-many-boolean-expressions
-                        defnode, nodes.NamedExpr
-                    )
+                    isinstance(defnode, nodes.NamedExpr)
                     and frame is defframe
                     and defframe.parent_of(stmt)
                     and stmt is defstmt
-                    and (
-                        (
-                            defnode.lineno == node.lineno
-                            and defnode.col_offset < node.col_offset
-                        )
-                        or (defnode.lineno < node.lineno)
-                    )
+                    and _is_before(defnode, node)
                 ):
                     # Relation of a name to the same name in a named expression
                     # Could be used before assignment if self-referencing: