From f4173b2a93a568c2b9ff356c8e0aff29cc9e45e8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 3 Apr 2023 15:33:44 -0400 Subject: [PATCH] Move shadow tracking into `Scope` directly (#3854) --- crates/ruff/src/checkers/ast/mod.rs | 12 ++------ .../rules/unused_loop_control_variable.rs | 18 +++++------- crates/ruff_python_ast/src/context.rs | 6 ++-- crates/ruff_python_ast/src/scope.rs | 28 +++++++++++++------ 4 files changed, 33 insertions(+), 31 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 010d1e587c..1042c643d3 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4081,7 +4081,7 @@ impl<'a> Checker<'a> { } } else if existing_is_import && binding.redefines(existing) { self.ctx - .redefinitions + .shadowed_bindings .entry(existing_binding_index) .or_insert_with(Vec::new) .push(binding_id); @@ -4123,13 +4123,7 @@ impl<'a> Checker<'a> { // in scope. let scope = self.ctx.scope_mut(); if !(binding.kind.is_annotation() && scope.defines(name)) { - if let Some(rebound_index) = scope.add(name, binding_id) { - scope - .rebounds - .entry(name) - .or_insert_with(Vec::new) - .push(rebound_index); - } + scope.add(name, binding_id); } self.ctx.bindings.push(binding); @@ -4919,7 +4913,7 @@ impl<'a> Checker<'a> { continue; } - if let Some(indices) = self.ctx.redefinitions.get(index) { + if let Some(indices) = self.ctx.shadowed_bindings.get(index) { for index in indices { let rebound = &self.ctx.bindings[*index]; let mut diagnostic = Diagnostic::new( diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs b/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs index 4b338d6f59..bf74cc096c 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs @@ -166,17 +166,13 @@ pub fn unused_loop_control_variable( if certainty.into() && checker.patch(diagnostic.kind.rule()) { // Find the `BindingKind::LoopVar` corresponding to the name. let scope = checker.ctx.scope(); - let binding = scope - .get(name) - .into_iter() - .chain(scope.rebounds.get(name).into_iter().flatten()) - .find_map(|index| { - let binding = &checker.ctx.bindings[*index]; - binding - .source - .as_ref() - .and_then(|source| (source == &RefEquality(stmt)).then_some(binding)) - }); + let binding = scope.bindings_for_name(name).find_map(|index| { + let binding = &checker.ctx.bindings[*index]; + binding + .source + .as_ref() + .and_then(|source| (source == &RefEquality(stmt)).then_some(binding)) + }); if let Some(binding) = binding { if binding.kind.is_loop_var() { if !binding.used() { diff --git a/crates/ruff_python_ast/src/context.rs b/crates/ruff_python_ast/src/context.rs index 857311d44a..462bf993d7 100644 --- a/crates/ruff_python_ast/src/context.rs +++ b/crates/ruff_python_ast/src/context.rs @@ -30,8 +30,8 @@ pub struct Context<'a> { pub child_to_parent: FxHashMap, RefEquality<'a, Stmt>>, // A stack of all bindings created in any scope, at any point in execution. pub bindings: Bindings<'a>, - // Map from binding index to indices of bindings that redefine it in other scopes. - pub redefinitions: + // Map from binding index to indexes of bindings that shadow it in other scopes. + pub shadowed_bindings: std::collections::HashMap, BuildNoHashHasher>, pub exprs: Vec>, pub scopes: Scopes<'a>, @@ -71,7 +71,7 @@ impl<'a> Context<'a> { depths: FxHashMap::default(), child_to_parent: FxHashMap::default(), bindings: Bindings::default(), - redefinitions: IntMap::default(), + shadowed_bindings: IntMap::default(), exprs: Vec::default(), scopes: Scopes::default(), scope_stack: ScopeStack::default(), diff --git a/crates/ruff_python_ast/src/scope.rs b/crates/ruff_python_ast/src/scope.rs index 2224221eb8..aa7c059e55 100644 --- a/crates/ruff_python_ast/src/scope.rs +++ b/crates/ruff_python_ast/src/scope.rs @@ -14,12 +14,10 @@ pub struct Scope<'a> { /// A list of star imports in this scope. These represent _module_ imports (e.g., `sys` in /// `from sys import *`), rather than individual bindings (e.g., individual members in `sys`). star_imports: Vec>, - /// A map from bound name to binding index, for live bindings. + /// A map from bound name to binding index, for current bindings. bindings: FxHashMap<&'a str, BindingId>, - /// A map from bound name to binding index, for bindings that were created - /// in the scope but rebound (and thus overridden) later on in the same - /// scope. - pub rebounds: FxHashMap<&'a str, Vec>, + /// A map from bound name to binding index, for bindings that were shadowed later in the scope. + shadowed_bindings: FxHashMap<&'a str, Vec>, } impl<'a> Scope<'a> { @@ -34,18 +32,23 @@ impl<'a> Scope<'a> { uses_locals: false, star_imports: Vec::default(), bindings: FxHashMap::default(), - rebounds: FxHashMap::default(), + shadowed_bindings: FxHashMap::default(), } } - /// Returns the [id](BindingId) of the binding with the given name. + /// Returns the [id](BindingId) of the binding bound to the given name. pub fn get(&self, name: &str) -> Option<&BindingId> { self.bindings.get(name) } /// Adds a new binding with the given name to this scope. pub fn add(&mut self, name: &'a str, id: BindingId) -> Option { - self.bindings.insert(name, id) + if let Some(id) = self.bindings.insert(name, id) { + self.shadowed_bindings.entry(name).or_default().push(id); + Some(id) + } else { + None + } } /// Returns `true` if this scope defines a binding with the given name. @@ -68,6 +71,15 @@ impl<'a> Scope<'a> { self.bindings.iter() } + /// 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 { + self.bindings + .get(name) + .into_iter() + .chain(self.shadowed_bindings.get(name).into_iter().flatten().rev()) + } + /// Adds a reference to a star import (e.g., `from sys import *`) to this scope. pub fn add_star_import(&mut self, import: StarImportation<'a>) { self.star_imports.push(import);