diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index b7b0d1e925..012d999ea3 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -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 = 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() diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs index 243be429ce..0bdbeaee1d 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -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], ); } diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_global_import_in_local_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_global_import_in_local_scope.snap index 37beaccce1..b2b02e92a7 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_global_import_in_local_scope.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_global_import_in_local_scope.snap @@ -17,4 +17,13 @@ source: crates/ruff/src/rules/pyflakes/mod.rs 4 3 | def f(): 5 4 | import os +: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 + | + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_import_shadow_in_local_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_import_shadow_in_local_scope.snap index d8e62a9c2e..747002c934 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_import_shadow_in_local_scope.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__del_shadowed_import_shadow_in_local_scope.snap @@ -17,13 +17,12 @@ source: crates/ruff/src/rules/pyflakes/mod.rs 4 3 | def f(): 5 4 | os = 1 -: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) - | +:5:5: F811 Redefinition of unused `os` from line 2 + | +4 | def f(): +5 | os = 1 + | ^^ F811 +6 | print(os) + | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index d39d36def7..619020ac12 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -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 + '_ { + 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! { diff --git a/crates/ruff_python_semantic/src/scope.rs b/crates/ruff_python_semantic/src/scope.rs index f64148b56e..04efe0e92f 100644 --- a/crates/ruff_python_semantic/src/scope.rs +++ b/crates/ruff_python_semantic/src/scope.rs @@ -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 { + 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);