From cf384d428885207007b81df92005458d1cd3753f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 21 Mar 2024 12:58:04 -0400 Subject: [PATCH] Tweaks --- .../resources/test/fixtures/refurb/FURB187.py | 11 +- .../refurb/rules/list_assign_reversed.rs | 176 +++++++++++------- ...es__refurb__tests__FURB187_FURB187.py.snap | 8 +- 3 files changed, 114 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 1e272aa86eeea2..48b29e896f3501 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 6693da47e9c2f3..803ad549ddb483 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,56 @@ 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 !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; } - if let [arg] = arguments.args.as_ref() { - peel_lists(arg) - } else { - expr + let [arg] = arguments.args.as_ref() else { + return None; + }; + let Some(arg) = func + .as_name_expr() + .is_some_and(|name| name.id == "reversed") + .then(|| arg.as_name_expr()) + .flatten() + else { + return None; + }; + 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 +172,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 1cde59e82667bb..43d0f5a1657d03 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 |