Tweaks

Author: charliermarsh

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)
 
 

crates/ruff_linter/src/rules/refurb/rules/list_assign_reversed.rs

@@ -1,32 +1,39 @@
-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]
 /// ```
 ///
 /// Use instead:
 /// ```python
 /// l = [1, 2, 3]
 /// l.reverse()
-/// l.reverse()
-/// l.reverse()
 /// ```
 ///
 /// ## References
@@ -39,52 +46,116 @@ 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, ..
     }) = 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")
-    {
+
+    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))
 }

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 |