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

Don't assume direct parentage when emitting used-before-assignment #5582

Merged
merged 17 commits into from
Jan 10, 2022
Merged
Show file tree
Hide file tree
Changes from 13 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
6 changes: 6 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ Release date: TBA

Closes #85, #2615

* Fixed false negative for ``used-before-assignment`` when a conditional
or context manager intervened before the try statement that suggested
it might fail.

Closes #4045

* Fixed extremely long processing of long lines with comma's.

Closes #5483
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 @@ -63,6 +63,12 @@ Other Changes

Closes #85, #2615

* Fixed false negative for ``used-before-assignment`` when a conditional
or context manager intervened before the try statement that suggested
it might fail.

Closes #4045

* Fix a false positive for ``assigning-non-slot`` when the slotted class
defined ``__setattr__``.

Expand Down
16 changes: 16 additions & 0 deletions pylint/checkers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1719,3 +1719,19 @@ def get_node_first_ancestor_of_type(
if isinstance(ancestor, ancestor_type):
return ancestor
return None


def get_node_first_ancestor_of_type_and_its_child(
node: nodes.NodeNG, ancestor_type: Union[Type[T_Node], Tuple[Type[T_Node]]]
) -> Tuple[Optional[T_Node], Optional[T_Node]]:
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
"""Modified version of get_node_first_ancestor_of_type to also return the
descendant visited directly before reaching the sought ancestor. Useful
for extracting whether a statement is guarded by a try, except, or finally
when searching for a TryFinally ancestor.
"""
last_ancestor = node
for ancestor in node.node_ancestors():
if isinstance(ancestor, ancestor_type):
return (ancestor, last_ancestor)
last_ancestor = ancestor
return None, None
69 changes: 55 additions & 14 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -677,21 +677,15 @@ def get_next_to_consume(self, node):

# If this node is in an ExceptHandler,
# filter out assignments in the try portion, assuming they may fail
if found_nodes and isinstance(node_statement.parent, nodes.ExceptHandler):
filtered_nodes = [
n
for n in found_nodes
if not (
isinstance(n.statement(future=True).parent, nodes.TryExcept)
and n.statement(future=True) in n.statement(future=True).parent.body
and node_statement.parent
in n.statement(future=True).parent.handlers
if found_nodes:
uncertain_nodes = (
self._uncertain_nodes_in_try_blocks_when_evaluating_except_blocks(
found_nodes, node_statement
)
]
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
)
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]

return found_nodes

Expand Down Expand Up @@ -740,6 +734,53 @@ def _uncertain_nodes_in_except_blocks(found_nodes, node, node_statement):
uncertain_nodes.append(other_node)
return uncertain_nodes

@staticmethod
def _uncertain_nodes_in_try_blocks_when_evaluating_except_blocks(
found_nodes: List[nodes.NodeNG], node_statement
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
) -> List[nodes.NodeNG]:
"""Return any nodes in ``found_nodes`` that should be treated as uncertain
because they are in a try block and the ``node_statement`` being evaluated
is in one of its except handlers.
"""
uncertain_nodes: List[nodes.NodeNG] = []
closest_except_handler = utils.get_node_first_ancestor_of_type(
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
node_statement, nodes.ExceptHandler
)
if closest_except_handler is None:
return uncertain_nodes
for other_node in found_nodes:
other_node_statement = other_node.statement(future=True)
# If the other statement is the except handler guarding `node`, it executes
if other_node_statement is closest_except_handler:
continue
# Ensure other_node is in a try block
(
other_node_try_ancestor,
other_node_try_ancestor_visited_child,
) = utils.get_node_first_ancestor_of_type_and_its_child(
other_node_statement, nodes.TryExcept
)
if other_node_try_ancestor is None:
continue
if (
other_node_try_ancestor_visited_child
not in other_node_try_ancestor.body
):
continue
# Make sure nesting is correct -- there should be at least one
# except handler that is a sibling attached to the try ancestor,
# or is an ancestor of the try ancestor.
if not any(
closest_except_handler in other_node_try_ancestor.handlers
or other_node_try_ancestor_except_handler
in closest_except_handler.node_ancestors()
for other_node_try_ancestor_except_handler in other_node_try_ancestor.handlers
):
continue
# Passed all tests for uncertain execution
uncertain_nodes.append(other_node)
return uncertain_nodes


# pylint: disable=too-many-public-methods
class VariablesChecker(BaseChecker):
Expand Down
51 changes: 51 additions & 0 deletions tests/functional/u/use/used_before_assignment_issue2615.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,57 @@ def main():
try:
res = 1 / 0
res = 42
if main():
res = None
with open(__file__, encoding="utf-8") as opened_file:
res = opened_file.readlines()
except ZeroDivisionError:
print(res) # [used-before-assignment]
print(res)


def nested_except_blocks():
"""Assignments in an except are tested against possibly failing
assignments in try blocks at two different nesting levels."""
try:
res = 1 / 0
res = 42
if main():
res = None
with open(__file__, encoding="utf-8") as opened_file:
res = opened_file.readlines()
except ZeroDivisionError:
try:
more_bad_division = 1 / 0
except ZeroDivisionError:
print(more_bad_division) # [used-before-assignment]
print(res) # [used-before-assignment]
print(res)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved


def consecutive_except_blocks():
"""An assignment assumed to execute in one TryExcept should continue to be
assumed to execute in a consecutive TryExcept.
"""
try:
res = 100
except ZeroDivisionError:
pass
try:
pass
except ValueError:
print(res)
Comment on lines +35 to +46
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand everything here.

If the first except is raised and the function do not return, it means in the second one res won't be defined ?

Suggested change
def consecutive_except_blocks():
"""An assignment assumed to execute in one TryExcept should continue to be
assumed to execute in a consecutive TryExcept.
"""
try:
res = 100
except ZeroDivisionError:
pass
try:
pass
except ValueError:
print(res)
def consecutive_except_blocks():
try:
res = 100
except ZeroDivisionError:
pass
try:
pass
except ValueError:
print(res) # [used-before-assignment]
def consecutive_except_blocks_with_return():
try:
res = 100
except ZeroDivisionError:
return
try:
pass
except ValueError:
print(res)

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, we want to avoid annoying users, and so outside a TryExcept we assume the try statements executed. By that same logic we assume that outside a TryExcept its except statements did not execute -- because it would be a bit backwards to assume the try statements never execute and thus that assignments in the except always happen.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't we say that like Pylance we consider that everything in a try/except can fail ? (#5384). Also, if we do not raise or return in the except it means we can reach line 43 from one branch or another and we would need to consider both branches in order to be correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that wasn't my understanding, and that's not what Pylance does. The yellow underline is Pylance, showing that the checker we just worked on is in agreement with Pylance:
Screen Shot 2021-12-23 at 10 44 31 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess a good way of putting it is that's why there are different private methods in this checker now:

_uncertain_nodes_in_except_blocks
_uncertain_nodes_in_try_blocks_when_evaluating_except_blocks
_uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks

We make different control flow inferences based on where the name is being used (try, except, finally, or outside).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the points you're making here. Especially the last example is something of which I can easily imagine complaints coming from. Code with requests and databases seem to often really on such patterns.

Perhaps you could add a summary of this explanation to the issue you mentioned? That way we don't lose this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Especially the last example is something of which I can easily imagine complaints coming from.

My last example was not quite right 😬 -- I neglected that subsequent handlers can't swallow exceptions raised in earlier handlers, so ValueError can't be passed down to a subsequent handler. I'll edit it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps you could add a summary of this explanation to the issue you mentioned? That way we don't lose this.

I think this discussion is getting at when an else clause should be used in a try/except/else. If you can depend on exhaustive handling by your except handlers, you don't need to guard your print(a) after the try/except. If it's not exhaustively handled, then your print(a) should be in an else.

I guess this could even be its own checker, so it could be enabled/disabled separately. (It would probably be easier to implement that way, given how much new logic would go into the exhaustiveness checking.)

Copy link
Member

Choose a reason for hiding this comment

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

I think relying on everyone knowing that library_function_that_might_sysexit will implicitly exit all the time is an anti-pattern. Personally I would initialize the variable to be sure, or at least to add a comment stating explicitly that there's an exit and it's ok to do that if there's a good reason to do that like performance. (a pylint disable is not that much more work then),

But I get that this might be a little opinionated and that we'd get complaints from this, thank you for explaining.

Since we're talking about addressing false negatives, not false positives, that seems like a smart way of iterating to me.

πŸ‘

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 discussion helped clarify for me that beefing up the checker to assume try statements always fail is worthy of discussion, but would need to be solved in tandem with #5524, so I suggest we keep discussing there for 2.14 or later.



def name_earlier_in_except_block():
"""Permit the name that might not have been assigned during the try block
to be defined inside a conditional inside the except block.
"""
try:
res = 1 / 0
except ZeroDivisionError:
if main():
res = 10
else:
res = 11
print(res)
4 changes: 3 additions & 1 deletion tests/functional/u/use/used_before_assignment_issue2615.txt
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
used-before-assignment:8:14:8:17:main:Using variable 'res' before assignment:UNDEFINED
used-before-assignment:12:14:12:17:main:Using variable 'res' before assignment:UNDEFINED
used-before-assignment:30:18:30:35:nested_except_blocks:Using variable 'more_bad_division' before assignment:UNDEFINED
used-before-assignment:31:18:31:21:nested_except_blocks:Using variable 'res' before assignment:UNDEFINED