Skip to content

Commit

Permalink
Add new potentially unsafe "unpack instead of concatenating" check
Browse files Browse the repository at this point in the history
  • Loading branch information
akx committed Jan 18, 2023
1 parent cdb4700 commit 3d8cbea
Show file tree
Hide file tree
Showing 10 changed files with 176 additions and 0 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 5 additions & 0 deletions resources/test/fixtures/flake8_simplify/SIM701.py
Original file line number Diff line number Diff line change
@@ -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]
3 changes: 3 additions & 0 deletions ruff.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1622,6 +1622,9 @@
"SIM4",
"SIM40",
"SIM401",
"SIM7",
"SIM70",
"SIM701",
"T",
"T1",
"T10",
Expand Down
5 changes: 5 additions & 0 deletions src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/rules/flake8_simplify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
70 changes: 70 additions & 0 deletions src/rules/flake8_simplify/rules/ast_bin_op.rs
Original file line number Diff line number Diff line change
@@ -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<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
}

/// 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);
}
2 changes: 2 additions & 0 deletions src/rules/flake8_simplify/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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,
};
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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: ~

15 changes: 15 additions & 0 deletions src/violations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down

0 comments on commit 3d8cbea

Please sign in to comment.