Skip to content

Commit

Permalink
[refurb] Implement no-slice-copy (FURB145) (#7007)
Browse files Browse the repository at this point in the history
## Summary

Implement
[`no-slice-copy`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_slice_copy.py)
as `slice-copy` (`FURB145`).

Related to #1348.

## Test Plan

`cargo test`
  • Loading branch information
tjkuson authored Sep 13, 2023
1 parent b0cbcd3 commit ebe9c03
Show file tree
Hide file tree
Showing 13 changed files with 325 additions and 40 deletions.
21 changes: 21 additions & 0 deletions crates/ruff/resources/test/fixtures/refurb/FURB145.py
Original file line number Diff line number Diff line change
@@ -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]
6 changes: 4 additions & 2 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand Down
36 changes: 36 additions & 0 deletions crates/ruff/src/rules/refurb/helpers.rs
Original file line number Diff line number Diff line change
@@ -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())
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Rules from [refurb](https://pypi.org/project/refurb/)/

mod helpers;
pub(crate) mod rules;

#[cfg(test)]
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
46 changes: 8 additions & 38 deletions crates/ruff/src/rules/refurb/rules/delete_full_slice.rs
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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"}
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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())
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 5 additions & 0 deletions crates/ruff/src/rules/refurb/rules/repeated_append.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
109 changes: 109 additions & 0 deletions crates/ruff/src/rules/refurb/rules/slice_copy.rs
Original file line number Diff line number Diff line change
@@ -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)
}
Loading

0 comments on commit ebe9c03

Please sign in to comment.