Fix false negative for used-before-assignment (ExceptHandler) (#4791)

* Fix false negative for used-before-assignment (ExceptHandler)

Closes #626.

* Fix unused-variable check for exception variables

ChangeLog

@@ -75,6 +75,11 @@ Release date: TBA
   Closes #3608
   Closes #4346
 
+* Fix false negative for ``used-before-assignment`` when the variable is assigned
+  in an exception handler, but used outside of the handler.
+
+  Closes #626
+
 
 What's New in Pylint 2.9.6?
 ===========================

doc/whatsnew/2.10.rst

@@ -61,3 +61,8 @@ Other Changes
   Closes #3249
   Closes #3608
   Closes #4346
+
+* Fix false negative for ``used-before-assignment`` when the variable is assigned
+  in an exception handler, but used outside of the handler.
+
+  Closes #626

pylint/checkers/variables.py

@@ -545,36 +545,55 @@ def consumed(self):
     def scope_type(self):
         return self._atomic.scope_type
 
-    def mark_as_consumed(self, name, new_node):
+    def mark_as_consumed(self, name, consumed_nodes):
         """
-        Mark the name as consumed and delete it from
+        Mark the given nodes as consumed for the name.
+        If all of the nodes for the name were consumed, delete the name from
         the to_consume dictionary
         """
-        self.consumed[name] = new_node
-        del self.to_consume[name]
+        unconsumed = [n for n in self.to_consume[name] if n not in set(consumed_nodes)]
+        self.consumed[name] = consumed_nodes
+
+        if unconsumed:
+            self.to_consume[name] = unconsumed
+        else:
+            del self.to_consume[name]
 
     def get_next_to_consume(self, node):
-        # Get the definition of `node` from this scope
+        """
+        Return a list of the nodes that define `node` from this scope.
+        Return None to indicate a special case that needs to be handled by the caller.
+        """
         name = node.name
         parent_node = node.parent
-        found_node = self.to_consume.get(name)
+        found_nodes = self.to_consume.get(name)
         if (
-            found_node
+            found_nodes
             and isinstance(parent_node, astroid.Assign)
-            and parent_node == found_node[0].parent
+            and parent_node == found_nodes[0].parent
         ):
-            lhs = found_node[0].parent.targets[0]
+            lhs = found_nodes[0].parent.targets[0]
             if lhs.name == name:  # this name is defined in this very statement
-                found_node = None
+                found_nodes = None
 
         if (
-            found_node
+            found_nodes
             and isinstance(parent_node, astroid.For)
             and parent_node.iter == node
-            and parent_node.target in found_node
+            and parent_node.target in found_nodes
         ):
-            found_node = None
-        return found_node
+            found_nodes = None
+
+        # Filter out assignments in ExceptHandlers that node is not contained in
+        if found_nodes:
+            found_nodes = [
+                n
+                for n in found_nodes
+                if not isinstance(n.statement(), astroid.ExceptHandler)
+                or n.statement().parent_of(node)
+            ]
+
+        return found_nodes
 
 
 # pylint: disable=too-many-public-methods
@@ -943,6 +962,7 @@ def visit_assignname(self, node):
     def visit_delname(self, node):
         self.visit_name(node)
 
+    # pylint: disable=too-many-branches
     def visit_name(self, node):
         """Check that a name is defined in the current scope"""
         stmt = node.statement()
@@ -1019,12 +1039,17 @@ def visit_name(self, node):
                 self._loopvar_name(node, name)
                 break
 
-            found_node = current_consumer.get_next_to_consume(node)
-            if found_node is None:
+            found_nodes = current_consumer.get_next_to_consume(node)
+            if found_nodes is None:
                 continue
 
             # checks for use before assignment
-            defnode = utils.assign_parent(current_consumer.to_consume[name][0])
+            if found_nodes:
+                defnode = utils.assign_parent(found_nodes[0])
+            else:
+                defnode = None
+                if used_before_assignment_is_enabled:
+                    self.add_message("used-before-assignment", args=name, node=node)
 
             if (
                 undefined_variable_is_enabled or used_before_assignment_is_enabled
@@ -1156,7 +1181,7 @@ def visit_name(self, node):
                         elif current_consumer.scope_type == "lambda":
                             self.add_message("undefined-variable", node=node, args=name)
 
-            current_consumer.mark_as_consumed(name, found_node)
+            current_consumer.mark_as_consumed(name, found_nodes)
             # check it's not a loop variable used outside the loop
             self._loopvar_name(node, name)
             break
@@ -1716,6 +1741,12 @@ def _check_is_unused(self, name, node, stmt, global_names, nonlocal_names):
             if utils.is_overload_stub(node):
                 return
 
+            # Special case for exception variable
+            if isinstance(stmt.parent, astroid.ExceptHandler) and any(
+                n.name == name for n in stmt.parent.nodes_of_class(astroid.Name)
+            ):
+                return
+
             self.add_message(message_name, args=name, node=stmt)
 
     def _is_name_ignored(self, stmt, name):

tests/functional/u/unused/unused_variable.py

@@ -144,8 +144,7 @@ def func3():
         print(f"{error}")
         try:
             1 / 2
-        except TypeError as error:
-        # TODO fix bug for not identifying unused variables in nested exceptions see issue #4391
+        except TypeError as error:  # [unused-variable]
             print("warning")
 
 def func4():

tests/functional/u/unused/unused_variable.txt

@@ -19,4 +19,5 @@ unused-import:104:4:test_global:Unused version imported from sys as VERSION
 unused-import:105:4:test_global:Unused import this
 unused-import:106:4:test_global:Unused re imported as RE
 unused-variable:110:4:function2:Unused variable 'unused'
-unused-variable:154:4:func4:Unused variable 'error'
+unused-variable:147:8:func3:Unused variable 'error'
+unused-variable:153:4:func4:Unused variable 'error'

tests/functional/u/use/used_before_assignment_issue626.py

@@ -0,0 +1,44 @@
+# pylint: disable=missing-docstring,invalid-name
+def main1():
+    try:
+        raise ValueError
+    except ValueError as e:  # [unused-variable]
+        pass
+
+    print(e)  # [used-before-assignment]
+
+
+def main2():
+    try:
+        raise ValueError
+    except ValueError as e:
+        print(e)
+
+
+def main3():
+    try:
+        raise ValueError
+    except ValueError as e:  # [unused-variable]
+        pass
+
+    e = 10
+    print(e)
+
+
+def main4():
+    try:
+        raise ValueError
+    except ValueError as e:  # [unused-variable]
+        pass
+
+    try:
+        raise ValueError
+    except ValueError as e:
+        pass
+
+    try:
+        raise ValueError
+    except ValueError as e:
+        pass
+
+    print(e)  # [used-before-assignment]

tests/functional/u/use/used_before_assignment_issue626.txt

@@ -0,0 +1,5 @@
+unused-variable:5:4:main1:Unused variable 'e'
+used-before-assignment:8:10:main1:Using variable 'e' before assignment
+unused-variable:21:4:main3:Unused variable 'e'
+unused-variable:31:4:main4:Unused variable 'e'
+used-before-assignment:44:10:main4:Using variable 'e' before assignment