Emit possibly-used-before-assignment after if/else switches (#8952)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Author: jacobtylerwalls

doc/data/messages/p/possibly-used-before-assignment/bad.py

@@ -0,0 +1,4 @@
+def check_lunchbox(items: list[str]):
+    if not items:
+        empty = True
+    print(empty)  # [possibly-used-before-assignment]

doc/data/messages/p/possibly-used-before-assignment/details.rst

@@ -0,0 +1,15 @@
+If you rely on a pattern like:
+
+.. sourcecode:: python
+
+    if guarded():
+        var = 1
+
+    if guarded():
+        print(var)  # emits possibly-used-before-assignment
+
+you may be concerned that ``possibly-used-before-assignment`` is not totally useful
+in this instance. However, consider that pylint, as a static analysis tool, does
+not know if ``guarded()`` is deterministic or talks to
+a database. (Likewise, for ``guarded`` instead of ``guarded()``, any other
+part of your program may have changed its value in the meantime.)

doc/data/messages/p/possibly-used-before-assignment/good.py

@@ -0,0 +1,5 @@
+def check_lunchbox(items: list[str]):
+    empty = False
+    if not items:
+        empty = True
+    print(empty)

doc/user_guide/checkers/features.rst

@@ -1374,6 +1374,9 @@ Variables checker Messages
   Used when an invalid (non-string) object occurs in __all__.
 :no-name-in-module (E0611): *No name %r in module %r*
   Used when a name cannot be found in a module.
+:possibly-used-before-assignment (E0606): *Possibly using variable %r before assignment*
+  Emitted when a local variable is accessed before its assignment took place in
+  both branches of an if/else switch.
 :undefined-variable (E0602): *Undefined variable %r*
   Used when an undefined variable is accessed.
 :undefined-all-variable (E0603): *Undefined variable name %r in __all__*

doc/user_guide/messages/messages_overview.rst

@@ -139,6 +139,7 @@ All messages in the error category:
    error/not-in-loop
    error/notimplemented-raised
    error/positional-only-arguments-expected
+   error/possibly-used-before-assignment
    error/potential-index-error
    error/raising-bad-type
    error/raising-non-exception

doc/whatsnew/fragments/1727.new_check

@@ -0,0 +1,4 @@
+Add check ``possibly-used-before-assignment`` when relying on names after an ``if/else``
+switch when one branch failed to define the name, raise, or return.
+
+Closes #1727

pylint/checkers/variables.py

@@ -403,6 +403,12 @@ def _has_locals_call_after_node(stmt: nodes.NodeNG, scope: nodes.FunctionDef) ->
         "invalid-all-format",
         "Used when __all__ has an invalid format.",
     ),
+    "E0606": (
+        "Possibly using variable %r before assignment",
+        "possibly-used-before-assignment",
+        "Emitted when a local variable is accessed before its assignment took place "
+        "in both branches of an if/else switch.",
+    ),
     "E0611": (
         "No name %r in module %r",
         "no-name-in-module",
@@ -537,6 +543,8 @@ def __init__(self, node: nodes.NodeNG, scope_type: str) -> None:
             copy.copy(node.locals), {}, collections.defaultdict(list), scope_type
         )
         self.node = node
+        self.names_under_always_false_test: set[str] = set()
+        self.names_defined_under_one_branch_only: set[str] = set()
 
     def __repr__(self) -> str:
         _to_consumes = [f"{k}->{v}" for k, v in self._atomic.to_consume.items()]
@@ -636,13 +644,6 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
         if VariablesChecker._comprehension_between_frame_and_node(node):
             return found_nodes
 
-        # Filter out assignments guarded by always false conditions
-        if found_nodes:
-            uncertain_nodes = self._uncertain_nodes_in_false_tests(found_nodes, node)
-            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]
-
         # Filter out assignments in ExceptHandlers that node is not contained in
         if found_nodes:
             found_nodes = [
@@ -652,6 +653,13 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
                 or n.statement().parent_of(node)
             ]
 
+        # Filter out assignments guarded by always false conditions
+        if found_nodes:
+            uncertain_nodes = self._uncertain_nodes_if_tests(found_nodes, node)
+            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]
+
         # Filter out assignments in an Except clause that the node is not
         # contained in, assuming they may fail
         if found_nodes:
@@ -688,8 +696,9 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
 
         return found_nodes
 
-    @staticmethod
-    def _inferred_to_define_name_raise_or_return(name: str, node: nodes.NodeNG) -> bool:
+    def _inferred_to_define_name_raise_or_return(
+        self, name: str, node: nodes.NodeNG
+    ) -> bool:
         """Return True if there is a path under this `if_node`
         that is inferred to define `name`, raise, or return.
         """
@@ -716,8 +725,8 @@ def _inferred_to_define_name_raise_or_return(name: str, node: nodes.NodeNG) -> b
         if not isinstance(node, nodes.If):
             return False
 
-        # Be permissive if there is a break
-        if any(node.nodes_of_class(nodes.Break)):
+        # Be permissive if there is a break or a continue
+        if any(node.nodes_of_class(nodes.Break, nodes.Continue)):
             return True
 
         # Is there an assignment in this node itself, e.g. in named expression?
@@ -739,17 +748,18 @@ def _inferred_to_define_name_raise_or_return(name: str, node: nodes.NodeNG) -> b
 
         # Only search else branch when test condition is inferred to be false
         if all_inferred and only_search_else:
-            return NamesConsumer._branch_handles_name(name, node.orelse)
-        # Only search if branch when test condition is inferred to be true
-        if all_inferred and only_search_if:
-            return NamesConsumer._branch_handles_name(name, node.body)
+            self.names_under_always_false_test.add(name)
+            return self._branch_handles_name(name, node.orelse)
         # Search both if and else branches
