Extend RUF005 to recursive and literal-literal concatenations

crates/ruff/resources/test/fixtures/ruff/RUF005.py

@@ -1,9 +1,38 @@
+###
+# Non-fixable Errors.
+###
+foo + [  # This will be preserved, but doesn't prevent the fix
+]
+[*foo] + [  # This will be preserved, but doesn't prevent the fix
+]
+first = [
+    # The order
+    1,  # here
+    2,  # is
+    # extremely
+    3,  # critical
+    # to preserve
+]
+second = first + [
+    # please
+    4,
+    # don't
+    5,
+    # touch
+    6,
+]
+
+
+###
+# Fixable errors.
+###
 class Fun:
     words = ("how", "fun!")
 
     def yay(self):
         return self.words
 
+
 yay = Fun().yay
 
 foo = [4, 5, 6]
@@ -13,36 +42,27 @@ def yay(self):
 spam = quux + (10, 11, 12)
 spom = list(spam)
 eggs = spom + [13, 14, 15]
-elatement = ("we all say", ) + yay()
-excitement = ("we all think", ) + Fun().yay()
-astonishment = ("we all feel", ) + Fun.words
+elatement = ("we all say",) + yay()
+excitement = ("we all think",) + Fun().yay()
+astonishment = ("we all feel",) + Fun.words
 
-chain = ['a', 'b', 'c'] + eggs + list(('yes', 'no', 'pants') + zoob)
+chain = ["a", "b", "c"] + eggs + list(("yes", "no", "pants") + zoob)
 
 baz = () + zoob
 
-first = [
-    # The order
-    1,  # here
-    2,  # is
-    # extremely
-    3,  # critical
-    # to preserve
-]
-second = first + [
-    # please
-    4,
-    # don't
-    5,
-    # touch
-    6,
-]
-
 [] + foo + [
 ]
 
-[] + foo + [  # This will be preserved, but doesn't prevent the fix
-]
+pylint_call = [sys.executable, "-m", "pylint"] + args + [path]
+pylint_call_tuple = (sys.executable, "-m", "pylint") + args + (path, path2)
+b = a + [2, 3] + [4]
+
+
+###
+# Non-errors.
+###
+a = (1,) + [2]
+a = [1, 2] + (3, 4)
 
 # Uses the non-preferred quote style, which should be retained.
 f"{[*a(), 'b']}"

crates/ruff/src/rules/ruff/rules/collection_literal_concatenation.rs

@@ -48,86 +48,136 @@ fn make_splat_elts(
     new_elts
 }
 
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 enum Kind {
     List,
     Tuple,
 }
 
