Fix false negative for used-before-assignment when an Except intervenes between Try/Finally (#5583)

Co-authored-by: Pierre Sassoulas <[email protected]>

Author: jacobtylerwalls

ChangeLog

@@ -87,6 +87,10 @@ Release date: TBA
 
   Closes #4045
 
+* Fixed false negative for ``used-before-assignment`` in finally blocks
+  if an except handler did not define the assignment that might have failed
+  in the try block.
+
 * Fixed extremely long processing of long lines with comma's.
 
   Closes #5483

doc/whatsnew/2.13.rst

@@ -150,6 +150,10 @@ Other Changes
 
   Closes #4045
 
+* Fixed false negative for ``used-before-assignment`` in finally blocks
+  if an except handler did not define the assignment that might have failed
+  in the try block.
+
 * Fix a false positive for ``assigning-non-slot`` when the slotted class
   defined ``__setattr__``.
 

pylint/checkers/variables.py

@@ -667,23 +667,15 @@ def get_next_to_consume(self, node: nodes.Name) -> Optional[List[nodes.NodeNG]]:
 
         # If this node is in a Finally block of a Try/Finally,
         # filter out assignments in the try portion, assuming they may fail
-        if (
-            found_nodes
-            and isinstance(node_statement.parent, nodes.TryFinally)
-            and node_statement in node_statement.parent.finalbody
-        ):
-            filtered_nodes = [
-                n
-                for n in found_nodes
-                if not (
-                    n.statement(future=True).parent is node_statement.parent
-                    and n.statement(future=True) in n.statement(future=True).parent.body
+        if found_nodes:
+            uncertain_nodes = (
+                self._uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks(
+                    found_nodes, node_statement
                 )
-            ]
-            filtered_nodes_set = set(filtered_nodes)
-            difference = [n for n in found_nodes if n not in filtered_nodes_set]
-            self.consumed_uncertain[node.name] += difference
-            found_nodes = filtered_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 this node is in an ExceptHandler,
         # filter out assignments in the try portion, assuming they may fail
@@ -794,6 +786,59 @@ def _uncertain_nodes_in_try_blocks_when_evaluating_except_blocks(
             uncertain_nodes.append(other_node)
         return uncertain_nodes
 
+    @staticmethod
+    def _uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks(
+        found_nodes: List[nodes.NodeNG], node_statement: nodes.Statement
+    ) -> List[nodes.NodeNG]:
+        uncertain_nodes: List[nodes.NodeNG] = []
+        (
+            closest_try_finally_ancestor,
+            child_of_closest_try_finally_ancestor,
+        ) = utils.get_node_first_ancestor_of_type_and_its_child(
+            node_statement, nodes.TryFinally
+        )
+        if closest_try_finally_ancestor is None:
+            return uncertain_nodes
+        if (
+            child_of_closest_try_finally_ancestor
+            not in closest_try_finally_ancestor.finalbody
+        ):
+            return uncertain_nodes
+        for other_node in found_nodes:
+            other_node_statement = other_node.statement(future=True)
+            (
+                other_node_try_finally_ancestor,
+                child_of_other_node_try_finally_ancestor,
+            ) = utils.get_node_first_ancestor_of_type_and_its_child(
+                other_node_statement, nodes.TryFinally
+            )
+            if other_node_try_finally_ancestor is None:
+                continue
+            # other_node needs to descend from the try of a try/finally.
+            if (
+                child_of_other_node_try_finally_ancestor
+                not in other_node_try_finally_ancestor.body
+            ):
+                continue
+            # If the two try/finally ancestors are not the same, then
+            # node_statement's closest try/finally ancestor needs to be in
+            # the final body of other_node's try/finally ancestor, or
+            # descend from one of the statements in that final body.
+            if (
+                other_node_try_finally_ancestor is not closest_try_finally_ancestor
+                and not any(
+                    other_node_final_statement is closest_try_finally_ancestor
+                    or other_node_final_statement.parent_of(
+                        closest_try_finally_ancestor
+                    )
+                    for other_node_final_statement in other_node_try_finally_ancestor.finalbody
+                )
+            ):
+                continue
+            # Passed all tests for uncertain execution
+            uncertain_nodes.append(other_node)
+        return uncertain_nodes
+
 
 # pylint: disable=too-many-public-methods
 class VariablesChecker(BaseChecker):

tests/functional/c/consider/consider_using_with_open.py

@@ -137,7 +137,8 @@ def test_defined_in_try_and_finally(self):
         except FileNotFoundError:
             Path("foo").touch()
         finally:
-            file_handle.open("foo", encoding="utf")  # must not trigger
+            # +1: [used-before-assignment]
+            file_handle.open("foo", encoding="utf")  # must not trigger consider-using-with
         with file_handle:
             return file_handle.read()
 

tests/functional/c/consider/consider_using_with_open.txt

@@ -4,3 +4,4 @@ consider-using-with:46:4:46:33:test_open_outside_assignment:Consider using 'with
 consider-using-with:47:14:47:43:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
 consider-using-with:52:8:52:37:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:UNDEFINED
 consider-using-with:120:26:122:13:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:UNDEFINED
+used-before-assignment:141:12:141:23:TestControlFlow.test_defined_in_try_and_finally:Using variable 'file_handle' before assignment:UNDEFINED

tests/functional/u/use/used_before_assignment_issue85.py

@@ -7,3 +7,120 @@ def main():
     finally:
         print(res)  # [used-before-assignment]
     print(res)
+
+
+def try_except_finally():
+    """When evaluating finally blocks, assume try statements fail."""
+    try:
+        res = 1 / 0
+        res = 42
+    except ZeroDivisionError:
+        print()
+    finally:
+        print(res)  # [used-before-assignment]
+    print(res)
+
+
+def try_except_finally_assignment_in_final_block():
+    """Assignment of the name in the final block does not warn."""
+    try:
+        res = 1 / 0
+        res = 42
+    except ZeroDivisionError:
+        print()
+    finally:
+        res = 999
+        print(res)
+    print(res)
+
+
+def try_except_finally_nested_try_finally_in_try():
+    """Don't confuse assignments in different finally statements where
+    one is nested inside a try.
+    """
+    try:
+        try:
+            res = 1 / 0
+        finally:
+            print(res)  # [used-before-assignment]
+        print(1 / 0)
+    except ZeroDivisionError:
+        print()
+    finally:
+        res = 999  # this assignment could be confused for that above
+        print(res)
+    print(res)
+
+
+def try_except_finally_nested_in_finally():
+    """Until Pylint comes to a consensus on requiring all except handlers to
+    define a name, raise, or return (https://github.com/PyCQA/pylint/issues/5524),
+    Pylint assumes statements in try blocks succeed when accessed *after*
+    except or finally blocks and fail when accessed *in* except or finally
+    blocks.)
+    """
+    try:
+        outer_times = 1
+    finally:
+        try:
+            inner_times = 1
+        except TypeError:
+            pass
+        finally:
+            print(outer_times)  # [used-before-assignment]
+        print(inner_times)  # see docstring: might emit in a future version
+
+
+def try_except_finally_nested_in_finally_2():
+    """Neither name is accessed after a finally block."""
+    try:
+        outer_times = 1
+    finally:
+        try:
+            inner_times = 1
+        except TypeError:
+            pass
+        finally:
+            print(inner_times)  # [used-before-assignment]
+        print(outer_times)  # [used-before-assignment]
+
+
+def try_except_finally_nested_in_finally_3():
+    """One name is never accessed after a finally block, but just emit
+    once per name.
+    """
+    try:
+        outer_times = 1
+    finally:
+        try:
+            inner_times = 1
+        except TypeError:
+            pass
+        finally:
+            print(inner_times)  # [used-before-assignment]
+            print(outer_times)  # [used-before-assignment]
+        print(inner_times)
+        # used-before-assignment is only raised once per name
+        print(outer_times)
+
+
+def try_except_finally_nested_in_finally_4():
+    """Triple nesting: don't assume direct parentages of outer try/finally
+    and inner try/finally.
+    """
+    try:
+        outer_times = 1
+    finally:
+        try:
+            pass
+        finally:
+            try:
+                inner_times = 1
+            except TypeError:
+                pass
+            finally:
+                print(inner_times)  # [used-before-assignment]
+                print(outer_times)  # [used-before-assignment]
+            print(inner_times)
+            # used-before-assignment is only raised once per name
+            print(outer_times)

tests/functional/u/use/used_before_assignment_issue85.txt

@@ -1 +1,10 @@
 used-before-assignment:8:14:8:17:main:Using variable 'res' before assignment:UNDEFINED
+used-before-assignment:20:14:20:17:try_except_finally:Using variable 'res' before assignment:UNDEFINED
+used-before-assignment:45:18:45:21:try_except_finally_nested_try_finally_in_try:Using variable 'res' before assignment:UNDEFINED
+used-before-assignment:70:18:70:29:try_except_finally_nested_in_finally:Using variable 'outer_times' before assignment:UNDEFINED
+used-before-assignment:84:18:84:29:try_except_finally_nested_in_finally_2:Using variable 'inner_times' before assignment:UNDEFINED
+used-before-assignment:85:14:85:25:try_except_finally_nested_in_finally_2:Using variable 'outer_times' before assignment:UNDEFINED
+used-before-assignment:100:18:100:29:try_except_finally_nested_in_finally_3:Using variable 'inner_times' before assignment:UNDEFINED
+used-before-assignment:101:18:101:29:try_except_finally_nested_in_finally_3:Using variable 'outer_times' before assignment:UNDEFINED
+used-before-assignment:122:22:122:33:try_except_finally_nested_in_finally_4:Using variable 'inner_times' before assignment:UNDEFINED
+used-before-assignment:123:22:123:33:try_except_finally_nested_in_finally_4:Using variable 'outer_times' before assignment:UNDEFINED