diff --git a/doc/whatsnew/fragments/8437.bugfix b/doc/whatsnew/fragments/8437.bugfix new file mode 100644 index 0000000000..c1727f9508 --- /dev/null +++ b/doc/whatsnew/fragments/8437.bugfix @@ -0,0 +1,4 @@ +Fix a ``used-before-assignment`` false positive when imports +are made under the ``TYPE_CHECKING`` else if branch. + +Closes #8437 diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 9dd9362907..b477989bba 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -2120,53 +2120,6 @@ def is_class_attr(name: str, klass: nodes.ClassDef) -> bool: return False -def is_defined(name: str, node: nodes.NodeNG) -> bool: - """Searches for a tree node that defines the given variable name.""" - is_defined_so_far = False - - if isinstance(node, nodes.NamedExpr): - is_defined_so_far = node.target.name == name - - if isinstance(node, (nodes.Import, nodes.ImportFrom)): - is_defined_so_far = any(node_name[0] == name for node_name in node.names) - - if isinstance(node, nodes.With): - is_defined_so_far = any( - isinstance(item[1], nodes.AssignName) and item[1].name == name - for item in node.items - ) - - if isinstance(node, (nodes.ClassDef, nodes.FunctionDef)): - is_defined_so_far = node.name == name - - if isinstance(node, nodes.AnnAssign): - is_defined_so_far = ( - node.value - and isinstance(node.target, nodes.AssignName) - and node.target.name == name - ) - - if isinstance(node, nodes.Assign): - is_defined_so_far = any( - any( - ( - ( - isinstance(elt, nodes.Starred) - and isinstance(elt.value, nodes.AssignName) - and elt.value.name == name - ) - or (isinstance(elt, nodes.AssignName) and elt.name == name) - ) - for elt in get_all_elements(target) - ) - for target in node.targets - ) - - return is_defined_so_far or any( - is_defined(name, child) for child in node.get_children() - ) - - def get_inverse_comparator(op: str) -> str: """Returns the inverse comparator given a comparator. diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 130724fa54..422aedf9cd 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -544,7 +544,6 @@ def __init__(self, node: nodes.NodeNG, scope_type: str) -> None: copy.copy(node.locals), {}, collections.defaultdict(list), scope_type ) self.node = node - self._if_nodes_deemed_uncertain: set[nodes.If] = set() def __repr__(self) -> str: _to_consumes = [f"{k}->{v}" for k, v in self._atomic.to_consume.items()] @@ -695,26 +694,28 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None: return found_nodes @staticmethod - def _exhaustively_define_name_raise_or_return( - name: str, node: nodes.NodeNG - ) -> bool: - """Return True if there is a collectively exhaustive set of paths under - this `if_node` that define `name`, raise, or return. + def _inferred_to_define_name_raise_or_return(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. """ # Handle try and with if isinstance(node, (nodes.TryExcept, nodes.TryFinally)): # Allow either a path through try/else/finally OR a path through ALL except handlers - return ( - NamesConsumer._defines_name_raises_or_returns_recursive(name, node) - or isinstance(node, nodes.TryExcept) - and all( - NamesConsumer._defines_name_raises_or_returns_recursive( - name, handler - ) - for handler in node.handlers + try_except_node = node + if isinstance(node, nodes.TryFinally): + try_except_node = next( + (child for child in node.nodes_of_class(nodes.TryExcept)), + None, ) + handlers = try_except_node.handlers if try_except_node else [] + return NamesConsumer._defines_name_raises_or_returns_recursive( + name, node + ) or all( + NamesConsumer._defines_name_raises_or_returns_recursive(name, handler) + for handler in handlers ) - if isinstance(node, nodes.With): + + if isinstance(node, (nodes.With, nodes.For, nodes.While)): return NamesConsumer._defines_name_raises_or_returns_recursive(name, node) if not isinstance(node, nodes.If): @@ -728,13 +729,29 @@ def _exhaustively_define_name_raise_or_return( if NamesConsumer._defines_name_raises_or_returns(name, node): return True - # If there is no else, then there is no collectively exhaustive set of paths - if not node.orelse: - return False + test = node.test.value if isinstance(node.test, nodes.NamedExpr) else node.test + all_inferred = utils.infer_all(test) + only_search_if = False + only_search_else = True + for inferred in all_inferred: + if not isinstance(inferred, nodes.Const): + only_search_else = False + continue + val = inferred.value + only_search_if = only_search_if or (val != NotImplemented and val) + only_search_else = only_search_else and not val + + # 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) + # Search both if and else branches return NamesConsumer._branch_handles_name( name, node.body - ) and NamesConsumer._branch_handles_name(name, node.orelse) + ) or NamesConsumer._branch_handles_name(name, node.orelse) @staticmethod def _branch_handles_name(name: str, body: Iterable[nodes.NodeNG]) -> bool: @@ -742,9 +759,16 @@ def _branch_handles_name(name: str, body: Iterable[nodes.NodeNG]) -> bool: NamesConsumer._defines_name_raises_or_returns(name, if_body_stmt) or isinstance( if_body_stmt, - (nodes.If, nodes.TryExcept, nodes.TryFinally, nodes.With), + ( + nodes.If, + nodes.TryExcept, + nodes.TryFinally, + nodes.With, + nodes.For, + nodes.While, + ), ) - and NamesConsumer._exhaustively_define_name_raise_or_return( + and NamesConsumer._inferred_to_define_name_raise_or_return( name, if_body_stmt ) for if_body_stmt in body @@ -756,53 +780,41 @@ def _uncertain_nodes_in_false_tests( """Identify nodes of uncertain execution because they are defined under tests that evaluate false. - Don't identify a node if there is a collectively exhaustive set of paths - that define the name, raise, or return (e.g. every if/else branch). + 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). """ uncertain_nodes = [] for other_node in found_nodes: - if in_type_checking_block(other_node): - continue - - if not isinstance(other_node, nodes.AssignName): + if isinstance(other_node, nodes.AssignName): + name = other_node.name + elif isinstance(other_node, (nodes.Import, nodes.ImportFrom)): + name = node.name + else: continue - closest_if = utils.get_node_first_ancestor_of_type(other_node, nodes.If) - if closest_if is None: - continue - if node.frame() is not closest_if.frame(): - continue - if closest_if is not None and closest_if.parent_of(node): + all_if = [ + n + for n in other_node.node_ancestors() + if isinstance(n, nodes.If) and not n.parent_of(node) + ] + if not all_if: continue - # Name defined in every if/else branch - if NamesConsumer._exhaustively_define_name_raise_or_return( - other_node.name, closest_if + closest_if = all_if[0] + if ( + isinstance(node, nodes.AssignName) + and node.frame() is not closest_if.frame() ): continue - - # Higher-level if already determined to be always false - if any( - if_node.parent_of(closest_if) - for if_node in self._if_nodes_deemed_uncertain - ): - uncertain_nodes.append(other_node) + if closest_if.parent_of(node): continue - # All inferred values must test false - if isinstance(closest_if.test, nodes.NamedExpr): - test = closest_if.test.value - else: - test = closest_if.test - all_inferred = utils.infer_all(test) - if not all_inferred or not all( - isinstance(inferred, nodes.Const) and not inferred.value - for inferred in all_inferred - ): + outer_if = all_if[-1] + # Name defined in the if/else control flow + if NamesConsumer._inferred_to_define_name_raise_or_return(name, outer_if): continue uncertain_nodes.append(other_node) - self._if_nodes_deemed_uncertain.add(closest_if) return uncertain_nodes @@ -901,6 +913,18 @@ def _defines_name_raises_or_returns(name: str, node: nodes.NodeNG) -> bool: for child_named_expr in node.nodes_of_class(nodes.NamedExpr) ): return True + if isinstance(node, (nodes.Import, nodes.ImportFrom)) and any( + (node_name[1] and node_name[1] == name) or (node_name[0] == name) + for node_name in node.names + ): + return True + if isinstance(node, nodes.With) and any( + isinstance(item[1], nodes.AssignName) and item[1].name == name + for item in node.items + ): + return True + if isinstance(node, (nodes.ClassDef, nodes.FunctionDef)) and node.name == name: + return True return False @staticmethod @@ -919,6 +943,10 @@ def _defines_name_raises_or_returns_recursive( for nested_stmt in stmt.get_children() ): return True + if isinstance( + stmt, nodes.TryExcept + ) and NamesConsumer._defines_name_raises_or_returns_recursive(name, stmt): + return True return False @staticmethod @@ -1688,16 +1716,25 @@ def _check_consumer( if found_nodes is None: return (VariableVisitConsumerAction.CONTINUE, None) if not found_nodes: - if node.name in current_consumer.consumed_uncertain: - confidence = CONTROL_FLOW - else: - confidence = HIGH - self.add_message( - "used-before-assignment", - args=node.name, - node=node, - confidence=confidence, - ) + if ( + not ( + self._postponed_evaluation_enabled + and utils.is_node_in_type_annotation_context(node) + ) + and not self._is_builtin(node.name) + and not self._is_variable_annotation_in_function(node) + ): + confidence = ( + CONTROL_FLOW + if node.name in current_consumer.consumed_uncertain + else HIGH + ) + self.add_message( + "used-before-assignment", + args=node.name, + node=node, + confidence=confidence, + ) # Mark for consumption any nodes added to consumed_uncertain by # get_next_to_consume() because they might not have executed. return ( @@ -1839,7 +1876,9 @@ def _check_consumer( confidence=HIGH, ) - elif self._is_only_type_assignment(node, defstmt): + elif not self._is_builtin(node.name) and self._is_only_type_assignment( + node, defstmt + ): if node.scope().locals.get(node.name): self.add_message( "used-before-assignment", args=node.name, node=node, confidence=HIGH @@ -2030,7 +2069,6 @@ def _in_lambda_or_comprehension_body( parent = parent.parent return False - # pylint: disable = too-many-branches @staticmethod def _is_variable_violation( node: nodes.Name, @@ -2184,42 +2222,6 @@ def _is_variable_violation( anc is defnode.value for anc in node.node_ancestors() ) - # Look for type checking definitions inside a type checking guard. - # Relevant for function annotations only, not variable annotations (AnnAssign) - if ( - isinstance(defstmt, (nodes.Import, nodes.ImportFrom)) - and isinstance(defstmt.parent, nodes.If) - and in_type_checking_block(defstmt) - and not in_type_checking_block(node) - ): - defstmt_parent = defstmt.parent - - maybe_annotation = utils.get_node_first_ancestor_of_type( - node, nodes.AnnAssign - ) - if not ( - maybe_annotation - and utils.get_node_first_ancestor_of_type( - maybe_annotation, nodes.FunctionDef - ) - ): - # Exempt those definitions that are used inside the type checking - # guard or that are defined in any elif/else type checking guard branches. - used_in_branch = defstmt_parent.parent_of(node) - if not used_in_branch: - if defstmt_parent.has_elif_block(): - defined_in_or_else = utils.is_defined( - node.name, defstmt_parent.orelse[0] - ) - else: - defined_in_or_else = any( - utils.is_defined(node.name, content) - for content in defstmt_parent.orelse - ) - - if not defined_in_or_else: - maybe_before_assign = True - return maybe_before_assign, annotation_return, use_outer_definition @staticmethod @@ -2259,18 +2261,15 @@ def _maybe_used_and_assigned_at_once(defstmt: nodes.Statement) -> bool: for call in value.nodes_of_class(klass=nodes.Call) ) - def _is_only_type_assignment( - self, node: nodes.Name, defstmt: nodes.Statement - ) -> bool: + def _is_builtin(self, name: str) -> bool: + return name in self.linter.config.additional_builtins or utils.is_builtin(name) + + @staticmethod + def _is_only_type_assignment(node: nodes.Name, defstmt: nodes.Statement) -> bool: """Check if variable only gets assigned a type and never a value.""" if not isinstance(defstmt, nodes.AnnAssign) or defstmt.value: return False - if node.name in self.linter.config.additional_builtins or utils.is_builtin( - node.name - ): - return False - defstmt_frame = defstmt.frame(future=True) node_frame = node.frame(future=True) @@ -2359,6 +2358,16 @@ def _is_never_evaluated( return True return False + @staticmethod + def _is_variable_annotation_in_function(node: nodes.NodeNG) -> bool: + is_annotation = utils.get_node_first_ancestor_of_type(node, nodes.AnnAssign) + return ( + is_annotation + and utils.get_node_first_ancestor_of_type( # type: ignore[return-value] + is_annotation, nodes.FunctionDef + ) + ) + def _ignore_class_scope(self, node: nodes.NodeNG) -> bool: """Return True if the node is in a local class scope, as an assignment. diff --git a/tests/functional/u/undefined/undefined_variable.txt b/tests/functional/u/undefined/undefined_variable.txt index ab1a004209..b41f9ae46f 100644 --- a/tests/functional/u/undefined/undefined_variable.txt +++ b/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.:Undefined variable 'LambdaClass4':UNDEFINED undefined-variable:234:25:234:37:LambdaClass5.:Undefined variable 'LambdaClass5':UNDEFINED -used-before-assignment:255:26:255:34:func_should_fail:Using variable 'datetime' before assignment:HIGH +used-before-assignment:255:26:255:34:func_should_fail:Using variable 'datetime' before assignment:CONTROL_FLOW 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 diff --git a/tests/functional/u/used/used_before_assignment_postponed_evaluation.py b/tests/functional/u/used/used_before_assignment_postponed_evaluation.py new file mode 100644 index 0000000000..4ff22470cc --- /dev/null +++ b/tests/functional/u/used/used_before_assignment_postponed_evaluation.py @@ -0,0 +1,13 @@ +"""Tests for used-before-assignment when postponed evaluation of annotations is enabled""" +# pylint: disable=missing-function-docstring, invalid-name +from __future__ import annotations +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + var = 1 + import math + +print(var) # [used-before-assignment] + +def function_one(m: math): # no error for annotations + return m diff --git a/tests/functional/u/used/used_before_assignment_postponed_evaluation.txt b/tests/functional/u/used/used_before_assignment_postponed_evaluation.txt new file mode 100644 index 0000000000..88a9587369 --- /dev/null +++ b/tests/functional/u/used/used_before_assignment_postponed_evaluation.txt @@ -0,0 +1 @@ +used-before-assignment:10:6:10:9::Using variable 'var' before assignment:CONTROL_FLOW diff --git a/tests/functional/u/used/used_before_assignment_typing.py b/tests/functional/u/used/used_before_assignment_typing.py index 5823babfc1..9ec040ff62 100644 --- a/tests/functional/u/used/used_before_assignment_typing.py +++ b/tests/functional/u/used/used_before_assignment_typing.py @@ -7,11 +7,12 @@ if TYPE_CHECKING: if True: # pylint: disable=using-constant-test import math + import dbm + print(dbm) # no error when defined and used in the same false branch from urllib.request import urlopen import array import base64 import binascii - import bisect import calendar import collections import copy diff --git a/tests/functional/u/used/used_before_assignment_typing.txt b/tests/functional/u/used/used_before_assignment_typing.txt index c0a31fae08..12794f0e95 100644 --- a/tests/functional/u/used/used_before_assignment_typing.txt +++ b/tests/functional/u/used/used_before_assignment_typing.txt @@ -1,5 +1,5 @@ -undefined-variable:68:21:68:28:MyClass.incorrect_typing_method:Undefined variable 'MyClass':UNDEFINED -undefined-variable:73:26:73:33:MyClass.incorrect_nested_typing_method:Undefined variable 'MyClass':UNDEFINED -undefined-variable:78:20:78:27:MyClass.incorrect_default_method:Undefined variable 'MyClass':UNDEFINED -used-before-assignment:139:35:139:39:MyFourthClass.is_close:Using variable 'math' before assignment:HIGH -used-before-assignment:152:20:152:28:VariableAnnotationsGuardedByTypeChecking:Using variable 'datetime' before assignment:HIGH +undefined-variable:69:21:69:28:MyClass.incorrect_typing_method:Undefined variable 'MyClass':UNDEFINED +undefined-variable:74:26:74:33:MyClass.incorrect_nested_typing_method:Undefined variable 'MyClass':UNDEFINED +undefined-variable:79:20:79:27:MyClass.incorrect_default_method:Undefined variable 'MyClass':UNDEFINED +used-before-assignment:140:35:140:39:MyFourthClass.is_close:Using variable 'math' before assignment:CONTROL_FLOW +used-before-assignment:153:20:153:28:VariableAnnotationsGuardedByTypeChecking:Using variable 'datetime' before assignment:CONTROL_FLOW