From d9dcf929d06825c27696d5919b99f6865f78fb07 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 19 Jan 2023 11:05:08 +0200 Subject: [PATCH] Add RUF005 "unpack instead of concatenating" check * Guard against complex splattees * Wrap tuples in parens --- README.md | 1 + resources/test/fixtures/ruff/RUF005.py | 22 +++ ruff.schema.json | 1 + src/checkers/ast.rs | 7 + src/registry.rs | 1 + src/rules/ruff/mod.rs | 1 + src/rules/ruff/rules/mod.rs | 3 + ..._of_concatenating_to_collection_literal.rs | 95 ++++++++++ ..._rules__ruff__tests__RUF005_RUF005.py.snap | 175 ++++++++++++++++++ src/violations.rs | 12 ++ 10 files changed, 318 insertions(+) create mode 100644 resources/test/fixtures/ruff/RUF005.py create mode 100644 src/rules/ruff/rules/unpack_instead_of_concatenating_to_collection_literal.rs create mode 100644 src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF005_RUF005.py.snap diff --git a/README.md b/README.md index 5ea82dcc7b2fa..48734545350eb 100644 --- a/README.md +++ b/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 | 🛠 | diff --git a/resources/test/fixtures/ruff/RUF005.py b/resources/test/fixtures/ruff/RUF005.py new file mode 100644 index 0000000000000..3df2f4b217ace --- /dev/null +++ b/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 diff --git a/ruff.schema.json b/ruff.schema.json index ac1ace87d5d41..429efff4e58b7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1631,6 +1631,7 @@ "RUF002", "RUF003", "RUF004", + "RUF005", "RUF1", "RUF10", "RUF100", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index e4ec6608f799e..245b83b057807 100644 --- a/src/checkers/ast.rs +++ b/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); diff --git a/src/registry.rs b/src/registry.rs index 244f934c01c74..190ee029ef290 100644 --- a/src/registry.rs +++ b/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, ); diff --git a/src/rules/ruff/mod.rs b/src/rules/ruff/mod.rs index 1265dec202974..c6dac563213b7 100644 --- a/src/rules/ruff/mod.rs +++ b/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( diff --git a/src/rules/ruff/rules/mod.rs b/src/rules/ruff/rules/mod.rs index f9b0757d68cc1..4327169ac0f7b 100644 --- a/src/rules/ruff/rules/mod.rs +++ b/src/rules/ruff/rules/mod.rs @@ -5,7 +5,10 @@ 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 { diff --git a/src/rules/ruff/rules/unpack_instead_of_concatenating_to_collection_literal.rs b/src/rules/ruff/rules/unpack_instead_of_concatenating_to_collection_literal.rs new file mode 100644 index 0000000000000..b5cec73bfe327 --- /dev/null +++ b/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 { + 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); +} diff --git a/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF005_RUF005.py.snap b/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF005_RUF005.py.snap new file mode 100644 index 0000000000000..b2ef003638d41 --- /dev/null +++ b/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: ~ + diff --git a/src/violations.rs b/src/violations.rs index 211313b2db3ff..90db30e357d4c 100644 --- a/src/violations.rs +++ b/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!(