Add RUF005 "unpack instead of concatenating" check (#1957)

This PR adds a new check that turns expressions such as `[1, 2, 3] + foo` into `[1, 2, 3, *foo]`, since the latter is easier to read and faster:

```
~ $ python3.11 -m timeit -s 'b = [6, 5, 4]' '[1, 2, 3] + b'
5000000 loops, best of 5: 81.4 nsec per loop
~ $ python3.11 -m timeit -s 'b = [6, 5, 4]' '[1, 2, 3, *b]'
5000000 loops, best of 5: 66.2 nsec per loop
```

However there's a couple of gotchas:

* This felt like a `simplify` rule, so I borrowed an unused `SIM` code even if the upstream `flake8-simplify` doesn't do this transform. If it should be assigned some other code, let me know 😄 
* **More importantly** this transform could be unsafe if the other operand of the `+` operation has overridden `__add__` to do something else. What's the `ruff` policy around potentially unsafe operations? (I think some of the suggestions other ported rules give could be semantically different from the original code, but I'm not sure.)
* I'm not a very established Rustacean, so there's no doubt my code isn't quite idiomatic. (For instance, is there a neater way to write that four-way `match` statement?)

Thanks for `ruff`, by the way! :)

README.md

@@ -1174,6 +1174,7 @@ For more, see [flake8-no-pep420](https://pypi.org/project/flake8-no-pep420/2.3.0
 | RUF002 | AmbiguousUnicodeCharacterDocstring | Docstring contains ambiguous unicode character '{confusable}' (did you mean '{representant}'?) | 🛠 |
 | RUF003 | AmbiguousUnicodeCharacterComment | Comment contains ambiguous unicode character '{confusable}' (did you mean '{representant}'?) | 🛠 |
 | RUF004 | KeywordArgumentBeforeStarArgument | Keyword argument `{name}` must come after starred arguments |  |
+| RUF005 | UnpackInsteadOfConcatenatingToCollectionLiteral | Consider `{expr}` instead of concatenation |  |
 | RUF100 | UnusedNOQA | Unused blanket `noqa` directive | 🛠 |
 
 <!-- End auto-generated sections. -->

resources/test/fixtures/ruff/RUF005.py

@@ -0,0 +1,22 @@
+class Fun:
+    words = ("how", "fun!")
+
+    def yay(self):
+        return self.words
+
+yay = Fun().yay
+
+foo = [4, 5, 6]
+bar = [1, 2, 3] + foo
+zoob = tuple(bar)
+quux = (7, 8, 9) + zoob
+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
+
+chain = ['a', 'b', 'c'] + eggs + list(('yes', 'no', 'pants') + zoob)
+
+baz = () + zoob

ruff.schema.json

@@ -1631,6 +1631,7 @@
         "RUF002",
         "RUF003",
         "RUF004",
+        "RUF005",
         "RUF1",
         "RUF10",
         "RUF100",

src/checkers/ast.rs

@@ -2722,6 +2722,13 @@ where
                         self.diagnostics.push(diagnostic);
                     }
                 }
