[refurb] Implement no-slice-copy (FURB145)

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]

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

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

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())
+}

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(

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}

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())
-}

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;

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]

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<String> {
+        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)
+}