[refurb] Implement list_assign_reversed lint (FURB187)

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]))

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 {

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

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(

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

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;

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