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`
This commit is contained in:
Charlie Marsh 2023-08-14 00:09:05 -04:00 committed by GitHub
parent bf4c6473c8
commit 1a9536c4e2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 55 additions and 65 deletions

View file

@ -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;
}

View file

@ -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,
}
}

View file

@ -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;

View file

@ -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<BindingId> {
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<Item = ScopeId> + '_ {
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])