From c74ef77e85931523ccb40108027ea85d6afe541e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 14 Jun 2023 10:07:46 -0400 Subject: [PATCH] Move binding accesses into `SemanticModel` method (#5084) --- crates/ruff/src/checkers/ast/mod.rs | 28 ++++++++----------- .../rules/unused_loop_control_variable.rs | 2 +- .../runtime_import_in_type_checking_block.rs | 2 +- .../rules/typing_only_runtime_import.rs | 2 +- .../rules/unused_arguments.rs | 25 ++++++++--------- .../rules/pyflakes/rules/undefined_local.rs | 2 +- .../rules/pyflakes/rules/unused_annotation.rs | 2 +- .../src/rules/pyflakes/rules/unused_import.rs | 2 +- .../rules/pyflakes/rules/unused_variable.rs | 2 +- .../rules/pylint/rules/global_statement.rs | 2 +- crates/ruff_python_semantic/src/binding.rs | 7 +---- crates/ruff_python_semantic/src/model.rs | 17 +++++++---- crates/ruff_python_semantic/src/reference.rs | 14 ++++++---- 13 files changed, 53 insertions(+), 54 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 6a496c39de..dbe401585a 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4208,7 +4208,7 @@ 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]; + let binding = self.semantic_model.binding(binding_id); // Determine whether the binding shadows any existing bindings. if let Some((stack_index, shadowed_id)) = self @@ -4218,7 +4218,7 @@ impl<'a> Checker<'a> { .enumerate() .find_map(|(stack_index, scope)| { scope.get(name).and_then(|binding_id| { - let binding = &self.semantic_model.bindings[binding_id]; + let binding = self.semantic_model.binding(binding_id); if binding.is_unbound() { None } else { @@ -4227,7 +4227,7 @@ impl<'a> Checker<'a> { }) }) { - let shadowed = &self.semantic_model.bindings[shadowed_id]; + let shadowed = self.semantic_model.binding(shadowed_id); let in_current_scope = stack_index == 0; if !shadowed.kind.is_builtin() && shadowed.source.map_or(true, |left| { @@ -4321,10 +4321,7 @@ impl<'a> Checker<'a> { // If this is an annotation, and we already have an existing value in the same scope, // don't treat it as an assignment (i.e., avoid adding it to the scope). - if self.semantic_model.bindings[binding_id] - .kind - .is_annotation() - { + if self.semantic_model.binding(binding_id).kind.is_annotation() { return binding_id; } } @@ -4424,7 +4421,7 @@ impl<'a> Checker<'a> { .scope() .get(id) .map_or(false, |binding_id| { - self.semantic_model.bindings[binding_id].kind.is_global() + self.semantic_model.binding(binding_id).kind.is_global() }) { pep8_naming::rules::non_lowercase_variable_in_function(self, expr, parent, id); @@ -4570,7 +4567,7 @@ impl<'a> Checker<'a> { // If the name is unbound, then it's an error. if self.enabled(Rule::UndefinedName) { - let binding = &self.semantic_model.bindings[binding_id]; + let binding = self.semantic_model.binding(binding_id); if binding.is_unbound() { self.diagnostics.push(Diagnostic::new( pyflakes::rules::UndefinedName { @@ -4734,10 +4731,7 @@ impl<'a> Checker<'a> { let parent = &self.semantic_model.scopes[scope.parent.unwrap()]; self.diagnostics .extend(flake8_unused_arguments::rules::unused_arguments( - self, - parent, - scope, - &self.semantic_model.bindings, + self, parent, scope, )); } } @@ -4826,7 +4820,7 @@ impl<'a> Checker<'a> { .map(|scope| { scope .binding_ids() - .map(|binding_id| &self.semantic_model.bindings[binding_id]) + .map(|binding_id| self.semantic_model.binding(binding_id)) .filter(|binding| { flake8_type_checking::helpers::is_valid_runtime_import( &self.semantic_model, @@ -4885,7 +4879,7 @@ impl<'a> Checker<'a> { // PLW0602 if self.enabled(Rule::GlobalVariableNotAssigned) { for (name, binding_id) in scope.bindings() { - let binding = &self.semantic_model.bindings[binding_id]; + let binding = self.semantic_model.binding(binding_id); if binding.kind.is_global() { if let Some(source) = binding.source { let stmt = &self.semantic_model.stmts[source]; @@ -4913,7 +4907,7 @@ impl<'a> Checker<'a> { if self.enabled(Rule::RedefinedWhileUnused) { for (name, binding_id) in scope.bindings() { if let Some(shadowed_id) = self.semantic_model.shadowed_binding(binding_id) { - let shadowed = &self.semantic_model.bindings[shadowed_id]; + let shadowed = self.semantic_model.binding(shadowed_id); if shadowed.is_used() { continue; } @@ -4925,7 +4919,7 @@ impl<'a> Checker<'a> { .start(), ); - let binding = &self.semantic_model.bindings[binding_id]; + let binding = self.semantic_model.binding(binding_id); let mut diagnostic = Diagnostic::new( pyflakes::rules::RedefinedWhileUnused { name: (*name).to_string(), 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 ad64d880b6..6d813426a1 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 @@ -157,7 +157,7 @@ pub(crate) fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, let scope = checker.semantic_model().scope(); if scope .bindings_for_name(name) - .map(|binding_id| &checker.semantic_model().bindings[binding_id]) + .map(|binding_id| checker.semantic_model().binding(binding_id)) .all(|binding| !binding.is_used()) { diagnostic.set_fix(Fix::suggested(Edit::range_replacement( diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index c4edc8b8d5..7706c1ae51 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -75,7 +75,7 @@ pub(crate) fn runtime_import_in_type_checking_block( let mut ignores_by_statement: FxHashMap> = FxHashMap::default(); for binding_id in scope.binding_ids() { - let binding = &checker.semantic_model().bindings[binding_id]; + let binding = checker.semantic_model().binding(binding_id); let Some(qualified_name) = binding.qualified_name() else { continue; diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index c509e252db..87acdf2b1f 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -197,7 +197,7 @@ pub(crate) fn typing_only_runtime_import( FxHashMap::default(); for binding_id in scope.binding_ids() { - let binding = &checker.semantic_model().bindings[binding_id]; + let binding = checker.semantic_model().binding(binding_id); // If we're in un-strict mode, don't flag typing-only imports that are // implicitly loaded by way of a valid runtime import. diff --git a/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs b/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs index f410b822cc..5e571e2d59 100644 --- a/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs +++ b/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs @@ -10,7 +10,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::analyze::function_type::FunctionType; use ruff_python_semantic::analyze::visibility; -use ruff_python_semantic::binding::Bindings; +use ruff_python_semantic::model::SemanticModel; use ruff_python_semantic::scope::{Scope, ScopeKind}; use crate::checkers::ast::Checker; @@ -221,7 +221,7 @@ fn function( argumentable: Argumentable, args: &Arguments, values: &Scope, - bindings: &Bindings, + model: &SemanticModel, dummy_variable_rgx: &Regex, ignore_variadic_names: bool, ) -> Vec { @@ -240,7 +240,7 @@ fn function( .flatten() .skip(usize::from(ignore_variadic_names)), ); - call(argumentable, args, values, bindings, dummy_variable_rgx) + call(argumentable, args, values, model, dummy_variable_rgx) } /// Check a method for unused arguments. @@ -248,7 +248,7 @@ fn method( argumentable: Argumentable, args: &Arguments, values: &Scope, - bindings: &Bindings, + model: &SemanticModel, dummy_variable_rgx: &Regex, ignore_variadic_names: bool, ) -> Vec { @@ -268,21 +268,21 @@ fn method( .flatten() .skip(usize::from(ignore_variadic_names)), ); - call(argumentable, args, values, bindings, dummy_variable_rgx) + call(argumentable, args, values, model, dummy_variable_rgx) } fn call<'a>( argumentable: Argumentable, args: impl Iterator, values: &Scope, - bindings: &Bindings, + model: &SemanticModel, dummy_variable_rgx: &Regex, ) -> Vec { let mut diagnostics: Vec = vec![]; for arg in args { if let Some(binding) = values .get(arg.arg.as_str()) - .map(|binding_id| &bindings[binding_id]) + .map(|binding_id| model.binding(binding_id)) { if binding.kind.is_argument() && !binding.is_used() @@ -303,7 +303,6 @@ pub(crate) fn unused_arguments( checker: &Checker, parent: &Scope, scope: &Scope, - bindings: &Bindings, ) -> Vec { match &scope.kind { ScopeKind::Function(ast::StmtFunctionDef { @@ -336,7 +335,7 @@ pub(crate) fn unused_arguments( Argumentable::Function, args, scope, - bindings, + checker.semantic_model(), &checker.settings.dummy_variable_rgx, checker .settings @@ -362,7 +361,7 @@ pub(crate) fn unused_arguments( Argumentable::Method, args, scope, - bindings, + checker.semantic_model(), &checker.settings.dummy_variable_rgx, checker .settings @@ -388,7 +387,7 @@ pub(crate) fn unused_arguments( Argumentable::ClassMethod, args, scope, - bindings, + checker.semantic_model(), &checker.settings.dummy_variable_rgx, checker .settings @@ -414,7 +413,7 @@ pub(crate) fn unused_arguments( Argumentable::StaticMethod, args, scope, - bindings, + checker.semantic_model(), &checker.settings.dummy_variable_rgx, checker .settings @@ -433,7 +432,7 @@ pub(crate) fn unused_arguments( Argumentable::Lambda, args, scope, - bindings, + checker.semantic_model(), &checker.settings.dummy_variable_rgx, checker .settings diff --git a/crates/ruff/src/rules/pyflakes/rules/undefined_local.rs b/crates/ruff/src/rules/pyflakes/rules/undefined_local.rs index 312e5a1d77..dae0af9655 100644 --- a/crates/ruff/src/rules/pyflakes/rules/undefined_local.rs +++ b/crates/ruff/src/rules/pyflakes/rules/undefined_local.rs @@ -68,7 +68,7 @@ pub(crate) fn undefined_local(checker: &mut Checker, name: &str) { // If the name was defined in that scope... if let Some(binding) = scope .get(name) - .map(|binding_id| &checker.semantic_model().bindings[binding_id]) + .map(|binding_id| checker.semantic_model().binding(binding_id)) { // And has already been accessed in the current scope... if let Some(range) = binding.references().find_map(|reference_id| { diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs b/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs index 44e96c53f6..1691dbe01e 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs @@ -39,7 +39,7 @@ pub(crate) fn unused_annotation(checker: &mut Checker, scope: ScopeId) { let bindings: Vec<_> = scope .bindings() .filter_map(|(name, binding_id)| { - let binding = &checker.semantic_model().bindings[binding_id]; + let binding = checker.semantic_model().binding(binding_id); if binding.kind.is_annotation() && !binding.is_used() && !checker.settings.dummy_variable_rgx.is_match(name) diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index 36b02cb5c0..09d4b98ad0 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -104,7 +104,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut let mut ignored: FxHashMap<(NodeId, Exceptions), Vec> = FxHashMap::default(); for binding_id in scope.binding_ids() { - let binding = &checker.semantic_model().bindings[binding_id]; + let binding = checker.semantic_model().binding(binding_id); if binding.is_used() || binding.is_explicit_export() { continue; diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index fb6932ca3f..6ffa78fae6 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -292,7 +292,7 @@ pub(crate) fn unused_variable(checker: &mut Checker, scope: ScopeId) { let bindings: Vec<_> = scope .bindings() - .map(|(name, binding_id)| (name, &checker.semantic_model().bindings[binding_id])) + .map(|(name, binding_id)| (name, checker.semantic_model().binding(binding_id))) .filter_map(|(name, binding)| { if (binding.kind.is_assignment() || binding.kind.is_named_expr_assignment()) && !binding.is_used() diff --git a/crates/ruff/src/rules/pylint/rules/global_statement.rs b/crates/ruff/src/rules/pylint/rules/global_statement.rs index c65bf96d57..1bdbe19e26 100644 --- a/crates/ruff/src/rules/pylint/rules/global_statement.rs +++ b/crates/ruff/src/rules/pylint/rules/global_statement.rs @@ -57,7 +57,7 @@ impl Violation for GlobalStatement { pub(crate) fn global_statement(checker: &mut Checker, name: &str) { let scope = checker.semantic_model().scope(); if let Some(binding_id) = scope.get(name) { - let binding = &checker.semantic_model().bindings[binding_id]; + let binding = checker.semantic_model().binding(binding_id); if binding.kind.is_global() { let source = checker.semantic_model().stmts[binding .source diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 42d828a7d2..d9fd471b41 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -198,15 +198,10 @@ impl nohash_hasher::IsEnabled for BindingId {} pub struct Bindings<'a>(IndexVec>); impl<'a> Bindings<'a> { - /// Pushes a new binding and returns its id + /// Pushes a new [`Binding`] and returns its [`BindingId`]. pub fn push(&mut self, binding: Binding<'a>) -> BindingId { self.0.push(binding) } - - /// Returns the id that will be assigned when pushing the next binding - pub fn next_id(&self) -> BindingId { - self.0.next_index() - } } impl<'a> Deref for Bindings<'a> { diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index afb0e389e9..2e2df908ab 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -106,6 +106,18 @@ impl<'a> SemanticModel<'a> { } } + /// Return the [`Binding`] for the given [`BindingId`]. + #[inline] + pub fn binding(&self, id: BindingId) -> &Binding { + &self.bindings[id] + } + + /// Resolve the [`Reference`] for the given [`ReferenceId`]. + #[inline] + pub fn reference(&self, id: ReferenceId) -> &Reference { + &self.references[id] + } + /// Return `true` if the `Expr` is a reference to `typing.${target}`. pub fn match_typing_expr(&self, expr: &Expr, target: &str) -> bool { self.resolve_call_path(expr).map_or(false, |call_path| { @@ -717,11 +729,6 @@ impl<'a> SemanticModel<'a> { self.bindings[binding_id].references.push(reference_id); } - /// Resolve a [`ReferenceId`]. - pub fn reference(&self, reference_id: ReferenceId) -> &Reference { - self.references.resolve(reference_id) - } - /// Return the [`ExecutionContext`] of the current scope. pub const fn execution_context(&self) -> ExecutionContext { if self.in_type_checking_block() diff --git a/crates/ruff_python_semantic/src/reference.rs b/crates/ruff_python_semantic/src/reference.rs index cac8bb8f39..0626a5c808 100644 --- a/crates/ruff_python_semantic/src/reference.rs +++ b/crates/ruff_python_semantic/src/reference.rs @@ -1,6 +1,7 @@ use ruff_text_size::TextRange; +use std::ops::Deref; -use ruff_index::{newtype_index, IndexVec}; +use ruff_index::{newtype_index, IndexSlice, IndexVec}; use crate::context::ExecutionContext; use crate::scope::ScopeId; @@ -38,7 +39,7 @@ pub struct ReferenceId; pub struct References(IndexVec); impl References { - /// Pushes a new read reference and returns its unique id. + /// Pushes a new [`Reference`] and returns its [`ReferenceId`]. pub fn push( &mut self, scope_id: ScopeId, @@ -51,9 +52,12 @@ impl References { context, }) } +} - /// Returns the [`Reference`] with the given id. - pub fn resolve(&self, id: ReferenceId) -> &Reference { - &self.0[id] +impl Deref for References { + type Target = IndexSlice; + + fn deref(&self) -> &Self::Target { + &self.0 } }