Only run unused private type rules over finalized bindings #6142
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/shadowed
branch
from
507e8a0
to
a771c18
Compare
Merged
LaBatata101
added a commit
to LaBatata101/ruff
that referenced
this pull request
Jul 28, 2023
These changes are based on astral-sh#6142
LaBatata101
pushed a commit
to LaBatata101/ruff
that referenced
this pull request
Jul 28, 2023
…#6142) In astral-sh#6134 and astral-sh#6136, we see some false positives for "shadowed" class definitions. For example, here, the first definition is flagged as unused, since from the perspective of the semantic model (which doesn't understand branching), it appears to be immediately shadowed in the `else`, and thus never used: ```python if sys.version_info >= (3, 11): class _RootLoggerConfiguration(TypedDict, total=False): level: _Level filters: Sequence[str | _FilterType] handlers: Sequence[str] else: class _RootLoggerConfiguration(TypedDict, total=False): level: _Level filters: Sequence[str] handlers: Sequence[str] ``` Instead of looking at _all_ bindings, we should instead look at the "live" bindings, which is similar to how other rules (like unused variables detection) is structured. We thus move the rule from `bindings.rs` (which iterates over _all_ bindings, regardless of whether they're shadowed) to `deferred_scopes.rs`, which iterates over all "live" bindings once a scope has been fully analyzed. `cargo test`
LaBatata101
added a commit
to LaBatata101/ruff
that referenced
this pull request
Jul 28, 2023
These changes are based on astral-sh#6142
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In #6134 and #6136, we see some false positives for "shadowed" class definitions. For example, here, the first definition is flagged as unused, since from the perspective of the semantic model (which doesn't understand branching), it appears to be immediately shadowed in the
else, and thus never used:Instead of looking at all bindings, we should instead look at the "live" bindings, which is similar to how other rules (like unused variables detection) is structured. We thus move the rule from
bindings.rs(which iterates over all bindings, regardless of whether they're shadowed) todeferred_scopes.rs, which iterates over all "live" bindings once a scope has been fully analyzed.Test Plan
cargo test