diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 08f9508c85..5e1c0b127b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4342,98 +4342,13 @@ impl<'a> Checker<'a> { // Create the `Binding`. let binding_id = self.semantic_model.push_binding(range, kind, flags); - let binding = &self.semantic_model.bindings[binding_id]; - // Determine whether the binding shadows any existing bindings. - if let Some((stack_index, shadowed_id)) = self - .semantic_model - .scopes - .ancestors(self.semantic_model.scope_id) - .enumerate() - .find_map(|(stack_index, scope)| { - scope.get(name).map(|binding_id| (stack_index, binding_id)) - }) - { - let shadowed = &self.semantic_model.bindings[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_model.stmts) - }) - }) - { - let shadows_import = matches!( - shadowed.kind, - BindingKind::Importation(..) - | BindingKind::FromImportation(..) - | BindingKind::SubmoduleImportation(..) - | BindingKind::FutureImportation - ); - 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() - && analyze::visibility::is_overload( - &self.semantic_model, - cast::decorator_list( - self.semantic_model.stmts[shadowed.source.unwrap()], - ), - )) - { - if self.enabled(Rule::RedefinedWhileUnused) { - #[allow(deprecated)] - let line = self.locator.compute_line_index( - shadowed - .trimmed_range(&self.semantic_model, self.locator) - .start(), - ); - - let mut diagnostic = Diagnostic::new( - pyflakes::rules::RedefinedWhileUnused { - name: name.to_string(), - line, - }, - binding.trimmed_range(&self.semantic_model, self.locator), - ); - if let Some(parent) = binding.source { - let parent = self.semantic_model.stmts[parent]; - if parent.is_import_from_stmt() - && parent.range().contains_range(binding.range) - { - diagnostic.set_parent(parent.start()); - } - } - self.diagnostics.push(diagnostic); - } - } - } else if shadows_import && binding.redefines(shadowed) { - self.semantic_model - .shadowed_bindings - .insert(binding_id, shadowed_id); - } - } - } - - // If there's an existing binding in this scope, copy its references. + // Add the `Binding` to the current scope. if let Some(shadowed) = self.semantic_model.scopes[scope_id] .get(name) .map(|binding_id| &self.semantic_model.bindings[binding_id]) { + // If there's an existing binding in this scope, copy its references. match &shadowed.kind { BindingKind::Builtin => { // Avoid overriding builtins. @@ -4459,6 +4374,17 @@ impl<'a> Checker<'a> { { return binding_id; } + } else if let Some(shadowed_id) = self + .semantic_model + .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. + self.semantic_model + .shadowed_bindings + .insert(binding_id, shadowed_id); } // Add the binding to the scope. @@ -4477,7 +4403,7 @@ impl<'a> Checker<'a> { { // Add the builtin to the scope. let binding_id = self.semantic_model.push_builtin(); - let scope = self.semantic_model.scope_mut(); + let scope = self.semantic_model.global_scope_mut(); scope.add(builtin, binding_id); } } @@ -4889,10 +4815,11 @@ impl<'a> Checker<'a> { if !(enforce_typing_imports || self.any_enabled(&[ - Rule::UnusedImport, - Rule::UndefinedLocalWithImportStarUsage, + Rule::ImportShadowedByLoopVar, Rule::RedefinedWhileUnused, Rule::UndefinedExport, + Rule::UndefinedLocalWithImportStarUsage, + Rule::UnusedImport, ])) { return; @@ -4953,8 +4880,8 @@ impl<'a> Checker<'a> { }; let mut diagnostics: Vec = vec![]; - for scope_id in self.semantic_model.dead_scopes.iter().rev() { - let scope = &self.semantic_model.scopes[*scope_id]; + for scope_id in self.semantic_model.dead_scopes.iter().rev().copied() { + let scope = &self.semantic_model.scopes[scope_id]; if scope.kind.is_module() { // F822 @@ -5013,22 +4940,135 @@ impl<'a> Checker<'a> { } } + // F402 + if self.enabled(Rule::ImportShadowedByLoopVar) { + for (name, binding_id) in scope.bindings() { + for shadow in self.semantic_model.shadowed_bindings(scope_id, binding_id) { + // If the shadowing binding isn't a loop variable, abort. + let binding = &self.semantic_model.bindings[shadow.binding_id()]; + if !binding.kind.is_loop_var() { + continue; + } + + // If the shadowed binding isn't an import, abort. + let shadowed = &self.semantic_model.bindings[shadow.shadowed_id()]; + if !matches!( + shadowed.kind, + BindingKind::Importation(..) + | BindingKind::FromImportation(..) + | BindingKind::SubmoduleImportation(..) + | BindingKind::FutureImportation + ) { + 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_model.stmts, + ) + }) + }) { + continue; + } + + #[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, + )); + } + } + } + // Imports in classes are public members. if scope.kind.is_class() { 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. + // F811 if self.enabled(Rule::RedefinedWhileUnused) { for (name, binding_id) in scope.bindings() { - if let Some(shadowed) = self.semantic_model.shadowed_binding(binding_id) { + for shadow in self.semantic_model.shadowed_bindings(scope_id, binding_id) { + // If the shadowing binding is a loop variable, abort, to avoid overlap + // with F402. + let binding = &self.semantic_model.bindings[shadow.binding_id()]; + if binding.kind.is_loop_var() { + continue; + } + + // If the shadowed binding is used, abort. + let shadowed = &self.semantic_model.bindings[shadow.shadowed_id()]; if shadowed.is_used() { continue; } - let binding = &self.semantic_model.bindings[binding_id]; + // If the shadowing binding isn't considered a "redifinition" 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::Importation(..) + | BindingKind::FromImportation(..) + | BindingKind::SubmoduleImportation(..) + | BindingKind::FutureImportation + ) && self.settings.dummy_variable_rgx.is_match(name) + { + continue; + } + + // If this is an overloaded function, abort. + if shadowed.kind.is_function_definition() + && analyze::visibility::is_overload( + &self.semantic_model, + cast::decorator_list( + self.semantic_model.stmts[shadowed.source.unwrap()], + ), + ) + { + continue; + } + } else { + // Only enforce cross-scope shadowing for imports. + if !matches!( + shadowed.kind, + BindingKind::Importation(..) + | BindingKind::FromImportation(..) + | BindingKind::SubmoduleImportation(..) + | BindingKind::FutureImportation + ) { + continue; + } + } + + // If the bindings are in different forks, abort. + 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_model.stmts, + ) + }) + }) + { + continue; + } #[allow(deprecated)] let line = self.locator.compute_line_index( @@ -5063,7 +5103,7 @@ impl<'a> Checker<'a> { } else { self.semantic_model .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 fe890582f6..3fb1afa73e 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -1890,7 +1890,7 @@ mod tests { try: pass except Exception as fu: pass "#, - &[Rule::RedefinedWhileUnused, Rule::UnusedVariable], + &[Rule::UnusedVariable, Rule::RedefinedWhileUnused], ); } diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index dd862e8ce6..c7538bc437 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -94,7 +94,6 @@ impl<'a> Binding<'a> { existing.kind, BindingKind::ClassDefinition | BindingKind::FunctionDefinition - | BindingKind::Builtin | BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 7b23c81b6a..3be600d13c 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -805,6 +805,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 09cadf47cf..00612ba052 100644 --- a/crates/ruff_python_semantic/src/scope.rs +++ b/crates/ruff_python_semantic/src/scope.rs @@ -87,6 +87,11 @@ impl<'a> Scope<'a> { self.bindings.iter().map(|(&name, &id)| (name, id)) } + /// Returns a tuple of the name and id of all bindings defined in this scope. + pub fn shadowed_binding(&self, id: BindingId) -> Option { + self.shadowed_bindings.get(&id).copied() + } + /// Returns an iterator over all [bindings](BindingId) bound to the given name, including /// those that were shadowed by later bindings. pub fn bindings_for_name(&self, name: &str) -> impl Iterator + '_ {