From 50d147c598159e0b5e3f949a723dce71290059e4 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Sun, 3 Mar 2024 18:58:31 +0100 Subject: [PATCH 1/2] [`refurb`] Implement `list_assign_reversed` lint (FURB187) --- .../resources/test/fixtures/refurb/FURB187.py | 61 +++++++ .../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 | 157 ++++++++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + ...es__refurb__tests__FURB187_FURB187.py.snap | 59 +++++++ ruff.schema.json | 1 + 8 files changed, 285 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..1e272aa86eeea --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py @@ -0,0 +1,61 @@ +# these should match + +# using functions to ensure `l` doesn't change type +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) + + +# these should not + +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 = {} + + # dont warn since d is a dict and does not 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 f8867e68cf5c3..773f59cee2dcf 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1518,6 +1518,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 4f545d1afae88..a84661f744cbc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1048,6 +1048,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..6693da47e9c2f --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs @@ -0,0 +1,157 @@ +use crate::checkers::ast::Checker; +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; + +/// ## What it does +/// Checks for uses of assignment of "reversed" expression on the list to the same binging. +/// +/// ## Why is this bad? +/// +/// Use of in-place method `.reverse()` is faster and allows to avoid copying the name of variable. +/// +/// ## Example +/// ```python +/// l = [1, 2, 3] +/// l = reversed(l) +/// l = list(reversed(l)) +/// l = l[::-1] +/// ``` +/// +/// Use instead: +/// ```python +/// l = [1, 2, 3] +/// l.reverse() +/// l.reverse() +/// 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 { + format!("Use of assignment of `reversed` on list `{}`", self.name) + } + + fn fix_title(&self) -> String { + format!("Use `{}.reverse()` instead", self.name) + } +} + +fn extract_name_from_reversed(expr: &Expr) -> Option<&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; + }; + + func.as_name_expr() + .is_some_and(|name_expr| name_expr.id == "reversed") + .then(|| arg.as_name_expr()) + .flatten() +} + +fn peel_lists(expr: &Expr) -> &Expr { + let Some(ExprCall { + func, arguments, .. + }) = expr.as_call_expr() + else { + return expr; + }; + if !arguments.keywords.is_empty() + || func + .as_name_expr() + .map_or(true, |expr_name| expr_name.id != "list") + { + return expr; + } + if let [arg] = arguments.args.as_ref() { + peel_lists(arg) + } else { + expr + } +} + +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) + != Some(1) + { + return None; + }; + value.as_name_expr() +} + +fn extract_name_from_general_reversed(expr: &Expr) -> Option<&ExprName> { + let expr = peel_lists(expr); + extract_name_from_reversed(expr).or_else(|| extract_name_from_sliced_reversed(expr)) +} + +// FURB187 +pub(crate) fn list_assign_reversed(checker: &mut Checker, assign: &StmtAssign) { + let [Expr::Name(target_name_expr)] = assign.targets.as_slice() else { + return; + }; + + let Some(arg_name_expr) = extract_name_from_general_reversed(assign.value.as_ref()) else { + return; + }; + + if arg_name_expr.id != target_name_expr.id { + return; + } + + let Some(binding) = checker + .semantic() + .only_binding(arg_name_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_name_expr.id.to_string(), + }, + assign.range, + ) + .with_fix(Fix::safe_edit(Edit::range_replacement( + format!("{}.reverse()", target_name_expr.id), + assign.range, + ))), + ); +} 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..1cde59e82667b --- /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: Use `l.reverse()` instead + +ℹ Safe fix +3 3 | # using functions to ensure `l` doesn't change type +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: Use `l.reverse()` instead + +ℹ 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: Use `l.reverse()` instead + +ℹ 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 a415b966c0512..cb8aea07b7170 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3023,6 +3023,7 @@ "FURB18", "FURB180", "FURB181", + "FURB187", "G", "G0", "G00", From b82b0c0b7624a5d3f6da83266ec66a0b213e58b5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 21 Mar 2024 12:58:04 -0400 Subject: [PATCH 2/2] Tweaks --- .../resources/test/fixtures/refurb/FURB187.py | 11 +- .../refurb/rules/list_assign_reversed.rs | 177 +++++++++++------- ...es__refurb__tests__FURB187_FURB187.py.snap | 8 +- 3 files changed, 115 insertions(+), 81 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py index 1e272aa86eeea..48b29e896f350 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB187.py @@ -1,6 +1,6 @@ -# these should match +# Errors + -# using functions to ensure `l` doesn't change type def a(): l = [] l = reversed(l) @@ -18,7 +18,7 @@ def c(): # False negative def c2(): - class Wrapper(): + class Wrapper: l: list[int] w = Wrapper() @@ -27,7 +27,8 @@ class Wrapper(): w.l = reversed(w.l) -# these should not +# OK + def d(): l = [] @@ -45,7 +46,7 @@ def e(): def f(): d = {} - # dont warn since d is a dict and does not have a .reverse() method + # Don't warn: `d` is a dictionary, which doesn't have a `reverse` method. d = reversed(d) 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 index 6693da47e9c2f..dc9e853acf561 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs @@ -1,23 +1,32 @@ -use crate::checkers::ast::Checker; 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 uses of assignment of "reversed" expression on the list to the same binging. +/// Checks for list reversals that can be performed in-place in lieu of +/// creating a new list. /// /// ## Why is this bad? -/// -/// Use of in-place method `.reverse()` is faster and allows to avoid copying the name of variable. +/// 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] /// ``` /// @@ -25,8 +34,6 @@ use ruff_python_semantic::analyze::typing; /// ```python /// l = [1, 2, 3] /// l.reverse() -/// l.reverse() -/// l.reverse() /// ``` /// /// ## References @@ -39,31 +46,59 @@ pub struct ListAssignReversed { impl AlwaysFixableViolation for ListAssignReversed { #[derive_message_formats] fn message(&self) -> String { - format!("Use of assignment of `reversed` on list `{}`", self.name) + let ListAssignReversed { name } = self; + format!("Use of assignment of `reversed` on list `{name}`") } fn fix_title(&self) -> String { - format!("Use `{}.reverse()` instead", self.name) + let ListAssignReversed { name } = self; + format!("Replace with `{name}.reverse()`") } } -fn extract_name_from_reversed(expr: &Expr) -> Option<&ExprName> { - let ExprCall { - func, arguments, .. - } = expr.as_call_expr()?; - if !arguments.keywords.is_empty() { - return None; +/// 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 [arg] = arguments.args.as_ref() else { - return None; + + 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; + } - func.as_name_expr() - .is_some_and(|name_expr| name_expr.id == "reversed") - .then(|| arg.as_name_expr()) - .flatten() + 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, .. @@ -71,20 +106,57 @@ fn peel_lists(expr: &Expr) -> &Expr { else { return expr; }; - if !arguments.keywords.is_empty() - || func - .as_name_expr() - .map_or(true, |expr_name| expr_name.id != "list") - { + + if !arguments.keywords.is_empty() { return expr; } - if let [arg] = arguments.args.as_ref() { - peel_lists(arg) - } else { - 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 { @@ -101,57 +173,18 @@ fn extract_name_from_sliced_reversed(expr: &Expr) -> Option<&ExprName> { else { return None; }; - if operand + if !operand .as_number_literal_expr() .and_then(|num| num.value.as_int()) .and_then(Int::as_u8) - != Some(1) + .is_some_and(|value| value == 1) { return None; }; value.as_name_expr() } -fn extract_name_from_general_reversed(expr: &Expr) -> Option<&ExprName> { +fn extract_reversed<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a ExprName> { let expr = peel_lists(expr); - extract_name_from_reversed(expr).or_else(|| extract_name_from_sliced_reversed(expr)) -} - -// FURB187 -pub(crate) fn list_assign_reversed(checker: &mut Checker, assign: &StmtAssign) { - let [Expr::Name(target_name_expr)] = assign.targets.as_slice() else { - return; - }; - - let Some(arg_name_expr) = extract_name_from_general_reversed(assign.value.as_ref()) else { - return; - }; - - if arg_name_expr.id != target_name_expr.id { - return; - } - - let Some(binding) = checker - .semantic() - .only_binding(arg_name_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_name_expr.id.to_string(), - }, - assign.range, - ) - .with_fix(Fix::safe_edit(Edit::range_replacement( - format!("{}.reverse()", target_name_expr.id), - assign.range, - ))), - ); + extract_name_from_reversed(expr, semantic).or_else(|| extract_name_from_sliced_reversed(expr)) } 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 index 1cde59e82667b..43d0f5a1657d0 100644 --- 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 @@ -8,10 +8,10 @@ FURB187.py:6:5: FURB187 [*] Use of assignment of `reversed` on list `l` 6 | l = reversed(l) | ^^^^^^^^^^^^^^^ FURB187 | - = help: Use `l.reverse()` instead + = help: Replace with `l.reverse()` ℹ Safe fix -3 3 | # using functions to ensure `l` doesn't change type +3 3 | 4 4 | def a(): 5 5 | l = [] 6 |- l = reversed(l) @@ -27,7 +27,7 @@ FURB187.py:11:5: FURB187 [*] Use of assignment of `reversed` on list `l` 11 | l = list(reversed(l)) | ^^^^^^^^^^^^^^^^^^^^^ FURB187 | - = help: Use `l.reverse()` instead + = help: Replace with `l.reverse()` ℹ Safe fix 8 8 | @@ -46,7 +46,7 @@ FURB187.py:16:5: FURB187 [*] Use of assignment of `reversed` on list `l` 16 | l = l[::-1] | ^^^^^^^^^^^ FURB187 | - = help: Use `l.reverse()` instead + = help: Replace with `l.reverse()` ℹ Safe fix 13 13 |