From 01fe2686124fb4b0a94184e17e7b246fa7e5b689 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Thu, 21 Mar 2024 18:09:09 +0100 Subject: [PATCH] [`refurb`] Implement `list_assign_reversed` lint (FURB187) (#10212) ## Summary Implement [use_reverse (FURB187)](https://github.com/dosisod/refurb/blob/master/refurb/checks/readability/use_reverse.py) lint. Tests were copied from original https://github.com/dosisod/refurb/blob/master/test/data/err_187.py. ## Test Plan cargo test --- .../resources/test/fixtures/refurb/FURB187.py | 62 ++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../refurb/rules/list_assign_reversed.rs | 190 ++++++++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + ...es__refurb__tests__FURB187_FURB187.py.snap | 59 ++++++ ruff.schema.json | 1 + 8 files changed, 319 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB187_FURB187.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py new file mode 100644 index 0000000000000..48b29e896f350 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py @@ -0,0 +1,62 @@ +# Errors + + +def a(): + l = [] + l = reversed(l) + + +def b(): + l = [] + l = list(reversed(l)) + + +def c(): + l = [] + l = l[::-1] + + +# False negative +def c2(): + class Wrapper: + l: list[int] + + w = Wrapper() + w.l = list(reversed(w.l)) + w.l = w.l[::-1] + w.l = reversed(w.l) + + +# OK + + +def d(): + l = [] + _ = reversed(l) + + +def e(): + l = [] + l = l[::-2] + l = l[1:] + l = l[1::-1] + l = l[:1:-1] + + +def f(): + d = {} + + # Don't warn: `d` is a dictionary, which doesn't have a `reverse` method. + d = reversed(d) + + +def g(): + l = "abc"[::-1] + + +def h(): + l = reversed([1, 2, 3]) + + +def i(): + l = list(reversed([1, 2, 3])) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c8f423cf16ae9..b3502815fd304 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1503,6 +1503,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } } + if checker.enabled(Rule::ListAssignReversed) { + refurb::rules::list_assign_reversed(checker, assign); + } } Stmt::AnnAssign( assign_stmt @ ast::StmtAnnAssign { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 774ad36d1b089..026618259e135 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1056,6 +1056,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd), (Refurb, "180") => (RuleGroup::Preview, rules::refurb::rules::MetaClassABCMeta), (Refurb, "181") => (RuleGroup::Preview, rules::refurb::rules::HashlibDigestHex), + (Refurb, "187") => (RuleGroup::Preview, rules::refurb::rules::ListAssignReversed), // flake8-logging (Flake8Logging, "001") => (RuleGroup::Stable, rules::flake8_logging::rules::DirectLoggerInstantiation), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index de5f0bdde06dd..5a8c6254fbf95 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -35,6 +35,7 @@ mod tests { #[test_case(Rule::RedundantLogBase, Path::new("FURB163.py"))] #[test_case(Rule::MetaClassABCMeta, Path::new("FURB180.py"))] #[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))] + #[test_case(Rule::ListAssignReversed, Path::new("FURB187.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_linter/src/rules/refurb/rules/list_assign_reversed.rs b/crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs new file mode 100644 index 0000000000000..dc9e853acf561 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs @@ -0,0 +1,190 @@ +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{ + Expr, ExprCall, ExprName, ExprSlice, ExprSubscript, ExprUnaryOp, Int, StmtAssign, UnaryOp, +}; +use ruff_python_semantic::analyze::typing; +use ruff_python_semantic::SemanticModel; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for list reversals that can be performed in-place in lieu of +/// creating a new list. +/// +/// ## Why is this bad? +/// When reversing a list, it's more efficient to use the in-place method +/// `.reverse()` instead of creating a new list, if the original list is +/// no longer needed. +/// +/// ## Example +/// ```python +/// l = [1, 2, 3] +/// l = reversed(l) +/// +/// l = [1, 2, 3] +/// l = list(reversed(l)) +/// +/// l = [1, 2, 3] +/// l = l[::-1] +/// ``` +/// +/// Use instead: +/// ```python +/// l = [1, 2, 3] +/// l.reverse() +/// ``` +/// +/// ## References +/// - [Python documentation: More on Lists](https://docs.python.org/3/tutorial/datastructures.html#more-on-lists) +#[violation] +pub struct ListAssignReversed { + name: String, +} + +impl AlwaysFixableViolation for ListAssignReversed { + #[derive_message_formats] + fn message(&self) -> String { + let ListAssignReversed { name } = self; + format!("Use of assignment of `reversed` on list `{name}`") + } + + fn fix_title(&self) -> String { + let ListAssignReversed { name } = self; + format!("Replace with `{name}.reverse()`") + } +} + +/// FURB187 +pub(crate) fn list_assign_reversed(checker: &mut Checker, assign: &StmtAssign) { + let [Expr::Name(target_expr)] = assign.targets.as_slice() else { + return; + }; + + let Some(reversed_expr) = extract_reversed(assign.value.as_ref(), checker.semantic()) else { + return; + }; + + if reversed_expr.id != target_expr.id { + return; + } + + let Some(binding) = checker + .semantic() + .only_binding(reversed_expr) + .map(|id| checker.semantic().binding(id)) + else { + return; + }; + if !typing::is_list(binding, checker.semantic()) { + return; + } + + checker.diagnostics.push( + Diagnostic::new( + ListAssignReversed { + name: target_expr.id.to_string(), + }, + assign.range(), + ) + .with_fix(Fix::safe_edit(Edit::range_replacement( + format!("{}.reverse()", target_expr.id), + assign.range(), + ))), + ); +} + +/// Recursively removes any `list` wrappers from the expression. +/// +/// For example, given `list(list(list([1, 2, 3])))`, this function +/// would return the inner `[1, 2, 3]` expression. +fn peel_lists(expr: &Expr) -> &Expr { + let Some(ExprCall { + func, arguments, .. + }) = expr.as_call_expr() + else { + return expr; + }; + + if !arguments.keywords.is_empty() { + return expr; + } + + if !func.as_name_expr().is_some_and(|name| name.id == "list") { + return expr; + } + + let [arg] = arguments.args.as_ref() else { + return expr; + }; + + peel_lists(arg) +} + +/// Given a call to `reversed`, returns the inner argument. +/// +/// For example, given `reversed(l)`, this function would return `l`. +fn extract_name_from_reversed<'a>( + expr: &'a Expr, + semantic: &SemanticModel, +) -> Option<&'a ExprName> { + let ExprCall { + func, arguments, .. + } = expr.as_call_expr()?; + + if !arguments.keywords.is_empty() { + return None; + } + + let [arg] = arguments.args.as_ref() else { + return None; + }; + + let arg = func + .as_name_expr() + .is_some_and(|name| name.id == "reversed") + .then(|| arg.as_name_expr()) + .flatten()?; + + if !semantic.is_builtin("reversed") { + return None; + } + + Some(arg) +} + +/// Given a slice expression, returns the inner argument if it's a reversed slice. +/// +/// For example, given `l[::-1]`, this function would return `l`. +fn extract_name_from_sliced_reversed(expr: &Expr) -> Option<&ExprName> { + let ExprSubscript { value, slice, .. } = expr.as_subscript_expr()?; + let ExprSlice { + lower, upper, step, .. + } = slice.as_slice_expr()?; + if lower.is_some() || upper.is_some() { + return None; + } + let Some(ExprUnaryOp { + op: UnaryOp::USub, + operand, + .. + }) = step.as_ref().and_then(|expr| expr.as_unary_op_expr()) + else { + return None; + }; + if !operand + .as_number_literal_expr() + .and_then(|num| num.value.as_int()) + .and_then(Int::as_u8) + .is_some_and(|value| value == 1) + { + return None; + }; + value.as_name_expr() +} + +fn extract_reversed<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a ExprName> { + let expr = peel_lists(expr); + extract_name_from_reversed(expr, semantic).or_else(|| extract_name_from_sliced_reversed(expr)) +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index a69a798cf5ec8..1cef073366127 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -5,6 +5,7 @@ pub(crate) use hashlib_digest_hex::*; pub(crate) use if_expr_min_max::*; pub(crate) use implicit_cwd::*; pub(crate) use isinstance_type_none::*; +pub(crate) use list_assign_reversed::*; pub(crate) use math_constant::*; pub(crate) use metaclass_abcmeta::*; pub(crate) use print_empty_string::*; @@ -27,6 +28,7 @@ mod hashlib_digest_hex; mod if_expr_min_max; mod implicit_cwd; mod isinstance_type_none; +mod list_assign_reversed; mod math_constant; mod metaclass_abcmeta; mod print_empty_string; diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB187_FURB187.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB187_FURB187.py.snap new file mode 100644 index 0000000000000..43d0f5a1657d0 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB187_FURB187.py.snap @@ -0,0 +1,59 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB187.py:6:5: FURB187 [*] Use of assignment of `reversed` on list `l` + | +4 | def a(): +5 | l = [] +6 | l = reversed(l) + | ^^^^^^^^^^^^^^^ FURB187 + | + = help: Replace with `l.reverse()` + +ℹ Safe fix +3 3 | +4 4 | def a(): +5 5 | l = [] +6 |- l = reversed(l) + 6 |+ l.reverse() +7 7 | +8 8 | +9 9 | def b(): + +FURB187.py:11:5: FURB187 [*] Use of assignment of `reversed` on list `l` + | + 9 | def b(): +10 | l = [] +11 | l = list(reversed(l)) + | ^^^^^^^^^^^^^^^^^^^^^ FURB187 + | + = help: Replace with `l.reverse()` + +ℹ Safe fix +8 8 | +9 9 | def b(): +10 10 | l = [] +11 |- l = list(reversed(l)) + 11 |+ l.reverse() +12 12 | +13 13 | +14 14 | def c(): + +FURB187.py:16:5: FURB187 [*] Use of assignment of `reversed` on list `l` + | +14 | def c(): +15 | l = [] +16 | l = l[::-1] + | ^^^^^^^^^^^ FURB187 + | + = help: Replace with `l.reverse()` + +ℹ Safe fix +13 13 | +14 14 | def c(): +15 15 | l = [] +16 |- l = l[::-1] + 16 |+ l.reverse() +17 17 | +18 18 | +19 19 | # False negative diff --git a/ruff.schema.json b/ruff.schema.json index 2bd6fa00a162c..b3e88111c7f73 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3030,6 +3030,7 @@ "FURB18", "FURB180", "FURB181", + "FURB187", "G", "G0", "G00",