fix: don't flag names as unused when read before their registering assignment (#3911)#4074
fix: don't flag names as unused when read before their registering assignment (#3911)#4074AhmedHedi0 wants to merge 1 commit into
Conversation
…signment Unused-variable tracking only registers a name in `register_variable` at the moment a single-name assignment binds it. Any read of that name encountered earlier in traversal order — e.g. a loop condition or body that reads a name only ever assigned inside the loop, or a name only ever assigned via tuple/list unpacking, `for`, `with`, or walrus targets (none of which call `register_variable`) — was silently dropped by `mark_variable_used`, since it only marked names already present in `variables`. This produced false-positive "unused variable" diagnostics, as in facebook#3911, where a `while` loop reads and reassigns two names unpacked from a tuple. Give `mark_variable_used` its own scope resolution, mirroring `look_up_name_for_read`'s walk (skip enclosing class scopes, skip walrus targets not yet visible in a comprehension's flow, skip `global`/`nonlocal` captures) but keyed off static scope info instead of flow, since static scope already knows every name a scope will eventually bind. When a read resolves to a name that is statically local to a module/function/method scope but not yet registered, record it in a new `used_variables_before_registration` set. `register_variable` now checks that set (alongside the existing `used` flag) when seeding a newly registered variable's initial `used` state. This closes the gap for reads that occur before their name's only registering assignment, without changing behavior for the common case where a read follows registration.
|
Hi @AhmedHedi0! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
1 participant
Summary
Fixes #3911
Unused-variable tracking only registers a name in
register_variableat themoment a single-name assignment binds it. Reads of a name encountered
before its only registering assignment e.g. a
whileloop condition orbody reading a name that's only ever assigned inside the loop, or a name
that's only ever assigned via tuple/list unpacking,
for,with, or walrustargets (none of which call
register_variable) were silently dropped bymark_variable_used, since it only marked names already present invariables. That produced the false-positive "unused variable" diagnostic in#3911, where a
whileloop reads and reassigns two names originally boundvia tuple unpacking.
The fix gives
mark_variable_usedits own scope-resolution walk, mirroringlook_up_name_for_read's logic (skip enclosing class scopes from nested codeblocks, skip walrus targets not yet visible in a comprehension's flow, skip
global/nonlocalcaptures), but keyed off static scope info rather thanflow, since static scope already knows every name a scope will eventually
bind. When a read resolves to a name that's statically local to a
module/function/method scope but not yet registered, it's recorded in a new
used_variables_before_registrationset, whichregister_variablechecks(alongside the existing
usedflag) when seeding a newly registeredvariable's initial
usedstate. This closes the gap for reads occurringbefore their name's only registering assignment, without changing behavior
for the common case where a read follows registration.
Note: I noticed the issue already has an assignee with what may be an
in-progress PR. Opening this in case the approach here is useful for
comparison, happy to defer to whichever fix maintainers prefer, or to have
this closed if it's redundant.
AI disclosure: this PR was developed with AI assistance under my direction
and review. I wrote none of the diff by hand, but reviewed the root-cause
analysis, caught and had it fix a regression in an early version(false-positive
unused-variable when a name is shadowed by an enclosingclass's field),
and verified the fix and test results myself before opening this PR.
Test Plan
pyrefly/lib/test/lsp/diagnostic.rscovering theoriginal repro (
test_generator_loop_reassignments), the same patternwithout a generator to confirm the bug isn't generator-specific
(
test_unpacked_target_reassigned_in_loop), plusfor/with/walrus/comprehension/nonlocal cases exercising the same read-before-registration
code path.
test_method_read_of_name_shadowed_by_class_fieldto guard against aregression caught during review, where an early version of the fix broke
name resolution across class-body boundaries.
python3 test.pyin full (fmt, lint, tests, conformance, jsonschema).All pass except 16 pre-existing
test::shape_dsl::*timeout failures("Test took too long (> 20s)"), which I confirmed are unrelated to this
change by reproducing the identical failure set on unmodified
main.this fix.