diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB131.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB131.py index ee981b934daae..02280e876c67a 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB131.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB131.py @@ -43,6 +43,12 @@ def yes_four(x: Dict[int, str]): del x[:] +def yes_five(x: Dict[int, str]): + # FURB131 + del x[:] + + x = 1 + # these should not del names["key"] diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs index 559ba86e1e6e0..352bcafe5803e 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs @@ -4,7 +4,6 @@ use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_semantic::analyze::typing::is_dict; -use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; @@ -129,22 +128,16 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo } // Exclude non-dictionary value. - let Expr::Name(ast::ExprName { - id: subscript_name, .. - }) = subscript_value.as_ref() - else { + let Some(name) = subscript_value.as_name_expr() else { return; }; - let scope = checker.semantic().current_scope(); - let bindings: Vec<&Binding> = scope - .get_all(subscript_name) - .map(|binding_id| checker.semantic().binding(binding_id)) - .collect(); - - let [binding] = bindings.as_slice() else { + let Some(binding) = checker + .semantic() + .only_binding(name) + .map(|id| checker.semantic().binding(id)) + else { return; }; - if !is_dict(binding, checker.semantic()) { return; } @@ -165,8 +158,7 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo // ``` if if_test.is_some_and(|test| { any_over_expr(test, &|expr| { - expr.as_name_expr() - .is_some_and(|expr| expr.id == *subscript_name) + ComparableExpr::from(expr) == ComparableExpr::from(name) }) }) { return; diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index bf04777f7934e..b939f3671d3c3 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -5,7 +5,6 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::helpers::any_over_expr; use ruff_python_semantic::analyze::typing::is_list; -use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; @@ -144,20 +143,16 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo } // Avoid non-list values. - let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + let Some(name) = value.as_name_expr() else { return; }; - let bindings: Vec<&Binding> = checker + let Some(binding) = checker .semantic() - .current_scope() - .get_all(id) - .map(|binding_id| checker.semantic().binding(binding_id)) - .collect(); - - let [binding] = bindings.as_slice() else { + .only_binding(name) + .map(|id| checker.semantic().binding(id)) + else { return; }; - if !is_list(binding, checker.semantic()) { return; } @@ -176,15 +171,12 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo // ```python // filtered = [x for x in y if x in filtered] // ``` - if let Some(value_name) = value.as_name_expr() { - if if_test.is_some_and(|test| { - any_over_expr(test, &|expr| { - expr.as_name_expr() - .is_some_and(|expr| expr.id == value_name.id) - }) - }) { - return; - } + if if_test.is_some_and(|test| { + any_over_expr(test, &|expr| { + expr.as_name_expr().is_some_and(|expr| expr.id == name.id) + }) + }) { + return; } checker diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_copy.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_copy.rs index 39ad7dc0592f6..a1f0049588188 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_copy.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_copy.rs @@ -3,7 +3,6 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use ruff_python_semantic::analyze::typing::is_list; -use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; @@ -102,20 +101,16 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm } // Avoid non-list values. - let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + let Some(name) = value.as_name_expr() else { return; }; - let bindings: Vec<&Binding> = checker + let Some(binding) = checker .semantic() - .current_scope() - .get_all(id) - .map(|binding_id| checker.semantic().binding(binding_id)) - .collect(); - - let [binding] = bindings.as_slice() else { + .only_binding(name) + .map(|id| checker.semantic().binding(id)) + else { return; }; - if !is_list(binding, checker.semantic()) { return; } diff --git a/crates/ruff_linter/src/rules/refurb/rules/check_and_remove_from_set.rs b/crates/ruff_linter/src/rules/refurb/rules/check_and_remove_from_set.rs index 083c9a05e0963..93d1ac8357082 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/check_and_remove_from_set.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/check_and_remove_from_set.rs @@ -97,9 +97,9 @@ pub(crate) fn check_and_remove_from_set(checker: &mut Checker, if_stmt: &ast::St // Check if what we assume is set is indeed a set. if !checker .semantic() - .resolve_name(check_set) - .map(|binding_id| checker.semantic().binding(binding_id)) - .map_or(false, |binding| is_set(binding, checker.semantic())) + .only_binding(check_set) + .map(|id| checker.semantic().binding(id)) + .is_some_and(|binding| is_set(binding, checker.semantic())) { return; }; diff --git a/crates/ruff_linter/src/rules/refurb/rules/delete_full_slice.rs b/crates/ruff_linter/src/rules/refurb/rules/delete_full_slice.rs index c7ce2b218c003..a6584201fcfca 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/delete_full_slice.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/delete_full_slice.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr}; use ruff_python_semantic::analyze::typing::{is_dict, is_list}; -use ruff_python_semantic::{Binding, SemanticModel}; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -70,7 +70,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete) // Fix is only supported for single-target deletions. if delete.targets.len() == 1 { - let replacement = generate_method_call(name, "clear", checker.generator()); + let replacement = generate_method_call(&name.id, "clear", checker.generator()); diagnostic.set_fix(Fix::unsafe_edit(Edit::replacement( replacement, delete.start(), @@ -83,7 +83,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete) } /// Match `del expr[:]` where `expr` is a list or a dict. -fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a str> { +fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a ast::ExprName> { // Check that it is `del expr[...]`. let subscript = expr.as_subscript_expr()?; @@ -100,22 +100,9 @@ fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a return None; } - // Check that it is del var[:] - let ast::ExprName { id: name, .. } = subscript.value.as_name_expr()?; - - // Let's find definition for var - let scope = semantic.current_scope(); - let bindings: Vec<&Binding> = scope - .get_all(name) - .map(|binding_id| semantic.binding(binding_id)) - .collect(); - - // NOTE: Maybe it is too strict of a limitation, but it seems reasonable. - let [binding] = bindings.as_slice() else { - return None; - }; - // It should only apply to variables that are known to be lists or dicts. + let name = subscript.value.as_name_expr()?; + let binding = semantic.binding(semantic.only_binding(name)?); if !(is_dict(binding, semantic) || is_list(binding, semantic)) { return None; } diff --git a/crates/ruff_linter/src/rules/refurb/rules/repeated_append.rs b/crates/ruff_linter/src/rules/refurb/rules/repeated_append.rs index 7ca2ab2accae4..b381349a658c1 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/repeated_append.rs @@ -76,7 +76,7 @@ impl Violation for RepeatedAppend { /// FURB113 pub(crate) fn repeated_append(checker: &mut Checker, stmt: &Stmt) { - let Some(appends) = match_consecutive_appends(checker.semantic(), stmt) else { + let Some(appends) = match_consecutive_appends(stmt, checker.semantic()) else { return; }; @@ -163,8 +163,8 @@ impl Ranged for AppendGroup<'_> { /// Match consecutive calls to `append` on list variables starting from the given statement. fn match_consecutive_appends<'a>( - semantic: &'a SemanticModel, stmt: &'a Stmt, + semantic: &'a SemanticModel, ) -> Option>> { // Match the current statement, to see if it's an append. let append = match_append(semantic, stmt)?; diff --git a/crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs b/crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs index b737d131c81c8..5d693dfc43894 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs @@ -6,7 +6,7 @@ use ruff_python_ast as ast; use ruff_python_ast::{Arguments, Expr, Int}; use ruff_python_codegen::Generator; use ruff_python_semantic::analyze::typing::{is_dict, is_list, is_set, is_tuple}; -use ruff_python_semantic::Binding; + use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -114,7 +114,7 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF }; // Get the first argument, which is the sequence to iterate over. - let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { + let Some(Expr::Name(sequence)) = arguments.args.first() else { return; }; @@ -138,7 +138,8 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF ); // The index is unused, so replace with `for value in sequence`. - let replace_iter = Edit::range_replacement(sequence.into(), stmt_for.iter.range()); + let replace_iter = + Edit::range_replacement(sequence.id.to_string(), stmt_for.iter.range()); let replace_target = Edit::range_replacement( pad( checker.locator().slice(value).to_string(), @@ -154,12 +155,11 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF (false, true) => { // Ensure the sequence object works with `len`. If it doesn't, the // fix is unclear. - let scope = checker.semantic().current_scope(); - let bindings: Vec<&Binding> = scope - .get_all(sequence) - .map(|binding_id| checker.semantic().binding(binding_id)) - .collect(); - let [binding] = bindings.as_slice() else { + let Some(binding) = checker + .semantic() + .only_binding(sequence) + .map(|id| checker.semantic().binding(id)) + else { return; }; // This will lead to a lot of false negatives, but it is the best @@ -193,7 +193,7 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF ) }) { let replace_iter = Edit::range_replacement( - generate_range_len_call(sequence, checker.generator()), + generate_range_len_call(&sequence.id, checker.generator()), stmt_for.iter.range(), ); diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB131_FURB131.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB131_FURB131.py.snap index 14ad4d9c3cf9a..1fc97fbc8ace2 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB131_FURB131.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB131_FURB131.py.snap @@ -127,6 +127,27 @@ FURB131.py:43:5: FURB131 [*] Prefer `clear` over deleting a full slice 43 |+ x.clear() 44 44 | 45 45 | -46 46 | # these should not +46 46 | def yes_five(x: Dict[int, str]): + +FURB131.py:48:5: FURB131 [*] Prefer `clear` over deleting a full slice + | +46 | def yes_five(x: Dict[int, str]): +47 | # FURB131 +48 | del x[:] + | ^^^^^^^^ FURB131 +49 | +50 | x = 1 + | + = help: Replace with `clear()` + +ℹ Suggested fix +45 45 | +46 46 | def yes_five(x: Dict[int, str]): +47 47 | # FURB131 +48 |- del x[:] + 48 |+ x.clear() +49 49 | +50 50 | x = 1 +51 51 | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index c9187aa39fba9..bd94df3ec47d1 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -616,6 +616,16 @@ impl<'a> SemanticModel<'a> { self.resolved_names.get(&name.into()).copied() } + /// Resolves the [`ast::ExprName`] to the [`BindingId`] of the symbol it refers to, if it's the + /// only binding to that name in its scope. + pub fn only_binding(&self, name: &ast::ExprName) -> Option { + self.resolve_name(name).filter(|id| { + let binding = self.binding(*id); + let scope = &self.scopes[binding.scope]; + scope.shadowed_binding(*id).is_none() + }) + } + /// Resolves the [`Expr`] to a fully-qualified symbol-name, if `value` resolves to an imported /// or builtin symbol. ///