From 3d8cbeacc436fc937c2f8624384024a153e2d0d4 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Wed, 18 Jan 2023 16:13:11 +0200 Subject: [PATCH] Add new potentially unsafe "unpack instead of concatenating" check --- README.md | 1 + .../test/fixtures/flake8_simplify/SIM701.py | 5 ++ ruff.schema.json | 3 + src/checkers/ast.rs | 5 ++ src/registry.rs | 1 + src/rules/flake8_simplify/mod.rs | 1 + src/rules/flake8_simplify/rules/ast_bin_op.rs | 70 ++++++++++++++++++ src/rules/flake8_simplify/rules/mod.rs | 2 + ...ke8_simplify__tests__SIM701_SIM701.py.snap | 73 +++++++++++++++++++ src/violations.rs | 15 ++++ 10 files changed, 176 insertions(+) create mode 100644 resources/test/fixtures/flake8_simplify/SIM701.py create mode 100644 src/rules/flake8_simplify/rules/ast_bin_op.rs create mode 100644 src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM701_SIM701.py.snap diff --git a/README.md b/README.md index 3ef4d6e04aff1c..9ea93a90c2bae4 100644 --- a/README.md +++ b/README.md @@ -1028,6 +1028,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/0.19.3/ | SIM223 | AndFalse | Use `False` instead of `... and False` | 🛠 | | SIM300 | YodaConditions | Yoda conditions are discouraged, use `left == right` instead | 🛠 | | SIM401 | DictGetWithDefault | Use `var = dict.get(key, "default")` instead of an `if` block | 🛠 | +| SIM701 | UnpackInsteadOfConcatenatingToCollectionLiteral | Use `[1, 2, 3, *bar]` instead of concatenation | | ### flake8-tidy-imports (TID) diff --git a/resources/test/fixtures/flake8_simplify/SIM701.py b/resources/test/fixtures/flake8_simplify/SIM701.py new file mode 100644 index 00000000000000..1d2765a9ca033e --- /dev/null +++ b/resources/test/fixtures/flake8_simplify/SIM701.py @@ -0,0 +1,5 @@ +foo = [4, 5, 6] +bar = [1, 2, 3] + foo +quux = (7, 8, 9) + bar +spam = quux + (10, 11, 12) +eggs = spam + [13, 14, 15] diff --git a/ruff.schema.json b/ruff.schema.json index 655114b43c1535..e6a7de7d858eaa 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1622,6 +1622,9 @@ "SIM4", "SIM40", "SIM401", + "SIM7", + "SIM70", + "SIM701", "T", "T1", "T10", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 572f8b62e92992..6370112442a4ca 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -2461,6 +2461,11 @@ where self.diagnostics.push(diagnostic); } } + if self.settings.rules.enabled(&RuleCode::SIM701) { + flake8_simplify::rules::unpack_instead_of_concatenating_to_collection_literal( + self, expr, + ); + } } ExprKind::UnaryOp { op, operand } => { let check_not_in = self.settings.rules.enabled(&RuleCode::E713); diff --git a/src/registry.rs b/src/registry.rs index a9bb57d67f3ff3..c7e443db15b12f 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -222,6 +222,7 @@ ruff_macros::define_rule_mapping!( SIM223 => violations::AndFalse, SIM300 => violations::YodaConditions, SIM401 => violations::DictGetWithDefault, + SIM701 => violations::UnpackInsteadOfConcatenatingToCollectionLiteral, // pyupgrade UP001 => violations::UselessMetaclassType, UP003 => violations::TypeOfPrimitive, diff --git a/src/rules/flake8_simplify/mod.rs b/src/rules/flake8_simplify/mod.rs index 1ea2a3b4e782ee..427a7c8aa26e89 100644 --- a/src/rules/flake8_simplify/mod.rs +++ b/src/rules/flake8_simplify/mod.rs @@ -38,6 +38,7 @@ mod tests { #[test_case(RuleCode::SIM223, Path::new("SIM223.py"); "SIM223")] #[test_case(RuleCode::SIM300, Path::new("SIM300.py"); "SIM300")] #[test_case(RuleCode::SIM401, Path::new("SIM401.py"); "SIM401")] + #[test_case(RuleCode::SIM701, Path::new("SIM701.py"); "SIM701")] fn rules(rule_code: RuleCode, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/src/rules/flake8_simplify/rules/ast_bin_op.rs b/src/rules/flake8_simplify/rules/ast_bin_op.rs new file mode 100644 index 00000000000000..8dd309c7419f58 --- /dev/null +++ b/src/rules/flake8_simplify/rules/ast_bin_op.rs @@ -0,0 +1,70 @@ +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; +use rustpython_ast::{Expr, ExprContext, ExprKind, Operator}; + +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 +} + +/// SIM701 (not in flake8-simplify) +/// NB: this could be unsafe if the non-literal expression has overridden __add__ +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; + } + let new_expr = match (&left.node, &right.node) { + (ExprKind::List { elts: l_elts, ctx }, _) => create_expr(ExprKind::List { + elts: make_splat_elts(right, l_elts, false), + ctx: ctx.clone(), + }), + (ExprKind::Tuple { elts: l_elts, ctx }, _) => create_expr(ExprKind::Tuple { + elts: make_splat_elts(right, l_elts, false), + ctx: ctx.clone(), + }), + (_, ExprKind::List { elts: r_elts, ctx }) => create_expr(ExprKind::List { + elts: make_splat_elts(left, r_elts, true), + ctx: ctx.clone(), + }), + (_, ExprKind::Tuple { elts: r_elts, ctx }) => create_expr(ExprKind::Tuple { + elts: make_splat_elts(left, r_elts, true), + ctx: ctx.clone(), + }), + _ => return, + }; + + let new_expr_string = unparse_expr(&new_expr, checker.stylist); + + let mut diagnostic = Diagnostic::new( + violations::UnpackInsteadOfConcatenatingToCollectionLiteral(new_expr_string.clone()), + Range::from_located(expr), + ); + if checker.patch(diagnostic.kind.code()) { + diagnostic.amend(Fix::replacement( + new_expr_string, + expr.location, + expr.end_location.unwrap(), + )); + } + checker.diagnostics.push(diagnostic); +} diff --git a/src/rules/flake8_simplify/rules/mod.rs b/src/rules/flake8_simplify/rules/mod.rs index b50dc07257aacf..0eca58cd7db392 100644 --- a/src/rules/flake8_simplify/rules/mod.rs +++ b/src/rules/flake8_simplify/rules/mod.rs @@ -1,3 +1,4 @@ +pub use ast_bin_op::unpack_instead_of_concatenating_to_collection_literal; pub use ast_bool_op::{ a_and_not_a, a_or_not_a, and_false, compare_with_tuple, duplicate_isinstance_call, or_true, }; @@ -18,6 +19,7 @@ pub use return_in_try_except_finally::return_in_try_except_finally; pub use use_contextlib_suppress::use_contextlib_suppress; pub use yoda_conditions::yoda_conditions; +mod ast_bin_op; mod ast_bool_op; mod ast_expr; mod ast_for; diff --git a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM701_SIM701.py.snap b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM701_SIM701.py.snap new file mode 100644 index 00000000000000..683cabaa914955 --- /dev/null +++ b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM701_SIM701.py.snap @@ -0,0 +1,73 @@ +--- +source: src/rules/flake8_simplify/mod.rs +expression: diagnostics +--- +- kind: + UnpackInsteadOfConcatenatingToCollectionLiteral: "[1, 2, 3, *foo]" + location: + row: 2 + column: 6 + end_location: + row: 2 + column: 21 + fix: + content: "[1, 2, 3, *foo]" + location: + row: 2 + column: 6 + end_location: + row: 2 + column: 21 + parent: ~ +- kind: + UnpackInsteadOfConcatenatingToCollectionLiteral: "7, 8, 9, *bar" + location: + row: 3 + column: 7 + end_location: + row: 3 + column: 22 + fix: + content: "7, 8, 9, *bar" + location: + row: 3 + column: 7 + end_location: + row: 3 + column: 22 + parent: ~ +- kind: + UnpackInsteadOfConcatenatingToCollectionLiteral: "*quux, 10, 11, 12" + location: + row: 4 + column: 7 + end_location: + row: 4 + column: 26 + fix: + content: "*quux, 10, 11, 12" + location: + row: 4 + column: 7 + end_location: + row: 4 + column: 26 + parent: ~ +- kind: + UnpackInsteadOfConcatenatingToCollectionLiteral: "[*spam, 13, 14, 15]" + location: + row: 5 + column: 7 + end_location: + row: 5 + column: 26 + fix: + content: "[*spam, 13, 14, 15]" + location: + row: 5 + column: 7 + end_location: + row: 5 + column: 26 + parent: ~ + diff --git a/src/violations.rs b/src/violations.rs index 9ee414c20b6a91..a6097cdc1c15e2 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -3222,6 +3222,21 @@ impl AlwaysAutofixableViolation for DictGetWithDefault { DictGetWithDefault("var = dict.get(key, \"default\")".to_string()) } } + +define_violation!( + pub struct UnpackInsteadOfConcatenatingToCollectionLiteral(pub String); +); +impl Violation for UnpackInsteadOfConcatenatingToCollectionLiteral { + fn message(&self) -> String { + let UnpackInsteadOfConcatenatingToCollectionLiteral(expr) = self; + format!("Use `{expr}` instead of concatenation") + } + + fn placeholder() -> Self { + UnpackInsteadOfConcatenatingToCollectionLiteral("[1, 2, 3, *bar]".to_string()) + } +} + // pyupgrade define_violation!(