Fix #5683: False positive used-before-assignment in loop else where the only non-break exit is via except (#5684)

jacobtylerwalls · web-flow · commit a8f81db85ebf · 2022-01-27T16:58:43.000+01:00
diff --git a/ChangeLog b/ChangeLog
@@ -157,6 +157,13 @@ Release date: TBA
 
   Closes #5500
 
+* When evaluating statements in the ``else`` clause of a loop, ``used-before-assignment``
+  assumes that assignments in the except blocks took place if the
+  except handlers constituted the only ways for the loop to finish without
+  breaking early.
+
+  Closes #5683
+
 * ``used-before-assignment`` now checks names in try blocks.
 
 * Fixed false positive with ``used-before-assignment`` for assignment expressions
diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst
@@ -205,6 +205,13 @@ Other Changes
 
   Closes #5500
 
+* When evaluating statements in the ``else`` clause of a loop, ``used-before-assignment``
+  assumes that assignments in the except blocks took place if the
+  except handlers constituted the only ways for the loop to finish without
+  breaking early.
+
+  Closes #5683
+
 * ``used-before-assignment`` now checks names in try blocks.
 
 * Fixed false positive with ``used-before-assignment`` for assignment expressions
diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
@@ -741,10 +741,108 @@ def _uncertain_nodes_in_except_blocks(
                     # if one of the except blocks does not define the name in question,
                     # raise, or return. See: https://github.com/PyCQA/pylint/issues/5524.
                     continue
+
+            if NamesConsumer._check_loop_finishes_via_except(
+                node, other_node_statement.parent.parent
+            ):
+                continue
+
             # Passed all tests for uncertain execution
             uncertain_nodes.append(other_node)
         return uncertain_nodes
 
+    @staticmethod
+    def _check_loop_finishes_via_except(
+        node: nodes.NodeNG, other_node_try_except: nodes.TryExcept
+    ) -> bool:
+        """Check for a case described in https://github.com/PyCQA/pylint/issues/5683.
+        It consists of a specific control flow scenario where the only
+        non-break exit from a loop consists of the very except handler we are
+        examining, such that code in the `else` branch of the loop can depend on it
+        being assigned.
+
+        Example:
+
+        for _ in range(3):
+            try:
+                do_something()
+            except:
+                name = 1  <-- only non-break exit from loop
+            else:
+                break
+        else:
+            print(name)
+        """
+        if not other_node_try_except.orelse:
+            return False
+        closest_loop: Optional[
+            Union[nodes.For, nodes.While]
+        ] = utils.get_node_first_ancestor_of_type(node, (nodes.For, nodes.While))
+        if closest_loop is None:
+            return False
+        if not any(
+            else_statement is node or else_statement.parent_of(node)
+            for else_statement in closest_loop.orelse
+        ):
+            # `node` not guarded by `else`
+            return False
+        for inner_else_statement in other_node_try_except.orelse:
+            if isinstance(inner_else_statement, nodes.Break):
+                break_stmt = inner_else_statement
+                break
+        else:
+            # No break statement
+            return False
+
+        def _try_in_loop_body(
+            other_node_try_except: nodes.TryExcept, loop: Union[nodes.For, nodes.While]
+        ) -> bool:
+            """Return True if `other_node_try_except` is a descendant of `loop`."""
+            return any(
+                loop_body_statement is other_node_try_except
+                or loop_body_statement.parent_of(other_node_try_except)
+                for loop_body_statement in loop.body
+            )
+
+        if not _try_in_loop_body(other_node_try_except, closest_loop):
+            for ancestor in closest_loop.node_ancestors():
+                if isinstance(ancestor, (nodes.For, nodes.While)):
+                    if _try_in_loop_body(other_node_try_except, ancestor):
+                        break
+            else:
+                # `other_node_try_except` didn't have a shared ancestor loop
+                return False
+
+        for loop_stmt in closest_loop.body:
+            if NamesConsumer._recursive_search_for_continue_before_break(
+                loop_stmt, break_stmt
+            ):
+                break
+        else:
+            # No continue found, so we arrived at our special case!
+            return True
+        return False
+
+    @staticmethod
+    def _recursive_search_for_continue_before_break(
+        stmt: nodes.Statement, break_stmt: nodes.Break
+    ) -> bool:
+        """Return True if any Continue node can be found in descendants of `stmt`
+        before encountering `break_stmt`, ignoring any nested loops.
+        """
+        if stmt is break_stmt:
+            return False
+        if isinstance(stmt, nodes.Continue):
+            return True
+        for child in stmt.get_children():
+            if isinstance(stmt, (nodes.For, nodes.While)):
+                continue
+            if NamesConsumer._recursive_search_for_continue_before_break(
+                child, break_stmt
+            ):
+                return True
+        return False
+
     @staticmethod
     def _uncertain_nodes_in_try_blocks_when_evaluating_except_blocks(
         found_nodes: List[nodes.NodeNG], node_statement: nodes.Statement
diff --git a/tests/functional/u/use/used_before_assignment_issue4761.py b/tests/functional/u/use/used_before_assignment_issue4761.py
@@ -10,3 +10,169 @@ def function():
         return 1
 
     return some_message
+
+
+# Cases related to a specific control flow where
+# the `else` of a loop can depend on a name only defined
+# in a single except handler because that except handler is the
+# only non-break exit branch.
+
+def valid_only_non_break_exit_from_loop_is_except_handler():
+    """https://github.com/PyCQA/pylint/issues/5683"""
+    for _ in range(3):
+        try:
+            function()  # not an exit branch because of `else` below
+        except ValueError as verr:
+            error = verr  # < exit branch where error is defined
+        else:
+            break  # < exit condition where error is *not* defined
+            # will skip else: raise error
+        print("retrying...")
+    else:
+        # This usage is valid because there is only one exit branch
+        raise error
+
+
+def invalid_no_outer_else():
+    """The reliance on the name is not guarded by else."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+        print("retrying...")
+    raise error  # [used-before-assignment]
+
+
+def invalid_no_outer_else_2():
+    """Same, but the raise is inside a loop."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+        raise error  # [used-before-assignment]
+
+
+def invalid_no_inner_else():
+    """No inner else statement."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        print("retrying...")
+        if function():
+            break
+    else:
+        raise error  # [used-before-assignment]
+
+
+def invalid_wrong_break_location():
+    """The break is in the wrong location."""
+    for _ in range(3):
+        try:
+            function()
+            break
+        except ValueError as verr:
+            error = verr
+            print("I give up")
+    else:
+        raise error  # [used-before-assignment]
+
+
+def invalid_no_break():
+    """No break."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        else:
+            pass
+    else:  # pylint: disable=useless-else-on-loop
+        raise error  # [used-before-assignment]
+
+
+def invalid_other_non_break_exit_from_loop_besides_except_handler():
+    """The continue creates another exit branch."""
+    while function():
+        if function():
+            continue
+        try:
+            pass
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+    else:
+        raise error  # [used-before-assignment]
+
+
+def valid_continue_does_not_matter():
+    """This continue doesn't matter: still just one exit branch."""
+    while function():
+        try:
+            for _ in range(3):
+                if function():
+                    continue
+                print(1 / 0)
+        except ZeroDivisionError as zde:
+            error = zde
+        else:
+            break
+    else:
+        raise error
+
+
+def invalid_conditional_continue_after_break():
+    """The continue is another exit branch"""
+    while function():
+        try:
+            if function():
+                break
+            if not function():
+                continue
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+    else:
+        raise error  # [used-before-assignment]
+
+
+def invalid_unrelated_loops():
+    """The loop else in question is not related to the try/except/else."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+    while function():
+        print('The time is:')
+        break
+    else:
+        raise error  # [used-before-assignment]
+
+
+def valid_nested_loops():
+    """The name `error` is still available in a nested else."""
+    for _ in range(3):
+        try:
+            function()
+        except ValueError as verr:
+            error = verr
+        else:
+            break
+    else:
+        while function():
+            print('The time is:')
+            break
+        else:
+            raise error
diff --git a/tests/functional/u/use/used_before_assignment_issue4761.txt b/tests/functional/u/use/used_before_assignment_issue4761.txt
@@ -1 +1,9 @@
 used-before-assignment:9:11:9:23:function:Using variable 'some_message' before assignment:CONTROL_FLOW
+used-before-assignment:46:10:46:15:invalid_no_outer_else:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:58:14:58:19:invalid_no_outer_else_2:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:72:14:72:19:invalid_no_inner_else:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:85:14:85:19:invalid_wrong_break_location:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:98:14:98:19:invalid_no_break:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:113:14:113:19:invalid_other_non_break_exit_from_loop_besides_except_handler:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:145:14:145:19:invalid_conditional_continue_after_break:Using variable 'error' before assignment:CONTROL_FLOW
+used-before-assignment:161:14:161:19:invalid_unrelated_loops:Using variable 'error' before assignment:CONTROL_FLOW
