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

Fix false negative for used-before-assignment when some except handlers don't define a name #5764

Merged
merged 22 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
6 changes: 6 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ Release date: TBA

Closes #5500

* Fixed a false negative for ``used-before-assignment`` when some but not all
except handlers defined a name relied upon after an except block when the
corresponding try block contained a return statement.

Partially closes #5524

* When evaluating statements in the ``else`` clause of a loop, ``used-before-assignment``
assumes that assignments in the except blocks took place if the
except handlers constituted the only ways for the loop to finish without
Expand Down
6 changes: 6 additions & 0 deletions doc/whatsnew/2.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,12 @@ Other Changes

Closes #5500

* Fixed a false negative for ``used-before-assignment`` when some but not all
except handlers defined a name relied upon after an except block when the
corresponding try block contained a return statement.

Partially closes #5524

* When evaluating statements in the ``else`` clause of a loop, ``used-before-assignment``
assumes that assignments in the except blocks took place if the
except handlers constituted the only ways for the loop to finish without
Expand Down
65 changes: 47 additions & 18 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -703,17 +703,18 @@ def _uncertain_nodes_in_except_blocks(
for other_node in found_nodes:
other_node_statement = other_node.statement(future=True)
# Only testing for statements in the except block of TryExcept
if not (
isinstance(other_node_statement.parent, nodes.ExceptHandler)
and isinstance(other_node_statement.parent.parent, nodes.TryExcept)
):
closest_except_handler = utils.get_node_first_ancestor_of_type(
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
other_node_statement, nodes.ExceptHandler
)
if not closest_except_handler:
continue
# If the other node is in the same scope as this node, assume it executes
if other_node_statement.parent.parent_of(node):
if closest_except_handler.parent_of(node):
continue
closest_try_except: nodes.TryExcept = closest_except_handler.parent
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
try_block_returns = any(
isinstance(try_statement, nodes.Return)
for try_statement in other_node_statement.parent.parent.body
for try_statement in closest_try_except.body
)
# If the try block returns, assume the except blocks execute.
if try_block_returns:
Expand All @@ -722,28 +723,56 @@ def _uncertain_nodes_in_except_blocks(
if (
isinstance(node_statement.parent, nodes.TryFinally)
and node_statement in node_statement.parent.finalbody
# We have already tested that other_node_statement has two parents
# and it was TryExcept, so getting one more parent is safe.
and other_node_statement.parent.parent.parent.parent_of(
node_statement
)
and closest_try_except.parent.parent_of(node_statement)
Copy link
Member

Choose a reason for hiding this comment

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

Good job generalizing here, this is a lot cleaner.

):
uncertain_nodes.append(other_node)
else:
# Assume the except blocks execute. Possibility for a false negative
# if one of the except blocks does not define the name in question,
# raise, or return. See: https://github.com/PyCQA/pylint/issues/5524.
# Assume the except blocks execute, so long as each handler
# defines the name, raises, or returns.
elif all(
NamesConsumer._defines_name_raises_or_returns(node.name, handler)
for handler in closest_try_except.handlers
):
continue

if NamesConsumer._check_loop_finishes_via_except(
node, other_node_statement.parent.parent
):
if NamesConsumer._check_loop_finishes_via_except(node, closest_try_except):
continue

# Passed all tests for uncertain execution
uncertain_nodes.append(other_node)
return uncertain_nodes

@staticmethod
def _defines_name_raises_or_returns(
name: str, handler: nodes.ExceptHandler
) -> bool:
"""Return True if some child of `handler` defines the name `name`,
raises, or returns.
"""
for stmt in handler.get_children():
if isinstance(stmt, (nodes.Raise, nodes.Return)):
return True
if isinstance(stmt, nodes.Assign):
for target in stmt.targets:
for elt in utils.get_all_elements(target):
if isinstance(elt, nodes.AssignName) and elt.name == name:
return True
if isinstance(stmt, nodes.If):
if (
isinstance(stmt.test, nodes.NamedExpr)
and stmt.test.target.name == name
):
return True
if isinstance(stmt.test, nodes.Call):
for arg_or_kwarg in stmt.test.args + [
kw.value for kw in stmt.test.keywords
]:
if (
isinstance(arg_or_kwarg, nodes.NamedExpr)
and arg_or_kwarg.target.name == name
):
return True
return False

@staticmethod
def _check_loop_finishes_via_except(
node: nodes.NodeNG, other_node_try_except: nodes.TryExcept
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,66 @@ def func_ok3(var):
except ZeroDivisionError:
msg = "Division by 0"
print(msg)


def func_ok4(var):
"""Define "msg" with a chained assignment."""
try:
return 1 / var.some_other_func()
except AttributeError:
msg2 = msg = "Division by 0"
print(msg2)
except ZeroDivisionError:
msg = "Division by 0"
print(msg)


def func_ok5(var):
"""Define 'msg' via unpacked iterable."""
try:
return 1 / var.some_other_func()
except AttributeError:
msg, msg2 = ["Division by 0", "Division by 0"]
print(msg2)
except ZeroDivisionError:
msg = "Division by 0"
print(msg)


def func_invalid1(var):
"""'msg' is not defined in one handler."""
try:
return 1 / var.some_other_func()
except AttributeError:
pass
except ZeroDivisionError:
msg = "Division by 0"
print(msg) # [used-before-assignment]


def func_invalid2(var):
"""'msg' is not defined in one handler."""
try:
return 1 / var.some_other_func()
except AttributeError:
msg: str
except ZeroDivisionError:
msg = "Division by 0"
print(msg) # [used-before-assignment]


def func_invalid3(var):
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
"""'msg' is not defined in one handler, but is defined in another
nested under an if.
"""
err_message = False
try:
return 1 / var.some_other_func()
except AttributeError:
msg: str
except ZeroDivisionError:
if err_message:
msg = "Division by 0"
else:
msg = None
print(msg) # [used-before-assignment]
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
used-before-assignment:16:14:16:29:function:Using variable 'failure_message' before assignment:CONTROL_FLOW
used-before-assignment:86:10:86:13:func_invalid1:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:97:10:97:13:func_invalid2:Using variable 'msg' before assignment:CONTROL_FLOW
used-before-assignment:114:10:114:13:func_invalid3:Using variable 'msg' before assignment:CONTROL_FLOW
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
"""Tests for used-before-assignment with assignments in except handlers after
try blocks with return statements.
See: https://github.com/PyCQA/pylint/issues/5500.
"""
# pylint: disable=inconsistent-return-statements


# Named expressions
def func_ok_namedexpr_1(var):
"""'msg' is defined in one handler with a named expression under an if."""
try:
return 1 / var.some_other_func()
except AttributeError:
if (msg := var.get_msg()):
pass
except ZeroDivisionError:
msg = "Division by 0"
print(msg)


def func_ok_namedexpr_2(var):
"""'msg' is defined in one handler with a named expression occurring
in a call used in an if test.
"""
try:
return 1 / var.some_other_func()
except AttributeError:
if print(msg := var.get_msg()):
pass
except ZeroDivisionError:
msg = "Division by 0"
print(msg)


def func_ok_namedexpr_3(var):
"""'msg' is defined in one handler with a named expression occurring
as a keyword in a call used in an if test.
"""
try:
return 1 / var.some_other_func()
except AttributeError:
if print("zero!", "here", sep=(msg := var.get_sep())):
pass
except ZeroDivisionError:
msg = "Division by 0"
print(msg)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[testoptions]
min_pyver=3.8