[pyflakes] suppress false positive in F821 for names used before del in stub files#23550
Conversation
|
| if self.in_stub_file() { | ||
| if let Some(shadowed_id) = | ||
| self.scopes[scope_id].shadowed_binding(binding_id) | ||
| { |
There was a problem hiding this comment.
Could you add a test case like this:
class MyClass: ...
del MyClass
def f(cls: MyClass) -> None: ...This properly gives an F821 diagnostic in a .py file in the playground, but if I'm reading this code correctly, I think it would give a false negative in .pyi files on this PR branch.
I think it may end up being easier to do this check in the rule itself where we can inspect the relative ranges of all the bindings rather than modifying the semantic model, but I haven't played with the code myself.
There was a problem hiding this comment.
Good catch — added the test case and fixed the logic. The semantic model now checks name.range.start() < deletion_range.start() so we only resolve to the shadowed binding when the reference textually precedes the del. The del-before-use pattern (class MyClass: ...; del MyClass; def f2(cls: MyClass): ...) now correctly triggers F821.
I kept the fix in the semantic model for now since the range check makes it precise, but happy to move it to the rule if you still prefer that approach.
9bcd19a to
c075c86
Compare
|
Rebased onto latest main. All tests pass locally, and the diff is clean (only the 4 files from this PR). Ready for re-review! |
c075c86 to
52b3334
Compare
|
Rebased onto latest main — the ecosystem report should be clean now. Ready for another look. |
|
All CI checks are now passing. The branch is rebased on latest main and the ecosystem report is clean. Ready for re-review. |
|
Thanks for the feedback @amyreese! I've rebased onto latest main and the tests are now passing. The ecosystem report is also clean. Ready for re-review when you have a chance. |
|
Rebased onto latest main and updated snapshots — CI is now passing. Ready for re-review @amyreese. |
|
Rebased onto latest main and updated snapshots. CI is passing and the ecosystem check shows no linter changes. Ready for re-review @amyreese. |
|
Hi @amyreese, thanks for the review! The CI is now fully green (all 20 checks passing). The snapshot has been updated and the unrelated ecosystem report changes have been cleaned up. Could you take another look when you get a chance? Happy to make any further adjustments. |
…b files In `.pyi` stub files, `del SomeName` is a valid pattern used to hide symbols from re-export while the name remains valid for use in type annotations within the same file. Since annotations are deferred in stubs, the `Deletion` binding is seen at resolve time even though the reference textually precedes the `del`. Fix: when resolving a name in a stub file that maps to a `Deletion` binding, look up the shadowed (pre-`del`) binding via `scope.shadowed_binding()` and resolve to it instead. Fixes astral-sh#23539
52b3334 to
8ff28da
Compare
|
Rebased onto latest main and updated the snapshot. Also fixed the actual logic gap that ntBre caught — the range check ( |
del in stub filespyflakes] suppress false positive in F821 for names used before del in stub files
Fixes #23539
Summary
In
.pyistub files,del SomeNameis a valid pattern used to hide symbols from re-export while the name remains valid for use in type annotations within the same file. However, ruff was incorrectly reporting F821 (undefined name) for references that appear before thedelstatement.Root Cause
In
.pyifiles, type annotations are deferred (visited after the full AST traversal). By the time the deferred annotation is visited,del MyClasshas already been processed and the scope mapsMyClassto aDeletionbinding. The semantic model then treats the reference as unresolved.Fix
In stub files, when resolving a name that maps to a
Deletionbinding, look up the shadowed binding (the binding that existed before thedel) viascope.shadowed_binding(). If a valid (non-unbound) shadowed binding exists, resolve to it instead of treating the reference as unresolved.This is consistent with how
delworks in stubs — it only affects re-export visibility, not the validity of the name within the file.Test Plan
Added
F821_34.pyicovering:delafter class definition with annotation reference (no false positive)delafter variable assignment with annotation reference (no false positive)delafter base class definition with subclass reference (no false positive)All 38 existing F821 tests continue to pass.