Fix #4761: Emit `used-before-assignment` where single assignment only made in except blocks (#5402)

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

9 files changed, 97 insertions, 13 deletions

diff --git a/ChangeLog b/ChangeLog
index 8ddac4258..22c918048 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -11,6 +11,13 @@ Release date: TBA
 ..
   Put new features here and also in 'doc/whatsnew/2.13.rst'
 
+* ``used-before-assignment`` now assumes that assignments in except blocks
+  may not have occurred and warns accordingly.
+
+  Closes #4761
+
+* ``used-before-assignment`` now checks names in try blocks.
+
 * Some files in ``pylint.testutils`` were deprecated. In the future imports should be done from the
   ``pylint.testutils.functional`` namespace directly.
 
@@ -18,6 +25,9 @@ Release date: TBA
 
   Closes #4716
 
+* The ``PyLinter`` class will now be initialiazed with a ``TextReporter``
+  as its reporter if none is provided.
+
 * Fix false positive - Allow unpacking of ``self`` in a subclass of ``typing.NamedTuple``.
 
   Closes #5312
@@ -36,9 +46,6 @@ Release date: TBA
   Insert your changelog randomly, it will reduce merge conflicts
   (Ie. not necessarily at the end)
 
-* The ``PyLinter`` class will now be initialiazed with a ``TextReporter``
-  as its reporter if none is provided.
-
 
 What's New in Pylint 2.12.2?
 ============================
diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst
index 0713e343a..55535638e 100644
--- a/doc/whatsnew/2.13.rst
+++ b/doc/whatsnew/2.13.rst
@@ -31,6 +31,13 @@ Other Changes
 
   Closes #5323
 
+* ``used-before-assignment`` now assumes that assignments in except blocks
+  may not have occurred and warns accordingly.
+
+  Closes #4761
+
+* ``used-before-assignment`` now checks names in try blocks.
+
 * Require Python ``3.6.2`` to run pylint.
 
   Closes #5065
diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index f2bba355c..13b8589af 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -534,7 +534,7 @@ MSGS = {
 
 
 ScopeConsumer = collections.namedtuple(
-    "ScopeConsumer", "to_consume consumed scope_type"
+    "ScopeConsumer", "to_consume consumed consumed_uncertain scope_type"
 )
 
 
@@ -544,17 +544,24 @@ class NamesConsumer:
     """
 
     def __init__(self, node, scope_type):
-        self._atomic = ScopeConsumer(copy.copy(node.locals), {}, scope_type)
+        self._atomic = ScopeConsumer(
+            copy.copy(node.locals), {}, collections.defaultdict(list), scope_type
+        )
         self.node = node
 
     def __repr__(self):
         to_consumes = [f"{k}->{v}" for k, v in self._atomic.to_consume.items()]
         consumed = [f"{k}->{v}" for k, v in self._atomic.consumed.items()]
+        consumed_uncertain = [
+            f"{k}->{v}" for k, v in self._atomic.consumed_uncertain.items()
+        ]
         to_consumes = ", ".join(to_consumes)
         consumed = ", ".join(consumed)
+        consumed_uncertain = ", ".join(consumed_uncertain)
         return f"""
 to_consume : {to_consumes}
 consumed : {consumed}
+consumed_uncertain: {consumed_uncertain}
 scope_type : {self._atomic.scope_type}
 """
 
@@ -570,6 +577,19 @@ scope_type : {self._atomic.scope_type}
         return self._atomic.consumed
 
     @property
+    def consumed_uncertain(self) -> DefaultDict[str, List[nodes.NodeNG]]:
+        """
+        Retrieves nodes filtered out by get_next_to_consume() that may not
+        have executed, such as statements in except blocks. Checkers that
+        want to treat the statements as executed (e.g. for unused-variable)
+        may need to add them back.
+
+        TODO: A pending PR will extend this to nodes in try blocks when
+        evaluating their corresponding except and finally blocks.
+        """
+        return self._atomic.consumed_uncertain
+
+    @property
     def scope_type(self):
         return self._atomic.scope_type
 
@@ -589,7 +609,9 @@ scope_type : {self._atomic.scope_type}
 
     def get_next_to_consume(self, node):
         """
-        Return a list of the nodes that define `node` from this scope.
+        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
+        except blocks, add it to self.consumed_uncertain instead of returning it.
         Return None to indicate a special case that needs to be handled by the caller.
         """
         name = node.name
@@ -617,9 +639,28 @@ scope_type : {self._atomic.scope_type}
             found_nodes = [
                 n
                 for n in found_nodes
-                if not isinstance(n.statement(), nodes.ExceptHandler)
-                or n.statement().parent_of(node)
+                if not isinstance(n.statement(future=True), nodes.ExceptHandler)
+                or n.statement(future=True).parent_of(node)
+            ]
+
+        # Filter out assignments in an Except clause that the node is not
+        # contained in, assuming they may fail
+        if found_nodes:
+            filtered_nodes = [
+                n
+                for n in found_nodes
+                if not (
+                    isinstance(n.statement(future=True).parent, nodes.ExceptHandler)
+                    and isinstance(
+                        n.statement(future=True).parent.parent, nodes.TryExcept
+                    )
+                )
+                or n.statement(future=True).parent.parent_of(node)
             ]
+            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
 
         return found_nodes
 
@@ -1042,6 +1083,14 @@ class VariablesChecker(BaseChecker):
             if action is VariableVisitConsumerAction.CONTINUE:
                 continue
             if action is VariableVisitConsumerAction.CONSUME:
+                # pylint: disable-next=fixme
+                # TODO: remove assert after _check_consumer return value better typed
+                assert found_nodes is not None, "Cannot consume an empty list of nodes."
+                # Any nodes added to consumed_uncertain by get_next_to_consume()
+                # should be added back so that they are marked as used.
+                # They will have already had a chance to emit used-before-assignment.
+                # We check here instead of before every single return in _check_consumer()
+                found_nodes += current_consumer.consumed_uncertain[node.name]
                 current_consumer.mark_as_consumed(node.name, found_nodes)
             if action in {
                 VariableVisitConsumerAction.RETURN,
@@ -1135,6 +1184,12 @@ class VariablesChecker(BaseChecker):
             return (VariableVisitConsumerAction.CONTINUE, None)
         if not found_nodes:
             self.add_message("used-before-assignment", args=node.name, node=node)
+            if current_consumer.consumed_uncertain[node.name]:
+                # If there are nodes added to consumed_uncertain by
+                # get_next_to_consume() because they might not have executed,
+                # return a CONSUME action so that _undefined_and_used_before_checker()
+                # will mark them as used
+                return (VariableVisitConsumerAction.CONSUME, found_nodes)
             return (VariableVisitConsumerAction.RETURN, found_nodes)
 
         self._check_late_binding_closure(node)
@@ -2370,7 +2425,7 @@ class VariablesChecker(BaseChecker):
         name = METACLASS_NAME_TRANSFORMS.get(name, name)
         if name:
             # check enclosing scopes starting from most local
-            for scope_locals, _, _ in self._to_consume[::-1]:
+            for scope_locals, _, _, _ in self._to_consume[::-1]:
                 found_nodes = scope_locals.get(name, [])
                 for found_node in found_nodes:
                     if found_node.lineno <= klass.lineno:
diff --git a/pylintrc b/pylintrc
index a38a35aed..7e983510e 100644
--- a/pylintrc
+++ b/pylintrc
@@ -332,7 +332,7 @@ max-locals=25
 max-returns=11
 
 # Maximum number of branch for function / method body
-max-branches=26
+max-branches=27
 
 # Maximum number of statements in function / method body
 max-statements=100
diff --git a/tests/functional/u/undefined/undefined_variable.py b/tests/functional/u/undefined/undefined_variable.py
index f4f22e523..6ce9aaa6e 100644
--- a/tests/functional/u/undefined/undefined_variable.py
+++ b/tests/functional/u/undefined/undefined_variable.py
@@ -35,7 +35,7 @@ LMBD = lambda x, y=doesnotexist: x+y  # [undefined-variable]
 LMBD2 = lambda x, y: x+z  # [undefined-variable]
 
 try:
-    POUET # don't catch me
+    POUET # [used-before-assignment]
 except NameError:
     POUET = 'something'
 
@@ -45,7 +45,7 @@ except Exception: # pylint:disable = broad-except
     POUETT = 'something'
 
 try:
-    POUETTT # don't catch me
+    POUETTT # [used-before-assignment]
 except: # pylint:disable = bare-except
     POUETTT = 'something'
 
diff --git a/tests/functional/u/undefined/undefined_variable.txt b/tests/functional/u/undefined/undefined_variable.txt
index c5a70f7a7..bee5ab475 100644
--- a/tests/functional/u/undefined/undefined_variable.txt
+++ b/tests/functional/u/undefined/undefined_variable.txt
@@ -8,7 +8,9 @@ undefined-variable:31:4:31:10:bad_default:Undefined variable 'augvar':UNDEFINED
 undefined-variable:32:8:32:14:bad_default:Undefined variable 'vardel':UNDEFINED
 undefined-variable:34:19:34:31:<lambda>:Undefined variable 'doesnotexist':UNDEFINED
 undefined-variable:35:23:35:24:<lambda>:Undefined variable 'z':UNDEFINED
+used-before-assignment:38:4:38:9::Using variable 'POUET' before assignment:UNDEFINED
 used-before-assignment:43:4:43:10::Using variable 'POUETT' before assignment:UNDEFINED
+used-before-assignment:48:4:48:11::Using variable 'POUETTT' before assignment:UNDEFINED
 used-before-assignment:56:4:56:9::Using variable 'PLOUF' before assignment:UNDEFINED
 used-before-assignment:65:11:65:14:if_branch_test:Using variable 'xxx' before assignment:UNDEFINED
 used-before-assignment:91:23:91:32:test_arguments:Using variable 'TestClass' before assignment:UNDEFINED
diff --git a/tests/functional/u/use/used_before_assignment_issue4761.py b/tests/functional/u/use/used_before_assignment_issue4761.py
new file mode 100644
index 000000000..ab6fc765b
--- /dev/null
+++ b/tests/functional/u/use/used_before_assignment_issue4761.py
@@ -0,0 +1,12 @@
+"""used-before-assignment (E0601)"""
+def function():
+    """Consider that except blocks may not execute."""
+    try:
+        pass
+    except ValueError:
+        some_message = 'some message'
+
+    if not some_message:  # [used-before-assignment]
+        return 1
+
+    return some_message
diff --git a/tests/functional/u/use/used_before_assignment_issue4761.txt b/tests/functional/u/use/used_before_assignment_issue4761.txt
new file mode 100644
index 000000000..cf8fd209a
--- /dev/null
+++ b/tests/functional/u/use/used_before_assignment_issue4761.txt
@@ -0,0 +1 @@
+used-before-assignment:9:11:9:23:function:Using variable 'some_message' before assignment:UNDEFINED
diff --git a/tests/lint/test_utils.py b/tests/lint/test_utils.py
index f7294f353..44703982e 100644
--- a/tests/lint/test_utils.py
+++ b/tests/lint/test_utils.py
@@ -15,7 +15,7 @@ def test_prepare_crash_report(tmp_path: PosixPath) -> None:
         template_path = prepare_crash_report(
             ex, str(python_file), str(tmp_path / "pylint-crash-%Y.txt")
         )
-    assert str(tmp_path) in str(template_path)
+    assert str(tmp_path) in str(template_path)  # pylint: disable=used-before-assignment
     with open(template_path, encoding="utf8") as f:
         template_content = f.read()
     assert python_content in template_content