-/// RUF005
-/// This suggestion could be unsafe if the non-literal expression in the
-/// expression has overridden the `__add__` (or `__radd__`) magic methods.
-pub(crate) fn collection_literal_concatenation(checker: &mut Checker, expr: &Expr) {
+/// Recursively merge all the tuples/lists of the expression and return the new suggested expression
+/// along with what kind of expression it is (or return none if the expression is not a concatenation of tuples/lists).
+fn build_new_expr(expr: &Expr) -> Option<(Expr, Kind)> {
     let Expr::BinOp(ast::ExprBinOp { left, op: Operator::Add, right, range: _ }) = expr else {
-        return;
+        return None;
+    };
+
+    let new_left = match left.as_ref() {
+        Expr::BinOp(ast::ExprBinOp { .. }) => match build_new_expr(left) {
+            Some((new_left, _)) => new_left,
+            None => *left.clone(),
+        },
+        _ => *left.clone(),
+    };
+
+    let new_right = match right.as_ref() {
+        Expr::BinOp(ast::ExprBinOp { .. }) => match build_new_expr(right) {
+            Some((new_right, _)) => new_right,
+            None => *right.clone(),
+        },
+        _ => *right.clone(),
     };
 
     // Figure out which way the splat is, and what the kind of the collection is.
-    let (kind, splat_element, other_elements, splat_at_left, ctx) =
-        match (left.as_ref(), right.as_ref()) {
-            (
-                Expr::List(ast::ExprList {
-                    elts: l_elts,
-                    ctx,
-                    range: _,
-                }),
-                _,
-            ) => (Kind::List, right, l_elts, false, ctx),
-            (
-                Expr::Tuple(ast::ExprTuple {
-                    elts: l_elts,
-                    ctx,
-                    range: _,
-                }),
-                _,
-            ) => (Kind::Tuple, right, l_elts, false, ctx),
-            (
-                _,
-                Expr::List(ast::ExprList {
-                    elts: r_elts,
-                    ctx,
-                    range: _,
-                }),
-            ) => (Kind::List, left, r_elts, true, ctx),
-            (
-                _,
-                Expr::Tuple(ast::ExprTuple {
-                    elts: r_elts,
-                    ctx,
-                    range: _,
-                }),
-            ) => (Kind::Tuple, left, r_elts, true, ctx),
-            _ => return,
-        };
+    let (kind, splat_element, other_elements, splat_at_left, ctx) = match (&new_left, &new_right) {
+        (
+            Expr::List(ast::ExprList {
+                elts: l_elts,
+                ctx,
+                range: _,
+            }),
+            _,
+        ) => (Kind::List, &new_right, l_elts, false, ctx),
+        (
+            Expr::Tuple(ast::ExprTuple {
+                elts: l_elts,
+                ctx,
+                range: _,
+            }),
+            _,
+        ) => (Kind::Tuple, &new_right, l_elts, false, ctx),
+        (
+            _,
+            Expr::List(ast::ExprList {
+                elts: r_elts,
+                ctx,
+                range: _,
+            }),
+        ) => (Kind::List, &new_left, r_elts, true, ctx),
+        (
+            _,
+            Expr::Tuple(ast::ExprTuple {
+                elts: r_elts,
+                ctx,
+                range: _,
+            }),
+        ) => (Kind::Tuple, &new_left, r_elts, true, ctx),
+        _ => return None,
+    };
 
     // We'll be a bit conservative here; only calls, names and attribute accesses
     // will be considered as splat elements.
-    if !(splat_element.is_call_expr()
+    let mut new_elts;
+    if splat_element.is_call_expr()
         || splat_element.is_name_expr()
-        || splat_element.is_attribute_expr())
+        || splat_element.is_attribute_expr()
     {
-        return;
+        new_elts = make_splat_elts(splat_element, other_elements, splat_at_left);
+    }
+    // If the splat element is itself a list/tuple, insert them in the other list/tuple.
+    else if (kind == Kind::List && splat_element.is_list_expr())
+        || (kind == Kind::Tuple && splat_element.is_tuple_expr())
+    {
+        new_elts = other_elements.clone();
+
+        let elts = match splat_element {
+            Expr::List(ast::ExprList { elts, .. }) => elts.clone(),
+            Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.clone(),
+            _ => return None,
+        };
+
+        new_elts.extend_from_slice(&elts);
+    } else {
+        return None;
     }
 
     let new_expr = match kind {
         Kind::List => {
             let node = ast::ExprList {
-                elts: make_splat_elts(splat_element, other_elements, splat_at_left),
+                elts: new_elts,
                 ctx: *ctx,
                 range: TextRange::default(),
             };
             node.into()
         }
         Kind::Tuple => {
             let node = ast::ExprTuple {
-                elts: make_splat_elts(splat_element, other_elements, splat_at_left),
+                elts: new_elts,
                 ctx: *ctx,
                 range: TextRange::default(),
             };
             node.into()
         }
     };
 
+    Some((new_expr, kind))
+}
+
+/// RUF005
+/// This suggestion could be unsafe if the non-literal expression in the
+/// expression has overridden the `__add__` (or `__radd__`) magic methods.
+pub(crate) fn collection_literal_concatenation(checker: &mut Checker, expr: &Expr) {
+    // Skip the current node if a diagnostic and fix for the parent node has already been generated.
+    if let Some(Expr::BinOp(ast::ExprBinOp {
+        op: Operator::Add, ..
+    })) = checker.ctx.expr_parent()
+    {
+        return;
+    }
+
+    let Some((new_expr, kind)) = build_new_expr(expr) else {
+        return
+    };
+
     let contents = match kind {
         // Wrap the new expression in parentheses if it was a tuple
         Kind::Tuple => format!("({})", checker.generator().expr(&new_expr)),