+                if self
+                    .settings
+                    .rules
+                    .enabled(&Rule::UnpackInsteadOfConcatenatingToCollectionLiteral)
+                {
+                    ruff::rules::unpack_instead_of_concatenating_to_collection_literal(self, expr);
+                }
             }
             ExprKind::UnaryOp { op, operand } => {
                 let check_not_in = self.settings.rules.enabled(&Rule::NotInTest);

src/registry.rs

@@ -421,6 +421,7 @@ ruff_macros::define_rule_mapping!(
     RUF002 => violations::AmbiguousUnicodeCharacterDocstring,
     RUF003 => violations::AmbiguousUnicodeCharacterComment,
     RUF004 => violations::KeywordArgumentBeforeStarArgument,
+    RUF005 => violations::UnpackInsteadOfConcatenatingToCollectionLiteral,
     RUF100 => violations::UnusedNOQA,
 );
 

src/rules/ruff/mod.rs

@@ -14,6 +14,7 @@ mod tests {
     use crate::registry::Rule;
     use crate::settings;
     #[test_case(Rule::KeywordArgumentBeforeStarArgument, Path::new("RUF004.py"); "RUF004")]
+    #[test_case(Rule::UnpackInsteadOfConcatenatingToCollectionLiteral, Path::new("RUF005.py"); "RUF005")]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.code(), path.to_string_lossy());
         let diagnostics = test_path(

src/rules/ruff/rules/ambiguous_unicode_character.rs

@@ -1,10 +1,11 @@
 use once_cell::sync::Lazy;
 use rustc_hash::FxHashMap;
-use rustpython_ast::{Expr, ExprKind, Keyword, KeywordData, Location};
 
 use crate::ast::types::Range;
 use crate::fix::Fix;
+use crate::message::Location;
 use crate::registry::{Diagnostic, DiagnosticKind};
+use crate::rules::ruff::rules::Context;
 use crate::settings::{flags, Settings};
 use crate::source_code::Locator;
 use crate::violations;
@@ -1597,13 +1598,6 @@ static CONFUSABLES: Lazy<FxHashMap<u32, u32>> = Lazy::new(|| {
     ])
 });
 
-#[derive(Clone, Copy)]
-pub enum Context {
-    String,
-    Docstring,
-    Comment,
-}
-
 pub fn ambiguous_unicode_character(
     locator: &Locator,
     start: Location,
@@ -1677,28 +1671,3 @@ pub fn ambiguous_unicode_character(
 
     diagnostics
 }
-
-/// RUF004
-pub fn keyword_argument_before_star_argument(
-    args: &[Expr],
-    keywords: &[Keyword],
-) -> Vec<Diagnostic> {
-    let mut diagnostics = vec![];
-    if let Some(arg) = args
-        .iter()
-        .rfind(|arg| matches!(arg.node, ExprKind::Starred { .. }))
-    {
-        for keyword in keywords {
-            if keyword.location < arg.location {
-                let KeywordData { arg, .. } = &keyword.node;
-                if let Some(arg) = arg {
-                    diagnostics.push(Diagnostic::new(
-                        violations::KeywordArgumentBeforeStarArgument(arg.to_string()),
-                        Range::from_located(keyword),
-                    ));
-                }
-            }
-        }
-    }
-    diagnostics
-}

src/rules/ruff/rules/mod.rs

@@ -0,0 +1,43 @@
+use rustpython_ast::{Expr, ExprKind, Keyword, KeywordData};
+
+use crate::ast::types::Range;
+use crate::registry::Diagnostic;
+use crate::violations;
+
+mod ambiguous_unicode_character;
+mod unpack_instead_of_concatenating_to_collection_literal;
+
+pub use ambiguous_unicode_character::ambiguous_unicode_character;
+pub use unpack_instead_of_concatenating_to_collection_literal::unpack_instead_of_concatenating_to_collection_literal;
+
+#[derive(Clone, Copy)]
+pub enum Context {
+    String,
+    Docstring,
+    Comment,
+}
+
+/// RUF004
+pub fn keyword_argument_before_star_argument(
+    args: &[Expr],
+    keywords: &[Keyword],
+) -> Vec<Diagnostic> {
+    let mut diagnostics = vec![];
+    if let Some(arg) = args
+        .iter()
+        .rfind(|arg| matches!(arg.node, ExprKind::Starred { .. }))
+    {
+        for keyword in keywords {
+            if keyword.location < arg.location {
+                let KeywordData { arg, .. } = &keyword.node;
+                if let Some(arg) = arg {
+                    diagnostics.push(Diagnostic::new(
+                        violations::KeywordArgumentBeforeStarArgument(arg.to_string()),
+                        Range::from_located(keyword),
+                    ));
+                }
+            }
+        }
+    }
+    diagnostics
+}

src/rules/ruff/rules/unpack_instead_of_concatenating_to_collection_literal.rs

@@ -0,0 +1,95 @@
+use rustpython_ast::{Expr, ExprContext, ExprKind, Operator};
+
+use crate::ast::helpers::{create_expr, unparse_expr};
+use crate::ast::types::Range;
+use crate::checkers::ast::Checker;
+use crate::fix::Fix;
+use crate::registry::Diagnostic;
+use crate::violations;
+
+fn make_splat_elts(
+    splat_element: &Expr,
+    other_elements: &[Expr],
+    splat_at_left: bool,
+) -> Vec<Expr> {
+    let mut new_elts = other_elements.to_owned();
+    let splat = create_expr(ExprKind::Starred {
+        value: Box::from(splat_element.clone()),
+        ctx: ExprContext::Load,
+    });
+    if splat_at_left {
+        new_elts.insert(0, splat);
+    } else {
+        new_elts.push(splat);
+    }
+    new_elts
+}
+
+#[derive(Debug)]
+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 fn unpack_instead_of_concatenating_to_collection_literal(checker: &mut Checker, expr: &Expr) {
+    let ExprKind::BinOp { op, left, right } = &expr.node else {
+        return;
+    };
+    if !matches!(op, Operator::Add) {
+        return;
+    }
+
+    // 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.node, &right.node)
+    {
+        (ExprKind::List { elts: l_elts, ctx }, _) => (Kind::List, right, l_elts, false, ctx),
+        (ExprKind::Tuple { elts: l_elts, ctx }, _) => (Kind::Tuple, right, l_elts, false, ctx),
+        (_, ExprKind::List { elts: r_elts, ctx }) => (Kind::List, left, r_elts, true, ctx),
+        (_, ExprKind::Tuple { elts: r_elts, ctx }) => (Kind::Tuple, left, r_elts, true, ctx),
+        _ => return,
+    };
+
+    // We'll be a bit conservative here; only calls, names and attribute accesses
+    // will be considered as splat elements.
+    if !matches!(
+        splat_element.node,
+        ExprKind::Call { .. } | ExprKind::Name { .. } | ExprKind::Attribute { .. }
+    ) {
+        return;
+    }
+
+    let new_expr = match kind {
+        Kind::List => create_expr(ExprKind::List {
+            elts: make_splat_elts(splat_element, other_elements, splat_at_left),
+            ctx: ctx.clone(),
+        }),
+        Kind::Tuple => create_expr(ExprKind::Tuple {
+            elts: make_splat_elts(splat_element, other_elements, splat_at_left),
+            ctx: ctx.clone(),
+        }),
+    };
+
+    let mut new_expr_string = unparse_expr(&new_expr, checker.stylist);
+
+    new_expr_string = match kind {
+        // Wrap the new expression in parentheses if it was a tuple
+        Kind::Tuple => format!("({new_expr_string})"),
+        Kind::List => new_expr_string,
+    };
+
+    let mut diagnostic = Diagnostic::new(
+        violations::UnpackInsteadOfConcatenatingToCollectionLiteral(new_expr_string.clone()),
+        Range::from_located(expr),
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        diagnostic.amend(Fix::replacement(
+            new_expr_string,
+            expr.location,
+            expr.end_location.unwrap(),
+        ));
+    }
+    checker.diagnostics.push(diagnostic);
+}

src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF005_RUF005.py.snap

@@ -0,0 +1,175 @@
+---
+source: src/rules/ruff/mod.rs
+expression: diagnostics
+---
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "[1, 2, 3, *foo]"
+  location:
+    row: 10
+    column: 6
+  end_location:
+    row: 10
+    column: 21
+  fix:
+    content: "[1, 2, 3, *foo]"
+    location:
+      row: 10
+      column: 6
+    end_location:
+      row: 10
+      column: 21
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(7, 8, 9, *zoob)"
+  location:
+    row: 12
+    column: 7
+  end_location:
+    row: 12
+    column: 23
+  fix:
+    content: "(7, 8, 9, *zoob)"
+    location:
+      row: 12
+      column: 7
+    end_location:
+      row: 12
+      column: 23
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(*quux, 10, 11, 12)"
+  location:
+    row: 13
+    column: 7
+  end_location:
+    row: 13
+    column: 26
+  fix:
+    content: "(*quux, 10, 11, 12)"
+    location:
+      row: 13
+      column: 7
+    end_location:
+      row: 13
+      column: 26
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "[*spom, 13, 14, 15]"
+  location:
+    row: 15
+    column: 7
+  end_location:
+    row: 15
+    column: 26
+  fix:
+    content: "[*spom, 13, 14, 15]"
+    location:
+      row: 15
+      column: 7
+    end_location:
+      row: 15
+      column: 26
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(\"we all say\", *yay())"
+  location:
+    row: 16
+    column: 12
+  end_location:
+    row: 16
+    column: 36
+  fix:
+    content: "(\"we all say\", *yay())"
+    location:
+      row: 16
+      column: 12
+    end_location:
+      row: 16
+      column: 36
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(\"we all think\", *Fun().yay())"
+  location:
+    row: 17
+    column: 13
+  end_location:
+    row: 17
+    column: 45
+  fix:
+    content: "(\"we all think\", *Fun().yay())"
+    location:
+      row: 17
+      column: 13
+    end_location:
+      row: 17
+      column: 45
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(\"we all feel\", *Fun.words)"
+  location:
+    row: 18
+    column: 15
+  end_location:
+    row: 18
+    column: 44
+  fix:
+    content: "(\"we all feel\", *Fun.words)"
+    location:
+      row: 18
+      column: 15
+    end_location:
+      row: 18
+      column: 44
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "[\"a\", \"b\", \"c\", *eggs]"
+  location:
+    row: 20
+    column: 8
+  end_location:
+    row: 20
+    column: 30
+  fix:
+    content: "[\"a\", \"b\", \"c\", *eggs]"
+    location:
+      row: 20
+      column: 8
+    end_location:
+      row: 20
+      column: 30
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(\"yes\", \"no\", \"pants\", *zoob)"
+  location:
+    row: 20
+    column: 38
+  end_location:
+    row: 20
+    column: 67
+  fix:
+    content: "(\"yes\", \"no\", \"pants\", *zoob)"
+    location:
+      row: 20
+      column: 38
+    end_location:
+      row: 20
+      column: 67
+  parent: ~
+- kind:
+    UnpackInsteadOfConcatenatingToCollectionLiteral: "(*zoob,)"
+  location:
+    row: 22
+    column: 6
+  end_location:
+    row: 22
+    column: 15
+  fix:
+    content: "(*zoob,)"
+    location:
+      row: 22
+      column: 6
+    end_location:
+      row: 22
+      column: 15
+  parent: ~
+

src/violations.rs

@@ -2652,6 +2652,18 @@ impl AlwaysAutofixableViolation for DictGetWithDefault {
         format!("Replace with `{contents}`")
     }
 }
+
+define_violation!(
+    pub struct UnpackInsteadOfConcatenatingToCollectionLiteral(pub String);
+);
+impl Violation for UnpackInsteadOfConcatenatingToCollectionLiteral {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let UnpackInsteadOfConcatenatingToCollectionLiteral(expr) = self;
+        format!("Consider `{expr}` instead of concatenation")
+    }
+}
+
 // pyupgrade
 
 define_violation!(