Fix used-before-assignment false negative with bare type annotations#10852
Fix used-before-assignment false negative with bare type annotations#10852worksbyfriday wants to merge 7 commits intopylint-dev:mainfrom
Conversation
When a variable has a bare type annotation (e.g. `err: int`) and is only assigned inside except blocks, pylint failed to report used-before-assignment. The bare annotation was treated as a valid definition, suppressing the check. The fix filters bare annotations (AnnAssign without a value) from found_nodes in get_next_to_consume() when there are uncertain definitions (e.g., assignments in except blocks). This ensures the code correctly falls through to _report_unfound_name_definition(). Closes pylint-dev#10847
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10852 +/- ##
==========================================
- Coverage 96.03% 96.03% -0.01%
==========================================
Files 177 177
Lines 19621 19633 +12
==========================================
+ Hits 18844 18855 +11
- Misses 777 778 +1
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
Thank you for working on pylint, let's optimize a little if we can.
| # 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( |
There was a problem hiding this comment.
Shouldn't this be filtered in self._uncertain_nodes_in_try_blocks_when_evaluating_except_blocks / _uncertain_nodes_if_tests instead ?
There was a problem hiding this comment.
Good question. I considered both approaches. The _uncertain_nodes_* methods filter nodes that are conditionally defined (in try/except, if/else). But bare annotations are not conditionally defined — they are simply not definitions at all (no value assigned).
The reworked approach (commit b498124) moves bare annotations to consumed_uncertain instead of removing them from to_consume. This way: the variable checker knows the annotation exists (it is tracked, not deleted), but it is marked as uncertain, so used-before-assignment fires correctly when the name is used before any real assignment. The unused-variable checker does not false-positive on the annotation because it has been consumed.
This felt cleaner than adding annotation-detection logic to the _uncertain_nodes_* methods, which are specifically about control flow uncertainty, not type annotation semantics. But open to restructuring if you prefer the filtering approach.
|
The primer doesn't look good |
|
You're right on both counts. The primer regressions show my fix is too broad — filtering bare annotations in Your suggestion to move the filtering to Will push an updated approach once I've worked through it. |
Instead of filtering bare type annotations out of found_nodes entirely (which caused unused-variable false positives in the primer), move them to consumed_uncertain. This preserves node tracking (preventing false unused-variable) while still treating bare annotations as uncertain definitions (correctly flagging used-before-assignment). The key insight: removing nodes from found_nodes makes them invisible to the unused-variable checker. Moving them to consumed_uncertain keeps them tracked but marks them as conditionally defined. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Pushed a rework based on the primer regression analysis. The key insight: Before (caused After: Bare annotations are moved to The change is in |
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
left a comment
There was a problem hiding this comment.
No concerns with the approach.
Rewrite test examples per review: use a specific exception type instead of Exception to avoid broad-exception-caught disables.
This comment has been minimized.
This comment has been minimized.
Use realistic int(text) conversion instead of bare assignments, and TypeError/OverflowError instead of ValueError. Co-Authored-By: Claude Opus 4.6 <[email protected]>
This comment has been minimized.
This comment has been minimized.
Rewrite except clauses to catch ValueError (the exception int() actually raises for bad input) instead of (TypeError, OverflowError). Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Updated — switched to catching |
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Pandas example in the primer comment is a new false positive.
Only move bare type annotations to consumed_uncertain when the uncertainty comes from except/try blocks, not from if/elif tests. If/elif branches are guaranteed to execute one path, so a bare annotation preceding them should not trigger possibly-used-before-assignment. Adds regression test for the pandas pattern where a bare annotation precedes an if/elif chain covering all runtime values. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Good catch — fixed. The bare annotation filter was too broad: it moved bare annotations to The fix tracks whether uncertainty came from except/try blocks specifically (where execution may genuinely not happen) vs if/elif tests (where one branch will always execute). Bare annotations are only demoted when the uncertainty source is except/try. Added a regression test for the pandas pattern ( |
Inline the has_except_uncertainty flag update into single expressions to stay under the 50-statement limit. Remove redundant per-block comments since the tracking purpose is documented in the block header. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 9391b02 |
|
I'll review this in the next week or so. |
| # If this node is in a Finally block of a Try/Finally, | ||
| # filter out assignments in the try portion, assuming they may fail |
There was a problem hiding this comment.
Please revert the unnecessary comment deletions like this (there are more than one).
| uncertain_nodes = self._uncertain_nodes_in_except_blocks( | ||
| found_nodes, node, node_statement | ||
| ) | ||
| has_except_uncertainty = has_except_uncertainty or bool(uncertain_nodes) |
There was a problem hiding this comment.
I think this project's style prefers implicit booleanness.
| print(err) | ||
|
|
||
|
|
||
| def bare_annotation_with_if_elif(axis): |
There was a problem hiding this comment.
Can you add a combination test case that involves both if/elif and try/except?
Description
Fixes #10847
When a variable has a bare type annotation (e.g.
err: int) without a value and is only assigned insideexceptblocks, pylint failed to reportused-before-assignment.Reproducer
Removing the type annotation correctly triggers the warning. Adding a value (
err: int = 0) correctly suppresses it.Root cause
In
NamesConsumer.get_next_to_consume(), the except-block assignments are correctly filtered intoconsumed_uncertain. However, the bare annotation'sAssignNamenode remains infound_nodes— and since it appears before the usage in line order,_is_variable_violation()setsmaybe_before_assign = False(usage line > definition line). The bare annotation is then treated as a valid definition, suppressing the check entirely.Fix
After all uncertain-node filtering in
get_next_to_consume(), if the remainingfound_nodesconsists only of bare annotations (AnnAssignwithout a value) and there are uncertain definitions inconsumed_uncertain, filter out the bare annotations. This causesfound_nodesto be empty, allowing the code to correctly reach_report_unfound_name_definition()which emits theused-before-assignmentwarning.Test
Added two test cases to
used_before_assignment_type_annotations.py:bare_annotation_with_except_assignment()— expectsused-before-assignmentbare_annotation_with_value_and_except()— no warning expected (annotation has a value)All 872 functional tests pass (13 skipped). The one pre-existing failure (
use_yield_from) is unrelated.Type of Changes