Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Run shadowed-variable analyses in deferred handlers (#5181) 

## Summary

This PR extracts a bunch of complex logic from `add_binding`, instead
running the the shadowing rules in the deferred handler, thereby
decoupling the binding phase (during which we build up the semantic
model) from the analysis phase, and generally making `add_binding` much
more focused.

This was made possible by improving the semantic model to better handle
deletions -- previously, we'd "lose track" of bindings if they were
deleted, which made this kind of refactor impossible.

## Test Plan

We have good automated coverage for this, but I want to benchmark it
separately.
This commit is contained in:
Charlie Marsh 2023-06-28 20:08:18 -04:00 committed by GitHub
parent 139a9f757b
commit 0e89c94947
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 222 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

233
crates/ruff/src/checkers/ast/mod.rs
View File

@ -4153,88 +4153,6 @@ impl<'a> Checker<'a> {
// Create the `Binding`.
let binding_id = self.semantic.push_binding(range, kind, flags);
let binding = self.semantic.binding(binding_id);
// Determine whether the binding shadows any existing bindings.
if let Some((stack_index, shadowed_id)) = self
.semantic
.scopes
.ancestors(self.semantic.scope_id)
.enumerate()
.find_map(|(stack_index, scope)| {
scope.get(name).and_then(|binding_id| {
let binding = self.semantic.binding(binding_id);
if binding.is_unbound() {
None
} else {
Some((stack_index, binding_id))
}
})
})
{
let shadowed = self.semantic.binding(shadowed_id);
let in_current_scope = stack_index == 0;
if !shadowed.kind.is_builtin()
&& shadowed.source.map_or(true, |left| {
binding.source.map_or(true, |right| {
!branch_detection::different_forks(left, right, &self.semantic.stmts)
})
})
{
let shadows_import = matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
);
if binding.kind.is_loop_var() && shadows_import {
if self.enabled(Rule::ImportShadowedByLoopVar) {
#[allow(deprecated)]
let line = self.locator.compute_line_index(shadowed.range.start());
self.diagnostics.push(Diagnostic::new(
pyflakes::rules::ImportShadowedByLoopVar {
name: name.to_string(),
line,
},
binding.range,
));
}
} else if in_current_scope {
if !shadowed.is_used()
&& binding.redefines(shadowed)
&& (!self.settings.dummy_variable_rgx.is_match(name) || shadows_import)
&& !(shadowed.kind.is_function_definition()
&& visibility::is_overload(
cast::decorator_list(self.semantic.stmts[shadowed.source.unwrap()]),
&self.semantic,
))
{
if self.enabled(Rule::RedefinedWhileUnused) {
#[allow(deprecated)]
let line = self.locator.compute_line_index(shadowed.range.start());
let mut diagnostic = Diagnostic::new(
pyflakes::rules::RedefinedWhileUnused {
name: name.to_string(),
line,
},
binding.range,
);
if let Some(range) = binding.parent_range(&self.semantic) {
diagnostic.set_parent(range.start());
}
self.diagnostics.push(diagnostic);
}
}
} else if shadows_import && binding.redefines(shadowed) {
self.semantic
.shadowed_bindings
.insert(binding_id, shadowed_id);
}
}
}
// If there's an existing binding in this scope, copy its references.
if let Some(shadowed_id) = self.semantic.scopes[scope_id].get(name) {
@ -4268,6 +4186,21 @@ impl<'a> Checker<'a> {
self.semantic.bindings[binding_id].references = references;
}
} else if let Some(shadowed_id) = self
.semantic
.scopes
.ancestors(scope_id)
.skip(1)
.find_map(|scope| scope.get(name))
{
// Otherwise, if there's an existing binding in a parent scope, mark it as shadowed.
let binding = self.semantic.binding(binding_id);
let shadowed = self.semantic.binding(shadowed_id);
if binding.redefines(shadowed) {
self.semantic
.shadowed_bindings
.insert(binding_id, shadowed_id);
}
}
// Add the binding to the scope.
@ -4286,7 +4219,7 @@ impl<'a> Checker<'a> {
{
// Add the builtin to the scope.
let binding_id = self.semantic.push_builtin();
let scope = self.semantic.scope_mut();
let scope = self.semantic.global_scope_mut();
scope.add(builtin, binding_id);
}
}
@ -4690,17 +4623,19 @@ impl<'a> Checker<'a> {
fn check_deferred_scopes(&mut self) {
if !self.any_enabled(&[
Rule::UnusedImport,
Rule::GlobalVariableNotAssigned,
Rule::UndefinedLocalWithImportStarUsage,
Rule::ImportShadowedByLoopVar,
Rule::RedefinedWhileUnused,
Rule::RuntimeImportInTypeCheckingBlock,
Rule::TypingOnlyFirstPartyImport,
Rule::TypingOnlyThirdPartyImport,
Rule::TypingOnlyStandardLibraryImport,
Rule::UndefinedExport,
Rule::TypingOnlyThirdPartyImport,
Rule::UnaliasedCollectionsAbcSetImport,
Rule::UnconventionalImportAlias,
Rule::UndefinedExport,
Rule::UndefinedLocalWithImportStarUsage,
Rule::UndefinedLocalWithImportStarUsage,
Rule::UnusedImport,
]) {
return;
}
@ -4767,8 +4702,8 @@ impl<'a> Checker<'a> {
};
let mut diagnostics: Vec<Diagnostic> = vec![];
for scope_id in self.deferred.scopes.iter().rev() {
let scope = &self.semantic.scopes[*scope_id];
for scope_id in self.deferred.scopes.iter().rev().copied() {
let scope = &self.semantic.scopes[scope_id];
if scope.kind.is_module() {
// F822
@ -4827,21 +4762,123 @@ impl<'a> Checker<'a> {
continue;
}
// Look for any bindings that were redefined in another scope, and remain
// unused. Note that we only store references in `shadowed_bindings` if
// the bindings are in different scopes.
if self.enabled(Rule::RedefinedWhileUnused) {
// F402
if self.enabled(Rule::ImportShadowedByLoopVar) {
for (name, binding_id) in scope.bindings() {
if let Some(shadowed_id) = self.semantic.shadowed_binding(binding_id) {
let shadowed = self.semantic.binding(shadowed_id);
if shadowed.is_used() {
for shadow in self.semantic.shadowed_bindings(scope_id, binding_id) {
// If the shadowing binding isn't a loop variable, abort.
let binding = &self.semantic.bindings[shadow.binding_id()];
if !binding.kind.is_loop_var() {
continue;
}
// If the shadowed binding isn't an import, abort.
let shadowed = &self.semantic.bindings[shadow.shadowed_id()];
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) {
continue;
}
// If the bindings are in different forks, abort.
if shadowed.source.map_or(true, |left| {
binding.source.map_or(true, |right| {
branch_detection::different_forks(left, right, &self.semantic.stmts)
})
}) {
continue;
}
#[allow(deprecated)]
let line = self.locator.compute_line_index(shadowed.range.start());
let binding = self.semantic.binding(binding_id);
self.diagnostics.push(Diagnostic::new(
pyflakes::rules::ImportShadowedByLoopVar {
name: name.to_string(),
line,
},
binding.range,
));
}
}
}
// F811
if self.enabled(Rule::RedefinedWhileUnused) {
for (name, binding_id) in scope.bindings() {
for shadow in self.semantic.shadowed_bindings(scope_id, binding_id) {
// If the shadowing binding is a loop variable, abort, to avoid overlap
// with F402.
let binding = &self.semantic.bindings[shadow.binding_id()];
if binding.kind.is_loop_var() {
continue;
}
// If the shadowed binding is used, abort.
let shadowed = &self.semantic.bindings[shadow.shadowed_id()];
if shadowed.is_used() {
continue;
}
// If the shadowing binding isn't considered a "redefinition" of the
// shadowed binding, abort.
if !binding.redefines(shadowed) {
continue;
}
if shadow.same_scope() {
// If the symbol is a dummy variable, abort, unless the shadowed
// binding is an import.
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) && self.settings.dummy_variable_rgx.is_match(name)
{
continue;
}
// If this is an overloaded function, abort.
if shadowed.kind.is_function_definition()
&& visibility::is_overload(
cast::decorator_list(
self.semantic.stmts[shadowed.source.unwrap()],
),
&self.semantic,
)
{
continue;
}
} else {
// Only enforce cross-scope shadowing for imports.
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) {
continue;
}
}
// If the bindings are in different forks, abort.
if shadowed.source.map_or(true, |left| {
binding.source.map_or(true, |right| {
branch_detection::different_forks(left, right, &self.semantic.stmts)
})
}) {
continue;
}
#[allow(deprecated)]
let line = self.locator.compute_line_index(shadowed.range.start());
let mut diagnostic = Diagnostic::new(
pyflakes::rules::RedefinedWhileUnused {
name: (*name).to_string(),
@ -4863,7 +4900,7 @@ impl<'a> Checker<'a> {
} else {
self.semantic
.scopes
.ancestor_ids(*scope_id)
.ancestor_ids(scope_id)
.flat_map(|scope_id| runtime_imports[scope_id.as_usize()].iter())
.copied()
.collect()

6
crates/ruff/src/rules/pyflakes/mod.rs
View File

@ -282,7 +282,7 @@ mod tests {
import os
# Despite this `del`, `import os` in `f` should still be flagged as shadowing an unused
# import. (This is a false negative.)
# import.
del os
"#,
"del_shadowed_global_import_in_local_scope"
@ -323,7 +323,7 @@ mod tests {
del os
def g():
# `import os` should still be flagged as shadowing an import.
# `import os` doesn't need to be flagged as shadowing an import.
os = 1
print(os)
"#,
@ -2114,7 +2114,7 @@ mod tests {
try: pass
except Exception as fu: pass
"#,
&[Rule::RedefinedWhileUnused, Rule::UnusedVariable],
&[Rule::UnusedVariable, Rule::RedefinedWhileUnused],
);
}

9
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_global_import_in_local_scope.snap
View File

@ -17,4 +17,13 @@ source: crates/ruff/src/rules/pyflakes/mod.rs
4 3 | def f():
5 4 | import os
<filename>:5:12: F811 Redefinition of unused `os` from line 2
|
4 | def f():
5 | import os
| ^^ F811
6 |
7 | # Despite this `del`, `import os` in `f` should still be flagged as shadowing an unused
|

15
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_import_shadow_in_local_scope.snap
View File

@ -17,13 +17,12 @@ source: crates/ruff/src/rules/pyflakes/mod.rs
4 3 | def f():
5 4 | os = 1
<filename>:12:9: F811 Redefinition of unused `os` from line 2
|
10 | def g():
11 | # `import os` should still be flagged as shadowing an import.
12 | os = 1
| ^^ F811
13 | print(os)
|
<filename>:5:5: F811 Redefinition of unused `os` from line 2
|
4 | def f():
5 | os = 1
| ^^ F811
6 | print(os)
|

63
crates/ruff_python_semantic/src/model.rs
View File

@ -1052,6 +1052,69 @@ impl<'a> SemanticModel<'a> {
pub const fn future_annotations(&self) -> bool {
self.flags.contains(SemanticModelFlags::FUTURE_ANNOTATIONS)
}
/// Return an iterator over all bindings shadowed by the given [`BindingId`], within the
/// containing scope, and across scopes.
pub fn shadowed_bindings(
&self,
scope_id: ScopeId,
binding_id: BindingId,
) -> impl Iterator<Item = ShadowedBinding> + '_ {
let mut first = true;
let mut binding_id = binding_id;
std::iter::from_fn(move || {
// First, check whether this binding is shadowing another binding in a different scope.
if std::mem::take(&mut first) {
if let Some(shadowed_id) = self.shadowed_bindings.get(&binding_id).copied() {
return Some(ShadowedBinding {
binding_id,
shadowed_id,
same_scope: false,
});
}
}
// Otherwise, check whether this binding is shadowing another binding in the same scope.
if let Some(shadowed_id) = self.scopes[scope_id].shadowed_binding(binding_id) {
let next = ShadowedBinding {
binding_id,
shadowed_id,
same_scope: true,
};
// Advance to the next binding in the scope.
first = true;
binding_id = shadowed_id;
return Some(next);
}
None
})
}
}
pub struct ShadowedBinding {
/// The binding that is shadowing another binding.
binding_id: BindingId,
/// The binding that is being shadowed.
shadowed_id: BindingId,
/// Whether the shadowing and shadowed bindings are in the same scope.
same_scope: bool,
}
impl ShadowedBinding {
pub const fn binding_id(&self) -> BindingId {
self.binding_id
}
pub const fn shadowed_id(&self) -> BindingId {
self.shadowed_id
}
pub const fn same_scope(&self) -> bool {
self.same_scope
}
}
bitflags! {

5
crates/ruff_python_semantic/src/scope.rs
View File

@ -126,6 +126,11 @@ impl<'a> Scope<'a> {
})
}
/// Returns the ID of the binding that the given binding shadows, if any.
pub fn shadowed_binding(&self, id: BindingId) -> Option<BindingId> {
self.shadowed_bindings.get(&id).copied()
}
/// Adds a reference to a star import (e.g., `from sys import *`) to this scope.
pub fn add_star_import(&mut self, import: StarImport<'a>) {
self.star_imports.push(import);