-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix used-before-assignment false negative with bare type annotations #10852
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
base: main
Are you sure you want to change the base?
Changes from all commits
4fb4079
b498124
95ecbab
330ef41
02b359a
f380c3e
9391b02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Fix ``used-before-assignment`` false negative when a variable has a bare type | ||
| annotation (without a value) and is only assigned inside ``except`` blocks. | ||
|
|
||
| Closes #10847 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -617,40 +617,59 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None: | |
| 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 | ||
| # Filter out assignments in except/try blocks, tracking whether any | ||
| # uncertainty came from these blocks (as opposed to if/elif tests). | ||
| has_except_uncertainty = False | ||
| if found_nodes: | ||
| uncertain_nodes = self._uncertain_nodes_in_except_blocks( | ||
| found_nodes, node, node_statement | ||
| ) | ||
| has_except_uncertainty = has_except_uncertainty or bool(uncertain_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] | ||
|
|
||
| # If this node is in a Finally block of a Try/Finally, | ||
| # filter out assignments in the try portion, assuming they may fail | ||
|
Comment on lines
-633
to
-634
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert the unnecessary comment deletions like this (there are more than one). |
||
| if found_nodes: | ||
| uncertain_nodes = ( | ||
| self._uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks( | ||
| found_nodes, node_statement, name | ||
| ) | ||
| ) | ||
| has_except_uncertainty = has_except_uncertainty or bool(uncertain_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] | ||
|
|
||
| # If this node is in an ExceptHandler, | ||
| # filter out assignments in the try portion, assuming they may fail | ||
| if found_nodes: | ||
| uncertain_nodes = ( | ||
| self._uncertain_nodes_in_try_blocks_when_evaluating_except_blocks( | ||
| found_nodes, node_statement | ||
| ) | ||
| ) | ||
| has_except_uncertainty = has_except_uncertainty or bool(uncertain_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] | ||
|
|
||
| # Treat bare type annotations (AnnAssign without a value) as uncertain | ||
| # when there are uncertain definitions from except/try blocks. | ||
| # A bare annotation like `x: int` does not actually assign a value, | ||
| # so it should not suppress possibly-used-before-assignment when the | ||
| # real assignments are in except blocks that may not execute. | ||
| # We only do this for except/try uncertainty, not for if/elif | ||
| # uncertainty, because if/elif branches are guaranteed to execute | ||
| # one path (the bare annotation masking is correct there). | ||
| if found_nodes and has_except_uncertainty: | ||
| bare_annotations = [ | ||
| n | ||
| for n in found_nodes | ||
| if isinstance(n.parent, nodes.AnnAssign) and n.parent.value is None | ||
| ] | ||
| if bare_annotations: | ||
| self.consumed_uncertain[name] += bare_annotations | ||
| bare_set = set(bare_annotations) | ||
| found_nodes = [n for n in found_nodes if n not in bare_set] | ||
|
|
||
| return found_nodes | ||
|
|
||
| def _inferred_to_define_name_raise_or_return( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,3 +107,46 @@ def loop_conditional_annotated_assignment(): | |
| data={"cat": "harf"} | ||
| token: str = data.get("cat") # [possibly-used-before-assignment] | ||
| print(token) | ||
|
|
||
|
|
||
| def bare_annotation_with_except_assignment(text): | ||
| """A bare type annotation should not suppress used-before-assignment | ||
| when the only real assignments are in except blocks. | ||
|
|
||
| https://github.com/pylint-dev/pylint/issues/10847 | ||
| """ | ||
| err: int | ||
| try: | ||
| result = int(text) | ||
| except ValueError: | ||
| err = 1 | ||
| result = -1 | ||
| if result < 0: | ||
| print(err) # [used-before-assignment] | ||
|
|
||
|
|
||
| def bare_annotation_with_value_and_except(text): | ||
| """A type annotation with a value should suppress the warning.""" | ||
| err: int = 0 | ||
| try: | ||
| result = int(text) | ||
| except ValueError: | ||
| err = 1 | ||
| result = -1 | ||
| if result < 0: | ||
| print(err) | ||
|
|
||
|
|
||
| def bare_annotation_with_if_elif(axis): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a combination test case that involves both if/elif and try/except? |
||
| """A bare type annotation with if/elif should not be a false positive. | ||
|
|
||
| Regression test: the pandas pattern where a bare annotation precedes | ||
| an if/elif chain that covers all runtime values. | ||
| https://github.com/pylint-dev/pylint/pull/10852#pullrequestreview-3872486590 | ||
| """ | ||
| klass: type | ||
| if axis == 0: | ||
| klass = int | ||
| elif axis == 1: | ||
| klass = str | ||
| return klass | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this project's style prefers implicit booleanness.