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

Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>

Author: 3 people

ChangeLog

@@ -11,13 +11,23 @@ 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.
 
 * Fix ``unnecessary_dict_index_lookup`` false positive when deleting a dictionary's entry.
 
   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?
 ============================

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

pylint/checkers/variables.py

@@ -534,7 +534,7 @@ def _has_locals_call_after_node(stmt, scope):
 
 
 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}
 """
 
@@ -569,6 +576,19 @@ def to_consume(self):
     def consumed(self):
         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 @@ def mark_as_consumed(self, name, consumed_nodes):
 
     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 @@ def get_next_to_consume(self, node):
             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 @@ def _undefined_and_used_before_checker(
             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 @@ def _check_consumer(
             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 @@ def _check_classdef_metaclasses(self, klass, parent_node):
         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:

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

tests/functional/u/undefined/undefined_variable.py

@@ -35,7 +35,7 @@ def bad_default(var, default=unknown2):  # [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 @@ def bad_default(var, default=unknown2):  # [undefined-variable]
     POUETT = 'something'
 
 try:
-    POUETTT # don't catch me
+    POUETTT # [used-before-assignment]
 except: # pylint:disable = bare-except
     POUETTT = 'something'
 

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

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

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

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