-        return NamesConsumer._branch_handles_name(
-            name, node.body
-        ) or NamesConsumer._branch_handles_name(name, node.orelse)
-
-    @staticmethod
-    def _branch_handles_name(name: str, body: Iterable[nodes.NodeNG]) -> bool:
+        if_branch_handles = self._branch_handles_name(name, node.body)
+        else_branch_handles = self._branch_handles_name(name, node.orelse)
+        if if_branch_handles ^ else_branch_handles:
+            self.names_defined_under_one_branch_only.add(name)
+        elif name in self.names_defined_under_one_branch_only:
+            self.names_defined_under_one_branch_only.remove(name)
+        return if_branch_handles and else_branch_handles
+
+    def _branch_handles_name(self, name: str, body: Iterable[nodes.NodeNG]) -> bool:
         return any(
             NamesConsumer._defines_name_raises_or_returns(name, if_body_stmt)
             or isinstance(
@@ -762,17 +772,15 @@ def _branch_handles_name(name: str, body: Iterable[nodes.NodeNG]) -> bool:
                     nodes.While,
                 ),
             )
-            and NamesConsumer._inferred_to_define_name_raise_or_return(
-                name, if_body_stmt
-            )
+            and self._inferred_to_define_name_raise_or_return(name, if_body_stmt)
             for if_body_stmt in body
         )
 
-    def _uncertain_nodes_in_false_tests(
+    def _uncertain_nodes_if_tests(
         self, found_nodes: list[nodes.NodeNG], node: nodes.NodeNG
     ) -> list[nodes.NodeNG]:
-        """Identify nodes of uncertain execution because they are defined under
-        tests that evaluate false.
+        """Identify nodes of uncertain execution because they are defined under if
+        tests.
 
         Don't identify a node if there is a path that is inferred to
         define the name, raise, or return (e.g. any executed if/elif/else branch).
@@ -808,7 +816,7 @@ def _uncertain_nodes_in_false_tests(
                 continue
 
             # Name defined in the if/else control flow
-            if NamesConsumer._inferred_to_define_name_raise_or_return(name, outer_if):
+            if self._inferred_to_define_name_raise_or_return(name, outer_if):
                 continue
 
             uncertain_nodes.append(other_node)
@@ -930,7 +938,7 @@ def _uncertain_nodes_in_except_blocks(
 
     @staticmethod
     def _defines_name_raises_or_returns(name: str, node: nodes.NodeNG) -> bool:
-        if isinstance(node, (nodes.Raise, nodes.Assert, nodes.Return)):
+        if isinstance(node, (nodes.Raise, nodes.Assert, nodes.Return, nodes.Continue)):
             return True
         if (
             isinstance(node, nodes.AnnAssign)
@@ -1993,11 +2001,19 @@ def _report_unfound_name_definition(
         ):
             return
 
-        confidence = (
-            CONTROL_FLOW if node.name in current_consumer.consumed_uncertain else HIGH
-        )
+        confidence = HIGH
+        if node.name in current_consumer.names_under_always_false_test:
+            confidence = INFERENCE
+        elif node.name in current_consumer.consumed_uncertain:
+            confidence = CONTROL_FLOW
+
+        if node.name in current_consumer.names_defined_under_one_branch_only:
+            msg = "possibly-used-before-assignment"
+        else:
+            msg = "used-before-assignment"
+
         self.add_message(
-            "used-before-assignment",
+            msg,
             args=node.name,
             node=node,
             confidence=confidence,

pylintrc

@@ -109,6 +109,7 @@ disable=
     # We anticipate #3512 where it will become optional
     fixme,
     consider-using-assignment-expr,
+    possibly-used-before-assignment,
 
 
 [REPORTS]

tests/functional/r/redefined/redefined_except_handler.txt

@@ -1,4 +1,4 @@
 redefined-outer-name:11:4:12:12::Redefining name 'err' from outer scope (line 8):UNDEFINED
 redefined-outer-name:57:8:58:16::Redefining name 'err' from outer scope (line 51):UNDEFINED
-used-before-assignment:69:14:69:29:func:Using variable 'CustomException' before assignment:CONTROL_FLOW
+used-before-assignment:69:14:69:29:func:Using variable 'CustomException' before assignment:HIGH
 redefined-outer-name:71:4:72:12:func:Redefining name 'CustomException' from outer scope (line 62):UNDEFINED

tests/functional/u/undefined/undefined_variable.txt

@@ -27,7 +27,7 @@ undefined-variable:166:4:166:13::Undefined variable 'unicode_2':UNDEFINED
 undefined-variable:171:4:171:13::Undefined variable 'unicode_3':UNDEFINED
 undefined-variable:226:25:226:37:LambdaClass4.<lambda>:Undefined variable 'LambdaClass4':UNDEFINED
 undefined-variable:234:25:234:37:LambdaClass5.<lambda>:Undefined variable 'LambdaClass5':UNDEFINED
-used-before-assignment:255:26:255:34:func_should_fail:Using variable 'datetime' before assignment:CONTROL_FLOW
+used-before-assignment:255:26:255:34:func_should_fail:Using variable 'datetime' before assignment:INFERENCE
 undefined-variable:291:18:291:24:not_using_loop_variable_accordingly:Undefined variable 'iteree':UNDEFINED
 undefined-variable:308:27:308:28:undefined_annotation:Undefined variable 'x':UNDEFINED
 used-before-assignment:309:7:309:8:undefined_annotation:Using variable 'x' before assignment:HIGH