Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/data/messages/p/possibly-used-before-assignment/bad.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
def check_lunchbox(items: list[str]):
if not items:
empty = True
print(empty) # [possibly-used-before-assignment]
15 changes: 15 additions & 0 deletions doc/data/messages/p/possibly-used-before-assignment/details.rst
Original file line number Diff line number Diff line change
@@ -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.)
5 changes: 5 additions & 0 deletions doc/data/messages/p/possibly-used-before-assignment/good.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
def check_lunchbox(items: list[str]):
empty = False
if not items:
empty = True
print(empty)
3 changes: 3 additions & 0 deletions doc/user_guide/checkers/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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__*
Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions doc/whatsnew/fragments/1727.new_check
Original file line number Diff line number Diff line change
@@ -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
82 changes: 49 additions & 33 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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()]
Expand Down Expand Up @@ -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 = [
Expand All @@ -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:
Expand Down Expand Up @@ -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.
"""
Expand All @@ -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?
Expand All @@ -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(
Expand All @@ -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).
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ disable=
# We anticipate #3512 where it will become optional
fixme,
consider-using-assignment-expr,
possibly-used-before-assignment,


[REPORTS]
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/r/redefined/redefined_except_handler.txt
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion tests/functional/u/undefined/undefined_variable.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/u/undefined/undefined_variable_py38.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ undefined-variable:106:6:106:19::Undefined variable 'else_assign_2':INFERENCE
used-before-assignment:141:10:141:16:type_annotation_used_improperly_after_comprehension:Using variable 'my_int' before assignment:HIGH
used-before-assignment:148:10:148:16:type_annotation_used_improperly_after_comprehension_2:Using variable 'my_int' before assignment:HIGH
used-before-assignment:186:9:186:10::Using variable 'z' before assignment:HIGH
used-before-assignment:193:6:193:19::Using variable 'NEVER_DEFINED' before assignment:CONTROL_FLOW
used-before-assignment:193:6:193:19::Using variable 'NEVER_DEFINED' before assignment:INFERENCE
24 changes: 21 additions & 3 deletions tests/functional/u/used/used_before_assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def redefine_time_import_with_global():
pass
else:
VAR4 = False
if VAR4: # [used-before-assignment]
if VAR4: # [possibly-used-before-assignment]
pass

if FALSE:
Expand All @@ -70,7 +70,7 @@ def redefine_time_import_with_global():
VAR5 = True
else:
VAR5 = True
if VAR5:
if VAR5: # [possibly-used-before-assignment]
pass

if FALSE:
Expand Down Expand Up @@ -116,7 +116,7 @@ def redefine_time_import_with_global():
VAR11 = num
if VAR11:
VAR12 = False
print(VAR12)
print(VAR12) # [possibly-used-before-assignment]

def turn_on2(**kwargs):
"""https://github.com/pylint-dev/pylint/issues/7873"""
Expand Down Expand Up @@ -180,3 +180,21 @@ def give_me_none():
class T: # pylint: disable=invalid-name, too-few-public-methods, undefined-variable
'''Issue #8754, no crash from unexpected assignment between attribute and variable'''
T.attr = attr


if outer():
NOT_ALWAYS_DEFINED = True
print(NOT_ALWAYS_DEFINED) # [used-before-assignment]


def inner_if_continues_outer_if_has_no_other_statements():
for i in range(5):
if isinstance(i, int):
# Testing no assignment here, before the inner if
if i % 2 == 0:
order = None
else:
continue
else:
order = None
print(order)
15 changes: 9 additions & 6 deletions tests/functional/u/used/used_before_assignment.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ used-before-assignment:10:4:10:9:outer:Using variable 'inner' before assignment:
used-before-assignment:19:20:19:40:ClassWithProperty:Using variable 'redefine_time_import' before assignment:HIGH
used-before-assignment:23:0:23:9::Using variable 'calculate' before assignment:HIGH
used-before-assignment:31:10:31:14:redefine_time_import:Using variable 'time' before assignment:HIGH
used-before-assignment:45:3:45:7::Using variable 'VAR2' before assignment:CONTROL_FLOW
used-before-assignment:63:3:63:7::Using variable 'VAR4' before assignment:CONTROL_FLOW
used-before-assignment:78:3:78:7::Using variable 'VAR6' before assignment:CONTROL_FLOW
used-before-assignment:113:6:113:11::Using variable 'VAR10' before assignment:CONTROL_FLOW
used-before-assignment:144:10:144:14::Using variable 'SALE' before assignment:CONTROL_FLOW
used-before-assignment:176:10:176:18::Using variable 'ALL_DONE' before assignment:CONTROL_FLOW
used-before-assignment:45:3:45:7::Using variable 'VAR2' before assignment:INFERENCE
possibly-used-before-assignment:63:3:63:7::Possibly using variable 'VAR4' before assignment:INFERENCE
possibly-used-before-assignment:73:3:73:7::Possibly using variable 'VAR5' before assignment:INFERENCE
used-before-assignment:78:3:78:7::Using variable 'VAR6' before assignment:INFERENCE
used-before-assignment:113:6:113:11::Using variable 'VAR10' before assignment:INFERENCE
possibly-used-before-assignment:119:6:119:11::Possibly using variable 'VAR12' before assignment:CONTROL_FLOW
used-before-assignment:144:10:144:14::Using variable 'SALE' before assignment:INFERENCE
used-before-assignment:176:10:176:18::Using variable 'ALL_DONE' before assignment:INFERENCE
used-before-assignment:187:6:187:24::Using variable 'NOT_ALWAYS_DEFINED' before assignment:INFERENCE
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
used-before-assignment:16:14:16:29:function:Using variable 'failure_message' before assignment:CONTROL_FLOW
used-before-assignment:120:10:120:13:func_invalid1:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:131:10:131:13:func_invalid2:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:150:10:150:13:func_invalid3:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:150:10:150:13:func_invalid3:Using variable 'msg' before assignment:INFERENCE
used-before-assignment:163:10:163:13:func_invalid4:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:175:10:175:13:func_invalid5:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:187:10:187:13:func_invalid6:Using variable 'msg' before assignment:CONTROL_FLOW
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def used_before_assignment_2(a):


def used_before_assignment_3(a):
if x == a: # [used-before-assignment]
if x == a: # [possibly-used-before-assignment]
if x > 3:
x = 2 # [redefined-outer-name]
Comment on lines -19 to 21
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a shame, but I can live with it.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ used-before-assignment:7:7:7:8:used_before_assignment_1:Using variable 'x' befor
redefined-outer-name:8:12:8:13:used_before_assignment_1:Redefining name 'x' from outer scope (line 3):UNDEFINED
used-before-assignment:13:7:13:8:used_before_assignment_2:Using variable 'x' before assignment:HIGH
redefined-outer-name:15:4:15:5:used_before_assignment_2:Redefining name 'x' from outer scope (line 3):UNDEFINED
used-before-assignment:19:7:19:8:used_before_assignment_3:Using variable 'x' before assignment:HIGH
possibly-used-before-assignment:19:7:19:8:used_before_assignment_3:Possibly using variable 'x' before assignment:CONTROL_FLOW
redefined-outer-name:21:12:21:13:used_before_assignment_3:Redefining name 'x' from outer scope (line 3):UNDEFINED
redefined-outer-name:30:4:30:5:not_used_before_assignment_2:Redefining name 'x' from outer scope (line 3):UNDEFINED
4 changes: 2 additions & 2 deletions tests/functional/u/used/used_before_assignment_issue2615.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
used-before-assignment:12:14:12:17:main:Using variable 'res' before assignment:CONTROL_FLOW
used-before-assignment:12:14:12:17:main:Using variable 'res' before assignment:INFERENCE
used-before-assignment:30:18:30:35:nested_except_blocks:Using variable 'more_bad_division' before assignment:CONTROL_FLOW
used-before-assignment:31:18:31:21:nested_except_blocks:Using variable 'res' before assignment:CONTROL_FLOW
used-before-assignment:31:18:31:21:nested_except_blocks:Using variable 'res' before assignment:INFERENCE
4 changes: 2 additions & 2 deletions tests/functional/u/used/used_before_assignment_issue626.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
unused-variable:5:4:6:12:main1:Unused variable 'e':UNDEFINED
used-before-assignment:8:10:8:11:main1:Using variable 'e' before assignment:CONTROL_FLOW
used-before-assignment:8:10:8:11:main1:Using variable 'e' before assignment:HIGH
unused-variable:21:4:22:12:main3:Unused variable 'e':UNDEFINED
unused-variable:31:4:32:12:main4:Unused variable 'e':UNDEFINED
used-before-assignment:44:10:44:11:main4:Using variable 'e' before assignment:CONTROL_FLOW
used-before-assignment:44:10:44:11:main4:Using variable 'e' before assignment:HIGH
Original file line number Diff line number Diff line change
@@ -1 +1 @@
used-before-assignment:10:6:10:9::Using variable 'var' before assignment:CONTROL_FLOW
used-before-assignment:10:6:10:9::Using variable 'var' before assignment:INFERENCE
4 changes: 2 additions & 2 deletions tests/functional/u/used/used_before_assignment_scoping.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
used-before-assignment:10:13:10:21:func_two:Using variable 'datetime' before assignment:CONTROL_FLOW
used-before-assignment:16:12:16:20:func:Using variable 'datetime' before assignment:CONTROL_FLOW
used-before-assignment:10:13:10:21:func_two:Using variable 'datetime' before assignment:INFERENCE
used-before-assignment:16:12:16:20:func:Using variable 'datetime' before assignment:INFERENCE
Loading
Loading