diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB145.py b/crates/ruff/resources/test/fixtures/refurb/FURB145.py new file mode 100644 index 00000000000000..885674c8d9d492 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/refurb/FURB145.py @@ -0,0 +1,21 @@ +l = [1, 2, 3, 4, 5] + +# Errors. +a = l[:] +b, c = 1, l[:] +d, e = l[:], 1 +m = l[::] +l[:] +print(l[:]) + +# False negatives. +aa = a[:] # Type inference. + +# OK. +t = (1, 2, 3, 4, 5) +f = t[:] # t.copy() is not supported. +g = l[1:3] +h = l[1:] +i = l[:3] +j = l[1:3:2] +k = l[::2] diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 97d593de0620a9..cc294481251656 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -16,7 +16,7 @@ use crate::rules::{ flake8_future_annotations, flake8_gettext, flake8_implicit_str_concat, flake8_logging_format, flake8_pie, flake8_print, flake8_pyi, flake8_pytest_style, flake8_self, flake8_simplify, flake8_tidy_imports, flake8_use_pathlib, flynt, numpy, pandas_vet, pep8_naming, pycodestyle, - pyflakes, pygrep_hooks, pylint, pyupgrade, ruff, + pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, }; use crate::settings::types::PythonVersion; @@ -113,10 +113,12 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryIterableAllocationForFirstElement) { ruff::rules::unnecessary_iterable_allocation_for_first_element(checker, subscript); } - if checker.enabled(Rule::InvalidIndexType) { ruff::rules::invalid_index_type(checker, subscript); } + if checker.settings.rules.enabled(Rule::SliceCopy) { + refurb::rules::slice_copy(checker, subscript); + } pandas_vet::rules::subscript(checker, value, expr); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 44770057b02d4f..75a315fe793cd9 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -916,6 +916,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "131") => (RuleGroup::Nursery, rules::refurb::rules::DeleteFullSlice), #[allow(deprecated)] (Refurb, "132") => (RuleGroup::Nursery, rules::refurb::rules::CheckAndRemoveFromSet), + #[allow(deprecated)] + (Refurb, "145") => (RuleGroup::Nursery, rules::refurb::rules::SliceCopy), _ => return None, }) diff --git a/crates/ruff/src/rules/refurb/helpers.rs b/crates/ruff/src/rules/refurb/helpers.rs new file mode 100644 index 00000000000000..e432d5b7b8a7f2 --- /dev/null +++ b/crates/ruff/src/rules/refurb/helpers.rs @@ -0,0 +1,36 @@ +use ruff_python_ast as ast; +use ruff_python_codegen::Generator; +use ruff_text_size::TextRange; + +/// Format a code snippet to call `name.method()`. +pub(super) fn generate_method_call(name: &str, method: &str, generator: Generator) -> String { + // Construct `name`. + let var = ast::ExprName { + id: name.to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + // Construct `name.method`. + let attr = ast::ExprAttribute { + value: Box::new(var.into()), + attr: ast::Identifier::new(method.to_string(), TextRange::default()), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + // Make it into a call `name.method()` + let call = ast::ExprCall { + func: Box::new(attr.into()), + arguments: ast::Arguments { + args: vec![], + keywords: vec![], + range: TextRange::default(), + }, + range: TextRange::default(), + }; + // And finally, turn it into a statement. + let stmt = ast::StmtExpr { + value: Box::new(call.into()), + range: TextRange::default(), + }; + generator.stmt(&stmt.into()) +} diff --git a/crates/ruff/src/rules/refurb/mod.rs b/crates/ruff/src/rules/refurb/mod.rs index 2e5e5a59d9f189..a9109465950206 100644 --- a/crates/ruff/src/rules/refurb/mod.rs +++ b/crates/ruff/src/rules/refurb/mod.rs @@ -1,5 +1,6 @@ //! Rules from [refurb](https://pypi.org/project/refurb/)/ +mod helpers; pub(crate) mod rules; #[cfg(test)] @@ -16,6 +17,7 @@ mod tests { #[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))] #[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))] #[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))] + #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( 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 80685be3f31895..b251129db77e06 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 @@ -18,6 +18,11 @@ use crate::registry::AsRule; /// If an element should be removed from a set if it is present, it is more /// succinct and idiomatic to use `discard`. /// +/// ## Known problems +/// This rule is prone to false negatives due to type inference limitations, +/// as it will only detect sets that are instantiated as literals or annotated +/// with a type annotation. +/// /// ## Example /// ```python /// nums = {123, 456} 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 b284a744ae5712..e84b081761e566 100644 --- a/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs +++ b/crates/ruff/src/rules/refurb/rules/delete_full_slice.rs @@ -1,13 +1,13 @@ use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_codegen::Generator; use ruff_python_semantic::analyze::typing::{is_dict, is_list}; use ruff_python_semantic::{Binding, SemanticModel}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::registry::AsRule; +use crate::rules::refurb::helpers::generate_method_call; /// ## What it does /// Checks for `del` statements that delete the entire slice of a list or @@ -17,6 +17,11 @@ use crate::registry::AsRule; /// It's is faster and more succinct to remove all items via the `clear()` /// method. /// +/// ## Known problems +/// This rule is prone to false negatives due to type inference limitations, +/// as it will only detect lists and dictionaries that are instantiated as +/// literals or annotated with a type annotation. +/// /// ## Example /// ```python /// names = {"key": "value"} @@ -65,7 +70,7 @@ pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete) // Fix is only supported for single-target deletions. if checker.patch(diagnostic.kind.rule()) && delete.targets.len() == 1 { - let replacement = make_suggestion(name, checker.generator()); + let replacement = generate_method_call(name, "clear", checker.generator()); diagnostic.set_fix(Fix::suggested(Edit::replacement( replacement, delete.start(), @@ -118,38 +123,3 @@ fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a // Name is needed for the fix suggestion. Some(name) } - -/// Make fix suggestion for the given name, ie `name.clear()`. -fn make_suggestion(name: &str, generator: Generator) -> String { - // Here we construct `var.clear()` - // - // And start with construction of `var` - let var = ast::ExprName { - id: name.to_string(), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - // Make `var.clear`. - let attr = ast::ExprAttribute { - value: Box::new(var.into()), - attr: ast::Identifier::new("clear".to_string(), TextRange::default()), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - // Make it into a call `var.clear()` - let call = ast::ExprCall { - func: Box::new(attr.into()), - arguments: ast::Arguments { - args: vec![], - keywords: vec![], - range: TextRange::default(), - }, - range: TextRange::default(), - }; - // And finally, turn it into a statement. - let stmt = ast::StmtExpr { - value: Box::new(call.into()), - range: TextRange::default(), - }; - generator.stmt(&stmt.into()) -} diff --git a/crates/ruff/src/rules/refurb/rules/mod.rs b/crates/ruff/src/rules/refurb/rules/mod.rs index 1ea05511ee02a0..414a4168f4b50d 100644 --- a/crates/ruff/src/rules/refurb/rules/mod.rs +++ b/crates/ruff/src/rules/refurb/rules/mod.rs @@ -1,7 +1,9 @@ pub(crate) use check_and_remove_from_set::*; pub(crate) use delete_full_slice::*; pub(crate) use repeated_append::*; +pub(crate) use slice_copy::*; mod check_and_remove_from_set; mod delete_full_slice; mod repeated_append; +mod slice_copy; diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index 8557138f042594..90737de73979b6 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -21,6 +21,11 @@ use crate::registry::AsRule; /// a single `extend`. Each `append` resizes the list individually, whereas an /// `extend` can resize the list once for all elements. /// +/// ## Known problems +/// This rule is prone to false negatives due to type inference limitations, +/// as it will only detect lists that are instantiated as literals or annotated +/// with a type annotation. +/// /// ## Example /// ```python /// nums = [1, 2, 3] diff --git a/crates/ruff/src/rules/refurb/rules/slice_copy.rs b/crates/ruff/src/rules/refurb/rules/slice_copy.rs new file mode 100644 index 00000000000000..996915822f4c8e --- /dev/null +++ b/crates/ruff/src/rules/refurb/rules/slice_copy.rs @@ -0,0 +1,109 @@ +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::analyze::typing::is_list; +use ruff_python_semantic::{Binding, SemanticModel}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::registry::AsRule; +use crate::rules::refurb::helpers::generate_method_call; + +/// ## What it does +/// Checks for unbounded slice expressions to copy a list. +/// +/// ## Why is this bad? +/// The `list#copy` method is more readable and consistent with copying other +/// types. +/// +/// ## Known problems +/// This rule is prone to false negatives due to type inference limitations, +/// as it will only detect lists that are instantiated as literals or annotated +/// with a type annotation. +/// +/// ## Example +/// ```python +/// a = [1, 2, 3] +/// b = a[:] +/// ``` +/// +/// Use instead: +/// ```python +/// a = [1, 2, 3] +/// b = a.copy() +/// ``` +/// +/// ## References +/// - [Python documentation: Mutable Sequence Types](https://docs.python.org/3/library/stdtypes.html#mutable-sequence-types) +#[violation] +pub struct SliceCopy; + +impl Violation for SliceCopy { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Prefer `copy` method over slicing") + } + + fn autofix_title(&self) -> Option { + Some("Replace with `copy()`".to_string()) + } +} + +/// FURB145 +pub(crate) fn slice_copy(checker: &mut Checker, subscript: &ast::ExprSubscript) { + if subscript.ctx.is_store() || subscript.ctx.is_del() { + return; + } + + let Some(name) = match_list_full_slice(subscript, checker.semantic()) else { + return; + }; + let mut diagnostic = Diagnostic::new(SliceCopy, subscript.range()); + if checker.patch(diagnostic.kind.rule()) { + let replacement = generate_method_call(name, "copy", checker.generator()); + diagnostic.set_fix(Fix::suggested(Edit::replacement( + replacement, + subscript.start(), + subscript.end(), + ))); + } + checker.diagnostics.push(diagnostic); +} + +/// Matches `obj[:]` where `obj` is a list. +fn match_list_full_slice<'a>( + subscript: &'a ast::ExprSubscript, + semantic: &SemanticModel, +) -> Option<&'a str> { + // Check that it is `obj[:]`. + if !matches!( + subscript.slice.as_ref(), + Expr::Slice(ast::ExprSlice { + lower: None, + upper: None, + step: None, + range: _, + }) + ) { + return None; + } + + let ast::ExprName { id, .. } = subscript.value.as_name_expr()?; + + // Check that `obj` is a list. + let scope = semantic.current_scope(); + let bindings: Vec<&Binding> = scope + .get_all(id) + .map(|binding_id| semantic.binding(binding_id)) + .collect(); + let [binding] = bindings.as_slice() else { + return None; + }; + if !is_list(binding, semantic) { + return None; + } + + Some(id) +} diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB145_FURB145.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB145_FURB145.py.snap new file mode 100644 index 00000000000000..6686c0f3afe0dc --- /dev/null +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB145_FURB145.py.snap @@ -0,0 +1,128 @@ +--- +source: crates/ruff/src/rules/refurb/mod.rs +--- +FURB145.py:4:5: FURB145 [*] Prefer `copy` method over slicing + | +3 | # Errors. +4 | a = l[:] + | ^^^^ FURB145 +5 | b, c = 1, l[:] +6 | d, e = l[:], 1 + | + = help: Replace with `copy()` + +ℹ Suggested fix +1 1 | l = [1, 2, 3, 4, 5] +2 2 | +3 3 | # Errors. +4 |-a = l[:] + 4 |+a = l.copy() +5 5 | b, c = 1, l[:] +6 6 | d, e = l[:], 1 +7 7 | m = l[::] + +FURB145.py:5:11: FURB145 [*] Prefer `copy` method over slicing + | +3 | # Errors. +4 | a = l[:] +5 | b, c = 1, l[:] + | ^^^^ FURB145 +6 | d, e = l[:], 1 +7 | m = l[::] + | + = help: Replace with `copy()` + +ℹ Suggested fix +2 2 | +3 3 | # Errors. +4 4 | a = l[:] +5 |-b, c = 1, l[:] + 5 |+b, c = 1, l.copy() +6 6 | d, e = l[:], 1 +7 7 | m = l[::] +8 8 | l[:] + +FURB145.py:6:8: FURB145 [*] Prefer `copy` method over slicing + | +4 | a = l[:] +5 | b, c = 1, l[:] +6 | d, e = l[:], 1 + | ^^^^ FURB145 +7 | m = l[::] +8 | l[:] + | + = help: Replace with `copy()` + +ℹ Suggested fix +3 3 | # Errors. +4 4 | a = l[:] +5 5 | b, c = 1, l[:] +6 |-d, e = l[:], 1 + 6 |+d, e = l.copy(), 1 +7 7 | m = l[::] +8 8 | l[:] +9 9 | print(l[:]) + +FURB145.py:7:5: FURB145 [*] Prefer `copy` method over slicing + | +5 | b, c = 1, l[:] +6 | d, e = l[:], 1 +7 | m = l[::] + | ^^^^^ FURB145 +8 | l[:] +9 | print(l[:]) + | + = help: Replace with `copy()` + +ℹ Suggested fix +4 4 | a = l[:] +5 5 | b, c = 1, l[:] +6 6 | d, e = l[:], 1 +7 |-m = l[::] + 7 |+m = l.copy() +8 8 | l[:] +9 9 | print(l[:]) +10 10 | + +FURB145.py:8:1: FURB145 [*] Prefer `copy` method over slicing + | +6 | d, e = l[:], 1 +7 | m = l[::] +8 | l[:] + | ^^^^ FURB145 +9 | print(l[:]) + | + = help: Replace with `copy()` + +ℹ Suggested fix +5 5 | b, c = 1, l[:] +6 6 | d, e = l[:], 1 +7 7 | m = l[::] +8 |-l[:] + 8 |+l.copy() +9 9 | print(l[:]) +10 10 | +11 11 | # False negatives. + +FURB145.py:9:7: FURB145 [*] Prefer `copy` method over slicing + | + 7 | m = l[::] + 8 | l[:] + 9 | print(l[:]) + | ^^^^ FURB145 +10 | +11 | # False negatives. + | + = help: Replace with `copy()` + +ℹ Suggested fix +6 6 | d, e = l[:], 1 +7 7 | m = l[::] +8 8 | l[:] +9 |-print(l[:]) + 9 |+print(l.copy()) +10 10 | +11 11 | # False negatives. +12 12 | aa = a[:] # Type inference. + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index b54bbf9186e001..1b8adf69ca2691 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -811,6 +811,7 @@ mod tests { Rule::RepeatedAppend, Rule::DeleteFullSlice, Rule::CheckAndRemoveFromSet, + Rule::SliceCopy, Rule::QuadraticListSummation, ]; diff --git a/ruff.schema.json b/ruff.schema.json index 43ef15984012ca..7daca772e0ef56 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2079,6 +2079,8 @@ "FURB13", "FURB131", "FURB132", + "FURB14", + "FURB145", "G", "G0", "G00",