From 1a9536c4e213ca0627fb34c8c2bfeb058572b5e2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 14 Aug 2023 00:09:05 -0400 Subject: [PATCH] Remove `SemanticModel#find_binding` (#6546) ## Summary This method is almost never what you actually want, because it doesn't respect Python's scoping semantics. For example, if you call this within a class method, it will return class attributes, whereas Python actually _skips_ symbols in classes unless the load occurs within the class itself. I also want to move away from these kinds of dynamic lookups and more towards `resolve_name`, which performs a lookup based on the stored `BindingId` at the time of symbol resolution, and will make it much easier for us to separate model building from linting in the near future. ## Test Plan `cargo test` --- .../rules/private_member_access.rs | 34 ++++++++--------- crates/ruff/src/rules/pandas_vet/helpers.rs | 38 ++++++++++--------- .../pandas_vet/rules/inplace_argument.rs | 22 +++-------- crates/ruff_python_semantic/src/model.rs | 26 +++++++------ 4 files changed, 55 insertions(+), 65 deletions(-) diff --git a/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs b/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs index 4a3c0cafc3..8749d54b52 100644 --- a/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs +++ b/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::call_path::collect_call_path; use ruff_python_ast::{self as ast, Expr, Ranged}; -use ruff_python_semantic::ScopeKind; +use ruff_python_semantic::{BindingKind, ScopeKind}; use crate::checkers::ast::Checker; @@ -156,31 +156,27 @@ pub(crate) fn private_member_access(checker: &mut Checker, expr: &Expr) { if matches!(call_path.as_slice(), ["self" | "cls" | "mcs"]) { return; } + } + if let Expr::Name(name) = value.as_ref() { // Ignore accesses on class members from _within_ the class. if checker .semantic() - .scopes - .iter() - .rev() - .find_map(|scope| match &scope.kind { - ScopeKind::Class(ast::StmtClassDef { name, .. }) => Some(name), - _ => None, - }) - .is_some_and(|name| { - if call_path.as_slice() == [name.as_str()] { - checker - .semantic() - .find_binding(name) - .is_some_and(|binding| { - // TODO(charlie): Could the name ever be bound to a - // _different_ class here? - binding.kind.is_class_definition() - }) + .resolve_name(name) + .and_then(|id| { + if let BindingKind::ClassDefinition(scope) = checker.semantic().binding(id).kind + { + Some(scope) } else { - false + None } }) + .is_some_and(|scope| { + checker + .semantic() + .current_scope_ids() + .any(|parent| scope == parent) + }) { return; } diff --git a/crates/ruff/src/rules/pandas_vet/helpers.rs b/crates/ruff/src/rules/pandas_vet/helpers.rs index 1136bfa1c7..295f160754 100644 --- a/crates/ruff/src/rules/pandas_vet/helpers.rs +++ b/crates/ruff/src/rules/pandas_vet/helpers.rs @@ -1,4 +1,3 @@ -use ruff_python_ast as ast; use ruff_python_ast::Expr; use ruff_python_semantic::{BindingKind, Imported, SemanticModel}; @@ -26,23 +25,26 @@ pub(super) fn test_expression(expr: &Expr, semantic: &SemanticModel) -> Resoluti | Expr::ListComp(_) | Expr::DictComp(_) | Expr::GeneratorExp(_) => Resolution::IrrelevantExpression, - Expr::Name(ast::ExprName { id, .. }) => semantic.find_binding(id).map_or( - Resolution::IrrelevantBinding, - |binding| match &binding.kind { - BindingKind::Annotation - | BindingKind::Argument - | BindingKind::Assignment - | BindingKind::NamedExprAssignment - | BindingKind::UnpackedAssignment - | BindingKind::LoopVar - | BindingKind::Global - | BindingKind::Nonlocal(_) => Resolution::RelevantLocal, - BindingKind::Import(import) if matches!(import.call_path(), ["pandas"]) => { - Resolution::PandasModule - } - _ => Resolution::IrrelevantBinding, - }, - ), + Expr::Name(name) => { + semantic + .resolve_name(name) + .map_or(Resolution::IrrelevantBinding, |id| { + match &semantic.binding(id).kind { + BindingKind::Annotation + | BindingKind::Argument + | BindingKind::Assignment + | BindingKind::NamedExprAssignment + | BindingKind::UnpackedAssignment + | BindingKind::LoopVar + | BindingKind::Global + | BindingKind::Nonlocal(_) => Resolution::RelevantLocal, + BindingKind::Import(import) if matches!(import.call_path(), ["pandas"]) => { + Resolution::PandasModule + } + _ => Resolution::IrrelevantBinding, + } + }) + } _ => Resolution::RelevantLocal, } } diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs index 7ac4a06a89..7cc11c2025 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs @@ -2,8 +2,6 @@ use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::is_const_true; use ruff_python_ast::{self as ast, Keyword, PySourceType, Ranged}; -use ruff_python_semantic::BindingKind; -use ruff_python_semantic::Imported; use ruff_source_file::Locator; use crate::autofix::edits::{remove_argument, Parentheses}; @@ -53,20 +51,12 @@ impl Violation for PandasUseOfInplaceArgument { /// PD002 pub(crate) fn inplace_argument(checker: &mut Checker, call: &ast::ExprCall) { // If the function was imported from another module, and it's _not_ Pandas, abort. - if let Some(call_path) = checker.semantic().resolve_call_path(&call.func) { - if !call_path - .first() - .and_then(|module| checker.semantic().find_binding(module)) - .is_some_and(|binding| { - if let BindingKind::Import(import) = &binding.kind { - matches!(import.call_path(), ["pandas"]) - } else { - false - } - }) - { - return; - } + if checker + .semantic() + .resolve_call_path(&call.func) + .is_some_and(|call_path| !matches!(call_path.as_slice(), ["pandas", ..])) + { + return; } let mut seen_star = false; diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 0a97a0a638..1221883920 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -233,13 +233,6 @@ impl<'a> SemanticModel<'a> { }) } - /// Return the current [`Binding`] for a given `name`. - pub fn find_binding(&self, member: &str) -> Option<&Binding> { - self.current_scopes() - .find_map(|scope| scope.get(member)) - .map(|binding_id| &self.bindings[binding_id]) - } - /// Return the [`BindingId`] that the given [`BindingId`] shadows, if any. /// /// Note that this will only return bindings that are shadowed by a binding in a parent scope. @@ -618,6 +611,11 @@ impl<'a> SemanticModel<'a> { Some(binding_id) } + /// Resolves the [`ast::ExprName`] to the [`BindingId`] of the symbol it refers to, if any. + pub fn resolve_name(&self, name: &ast::ExprName) -> Option { + self.resolved_names.get(&name.into()).copied() + } + /// Resolves the [`Expr`] to a fully-qualified symbol-name, if `value` resolves to an imported /// or builtin symbol. /// @@ -642,11 +640,10 @@ impl<'a> SemanticModel<'a> { // If the name was already resolved, look it up; otherwise, search for the symbol. let head = match_head(value)?; - let binding = if let Some(id) = self.resolved_names.get(&head.into()) { - self.binding(*id) - } else { - self.find_binding(&head.id)? - }; + let binding = self + .resolve_name(head) + .or_else(|| self.lookup_symbol(&head.id)) + .map(|id| self.binding(id))?; match &binding.kind { BindingKind::Import(Import { call_path }) => { @@ -917,6 +914,11 @@ impl<'a> SemanticModel<'a> { self.scopes.ancestors(self.scope_id) } + /// Returns an iterator over all scopes IDs, starting from the current [`Scope`]. + pub fn current_scope_ids(&self) -> impl Iterator + '_ { + self.scopes.ancestor_ids(self.scope_id) + } + /// Returns the parent of the given [`Scope`], if any. pub fn parent_scope(&self, scope: &Scope) -> Option<&Scope<'a>> { scope.parent.map(|scope_id| &self.scopes[scope_id])