From e3c36465ec51b0838b9337a3bae60dc0a9aef708 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 21:39:38 -0400 Subject: [PATCH] Base parameter-find lookup on range, rather than name (#6960) ## Summary This simplifies the signature a bit and is more reliable in the (unusual? invalid?) case of duplicate parameters. --- crates/ruff/src/rules/refurb/helpers.rs | 38 ++++++++----------- .../refurb/rules/check_and_remove_from_set.rs | 4 +- .../rules/refurb/rules/delete_full_slice.rs | 4 +- .../src/rules/refurb/rules/repeated_append.rs | 2 +- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/crates/ruff/src/rules/refurb/helpers.rs b/crates/ruff/src/rules/refurb/helpers.rs index d1d92edd52..c6523858f4 100644 --- a/crates/ruff/src/rules/refurb/helpers.rs +++ b/crates/ruff/src/rules/refurb/helpers.rs @@ -3,6 +3,7 @@ use ruff_python_ast::helpers::map_subscript; use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; use ruff_python_semantic::{Binding, BindingKind, SemanticModel}; +use ruff_text_size::Ranged; /// Abstraction for a type checker, conservatively checks for the intended type(s). trait TypeChecker { @@ -17,7 +18,7 @@ trait TypeChecker { /// NOTE: this function doesn't perform more serious type inference, so it won't be able /// to understand if the value gets initialized from a call to a function always returning /// lists. This also implies no interfile analysis. -fn check_type(binding: &Binding, name: &str, semantic: &SemanticModel) -> bool { +fn check_type(binding: &Binding, semantic: &SemanticModel) -> bool { match binding.kind { BindingKind::Assignment => match binding.statement(semantic) { // ```python @@ -48,8 +49,7 @@ fn check_type(binding: &Binding, name: &str, semantic: &Semantic // // We trust the annotation and see if the type checker matches the annotation. Some(Stmt::FunctionDef(ast::StmtFunctionDef { parameters, .. })) => { - // TODO(charlie): Store a pointer to the argument in the binding. - let Some(parameter) = find_parameter_by_name(parameters.as_ref(), name) else { + let Some(parameter) = find_parameter(parameters.as_ref(), binding) else { return false; }; let Some(ref annotation) = parameter.parameter.annotation else { @@ -160,40 +160,34 @@ impl BuiltinTypeChecker for SetChecker { /// Test whether the given binding (and the given name) can be considered a list. /// For this, we check what value might be associated with it through it's initialization and /// what annotation it has (we consider `list` and `typing.List`). -pub(super) fn is_list<'a>(binding: &'a Binding, name: &str, semantic: &'a SemanticModel) -> bool { - check_type::(binding, name, semantic) +pub(super) fn is_list(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) } /// Test whether the given binding (and the given name) can be considered a dictionary. /// For this, we check what value might be associated with it through it's initialization and /// what annotation it has (we consider `dict` and `typing.Dict`). -pub(super) fn is_dict<'a>(binding: &'a Binding, name: &str, semantic: &'a SemanticModel) -> bool { - check_type::(binding, name, semantic) +pub(super) fn is_dict(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) } /// Test whether the given binding (and the given name) can be considered a set. /// For this, we check what value might be associated with it through it's initialization and /// what annotation it has (we consider `set` and `typing.Set`). -pub(super) fn is_set<'a>(binding: &'a Binding, name: &str, semantic: &'a SemanticModel) -> bool { - check_type::(binding, name, semantic) +pub(super) fn is_set(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) } +/// Find the [`ParameterWithDefault`] corresponding to the given [`Binding`]. #[inline] -fn find_parameter_by_name<'a>( +fn find_parameter<'a>( parameters: &'a Parameters, - name: &'a str, -) -> Option<&'a ParameterWithDefault> { - find_parameter_by_name_impl(¶meters.args, name) - .or_else(|| find_parameter_by_name_impl(¶meters.posonlyargs, name)) - .or_else(|| find_parameter_by_name_impl(¶meters.kwonlyargs, name)) -} - -#[inline] -fn find_parameter_by_name_impl<'a>( - parameters: &'a [ParameterWithDefault], - name: &'a str, + binding: &Binding, ) -> Option<&'a ParameterWithDefault> { parameters + .args .iter() - .find(|arg| arg.parameter.name.as_str() == name) + .chain(parameters.posonlyargs.iter()) + .chain(parameters.kwonlyargs.iter()) + .find(|arg| arg.parameter.name.range() == binding.range()) } diff --git a/crates/ruff/src/rules/refurb/rules/check_and_remove_from_set.rs b/crates/ruff/src/rules/refurb/rules/check_and_remove_from_set.rs index e9597c428e..e943d7c895 100644 --- a/crates/ruff/src/rules/refurb/rules/check_and_remove_from_set.rs +++ b/crates/ruff/src/rules/refurb/rules/check_and_remove_from_set.rs @@ -95,9 +95,7 @@ pub(crate) fn check_and_remove_from_set(checker: &mut Checker, if_stmt: &ast::St .semantic() .resolve_name(check_set) .map(|binding_id| checker.semantic().binding(binding_id)) - .map_or(false, |binding| { - is_set(binding, &check_set.id, checker.semantic()) - }) + .map_or(false, |binding| is_set(binding, checker.semantic())) { return; }; diff --git a/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs b/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs index 4b508ff449..7b2c1e7ec7 100644 --- a/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs +++ b/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs @@ -111,9 +111,7 @@ fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a }; // It should only apply to variables that are known to be lists or dicts. - if binding.source.is_none() - || !(is_dict(binding, name, semantic) || is_list(binding, name, semantic)) - { + if !(is_dict(binding, semantic) || is_list(binding, semantic)) { return None; } diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index d5e59eea71..0deb6e2ed9 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -299,7 +299,7 @@ fn match_append<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option