[refurb] Implement list_assign_reversed lint (FURB187)

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

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 {

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

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

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: 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