diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE800.py b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE800.py index 3584d2c03f32a..56a74de9c0e58 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE800.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE800.py @@ -1,9 +1,21 @@ {"foo": 1, **{"bar": 1}} # PIE800 +{**{"bar": 10}, "a": "b"} # PIE800 + foo({**foo, **{"bar": True}}) # PIE800 {**foo, **{"bar": 10}} # PIE800 +{ # PIE800 + "a": "b", + # Preserve + **{ + # all + "bar": 10, # the + # comments + }, +} + {**foo, **buzz, **{bar: 10}} # PIE800 {**foo, "bar": True } # OK diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index b975e4db472d1..99f958684de32 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -936,13 +936,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { flake8_trio::rules::zero_sleep_call(checker, call); } } - Expr::Dict( - dict @ ast::ExprDict { - keys, - values, - range: _, - }, - ) => { + Expr::Dict(dict) => { if checker.any_enabled(&[ Rule::MultiValueRepeatedKeyLiteral, Rule::MultiValueRepeatedKeyVariable, @@ -950,7 +944,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { pyflakes::rules::repeated_keys(checker, dict); } if checker.enabled(Rule::UnnecessarySpread) { - flake8_pie::rules::unnecessary_spread(checker, keys, values); + flake8_pie::rules::unnecessary_spread(checker, dict); } } Expr::Set(ast::ExprSet { elts, range: _ }) => { diff --git a/crates/ruff_linter/src/rules/flake8_pie/mod.rs b/crates/ruff_linter/src/rules/flake8_pie/mod.rs index 3cf692c3625c3..238c239add39e 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/mod.rs @@ -32,6 +32,7 @@ mod tests { } #[test_case(Rule::UnnecessaryPlaceholder, Path::new("PIE790.py"))] + #[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"))] #[test_case(Rule::ReimplementedContainerBuiltin, Path::new("PIE807.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs index 5edc51da5393d..12b33b6075548 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs @@ -1,9 +1,10 @@ -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr}; -use ruff_diagnostics::Diagnostic; -use ruff_diagnostics::Violation; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_text_size::Ranged; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -32,22 +33,76 @@ use crate::checkers::ast::Checker; pub struct UnnecessarySpread; impl Violation for UnnecessarySpread { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Unnecessary spread `**`") } + + fn fix_title(&self) -> Option { + Some(format!("Remove unnecessary dict")) + } } /// PIE800 -pub(crate) fn unnecessary_spread(checker: &mut Checker, keys: &[Option], values: &[Expr]) { - for item in keys.iter().zip(values.iter()) { +pub(crate) fn unnecessary_spread(checker: &mut Checker, dict: &ast::ExprDict) { + // The first "end" is the start of the dictionary, immediately following the open bracket. + let mut prev_end = dict.start() + TextSize::from(1); + for item in dict.keys.iter().zip(dict.values.iter()) { if let (None, value) = item { // We only care about when the key is None which indicates a spread `**` // inside a dict. - if let Expr::Dict(_) = value { - let diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); + if let Expr::Dict(inner) = value { + let mut diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); + if checker.settings.preview.is_enabled() { + if let Some(fix) = unnecessary_spread_fix(inner, prev_end, checker.locator()) { + diagnostic.set_fix(fix); + } + } checker.diagnostics.push(diagnostic); } } + prev_end = item.1.end(); + } +} + +/// Generate a [`Fix`] to remove an unnecessary dictionary spread. +fn unnecessary_spread_fix( + dict: &ast::ExprDict, + prev_end: TextSize, + locator: &Locator, +) -> Option { + // Find the `**` token preceding the spread. + let doublestar = SimpleTokenizer::starts_at(prev_end, locator.contents()) + .find(|tok| matches!(tok.kind(), SimpleTokenKind::DoubleStar))?; + + if let Some(last) = dict.values.last() { + // Ex) `**{a: 1, b: 2}` + let mut edits = vec![]; + for tok in SimpleTokenizer::starts_at(last.end(), locator.contents()).skip_trivia() { + match tok.kind() { + SimpleTokenKind::Comma => { + edits.push(Edit::range_deletion(tok.range())); + } + SimpleTokenKind::RBrace => { + edits.push(Edit::range_deletion(tok.range())); + break; + } + _ => {} + } + } + Some(Fix::safe_edits( + // Delete the first `**{` + Edit::deletion(doublestar.start(), dict.start() + TextSize::from(1)), + // Delete the trailing `}` + edits, + )) + } else { + // Ex) `**{}` + Some(Fix::safe_edit(Edit::deletion( + doublestar.start(), + dict.end(), + ))) } } diff --git a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE800_PIE800.py.snap b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE800_PIE800.py.snap index 16d06e62b780f..ad6c9e293eead 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE800_PIE800.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE800_PIE800.py.snap @@ -6,37 +6,67 @@ PIE800.py:1:14: PIE800 Unnecessary spread `**` 1 | {"foo": 1, **{"bar": 1}} # PIE800 | ^^^^^^^^^^ PIE800 2 | -3 | foo({**foo, **{"bar": True}}) # PIE800 +3 | {**{"bar": 10}, "a": "b"} # PIE800 | + = help: Remove unnecessary dict -PIE800.py:3:15: PIE800 Unnecessary spread `**` +PIE800.py:3:4: PIE800 Unnecessary spread `**` | 1 | {"foo": 1, **{"bar": 1}} # PIE800 2 | -3 | foo({**foo, **{"bar": True}}) # PIE800 - | ^^^^^^^^^^^^^ PIE800 +3 | {**{"bar": 10}, "a": "b"} # PIE800 + | ^^^^^^^^^^^ PIE800 4 | -5 | {**foo, **{"bar": 10}} # PIE800 +5 | foo({**foo, **{"bar": True}}) # PIE800 | + = help: Remove unnecessary dict -PIE800.py:5:11: PIE800 Unnecessary spread `**` +PIE800.py:5:15: PIE800 Unnecessary spread `**` | -3 | foo({**foo, **{"bar": True}}) # PIE800 +3 | {**{"bar": 10}, "a": "b"} # PIE800 4 | -5 | {**foo, **{"bar": 10}} # PIE800 - | ^^^^^^^^^^^ PIE800 +5 | foo({**foo, **{"bar": True}}) # PIE800 + | ^^^^^^^^^^^^^ PIE800 6 | -7 | {**foo, **buzz, **{bar: 10}} # PIE800 +7 | {**foo, **{"bar": 10}} # PIE800 | + = help: Remove unnecessary dict -PIE800.py:7:19: PIE800 Unnecessary spread `**` +PIE800.py:7:11: PIE800 Unnecessary spread `**` | -5 | {**foo, **{"bar": 10}} # PIE800 +5 | foo({**foo, **{"bar": True}}) # PIE800 6 | -7 | {**foo, **buzz, **{bar: 10}} # PIE800 - | ^^^^^^^^^ PIE800 +7 | {**foo, **{"bar": 10}} # PIE800 + | ^^^^^^^^^^^ PIE800 8 | -9 | {**foo, "bar": True } # OK +9 | { # PIE800 | + = help: Remove unnecessary dict + +PIE800.py:12:7: PIE800 Unnecessary spread `**` + | +10 | "a": "b", +11 | # Preserve +12 | **{ + | _______^ +13 | | # all +14 | | "bar": 10, # the +15 | | # comments +16 | | }, + | |_____^ PIE800 +17 | } + | + = help: Remove unnecessary dict + +PIE800.py:19:19: PIE800 Unnecessary spread `**` + | +17 | } +18 | +19 | {**foo, **buzz, **{bar: 10}} # PIE800 + | ^^^^^^^^^ PIE800 +20 | +21 | {**foo, "bar": True } # OK + | + = help: Remove unnecessary dict diff --git a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__preview__PIE800_PIE800.py.snap b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__preview__PIE800_PIE800.py.snap new file mode 100644 index 0000000000000..217e4c71f3a21 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__preview__PIE800_PIE800.py.snap @@ -0,0 +1,134 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pie/mod.rs +--- +PIE800.py:1:14: PIE800 [*] Unnecessary spread `**` + | +1 | {"foo": 1, **{"bar": 1}} # PIE800 + | ^^^^^^^^^^ PIE800 +2 | +3 | {**{"bar": 10}, "a": "b"} # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +1 |-{"foo": 1, **{"bar": 1}} # PIE800 + 1 |+{"foo": 1, "bar": 1} # PIE800 +2 2 | +3 3 | {**{"bar": 10}, "a": "b"} # PIE800 +4 4 | + +PIE800.py:3:4: PIE800 [*] Unnecessary spread `**` + | +1 | {"foo": 1, **{"bar": 1}} # PIE800 +2 | +3 | {**{"bar": 10}, "a": "b"} # PIE800 + | ^^^^^^^^^^^ PIE800 +4 | +5 | foo({**foo, **{"bar": True}}) # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +1 1 | {"foo": 1, **{"bar": 1}} # PIE800 +2 2 | +3 |-{**{"bar": 10}, "a": "b"} # PIE800 + 3 |+{"bar": 10, "a": "b"} # PIE800 +4 4 | +5 5 | foo({**foo, **{"bar": True}}) # PIE800 +6 6 | + +PIE800.py:5:15: PIE800 [*] Unnecessary spread `**` + | +3 | {**{"bar": 10}, "a": "b"} # PIE800 +4 | +5 | foo({**foo, **{"bar": True}}) # PIE800 + | ^^^^^^^^^^^^^ PIE800 +6 | +7 | {**foo, **{"bar": 10}} # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +2 2 | +3 3 | {**{"bar": 10}, "a": "b"} # PIE800 +4 4 | +5 |-foo({**foo, **{"bar": True}}) # PIE800 + 5 |+foo({**foo, "bar": True}) # PIE800 +6 6 | +7 7 | {**foo, **{"bar": 10}} # PIE800 +8 8 | + +PIE800.py:7:11: PIE800 [*] Unnecessary spread `**` + | +5 | foo({**foo, **{"bar": True}}) # PIE800 +6 | +7 | {**foo, **{"bar": 10}} # PIE800 + | ^^^^^^^^^^^ PIE800 +8 | +9 | { # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +4 4 | +5 5 | foo({**foo, **{"bar": True}}) # PIE800 +6 6 | +7 |-{**foo, **{"bar": 10}} # PIE800 + 7 |+{**foo, "bar": 10} # PIE800 +8 8 | +9 9 | { # PIE800 +10 10 | "a": "b", + +PIE800.py:12:7: PIE800 [*] Unnecessary spread `**` + | +10 | "a": "b", +11 | # Preserve +12 | **{ + | _______^ +13 | | # all +14 | | "bar": 10, # the +15 | | # comments +16 | | }, + | |_____^ PIE800 +17 | } + | + = help: Remove unnecessary dict + +ℹ Safe fix +9 9 | { # PIE800 +10 10 | "a": "b", +11 11 | # Preserve +12 |- **{ + 12 |+ +13 13 | # all +14 |- "bar": 10, # the + 14 |+ "bar": 10 # the +15 15 | # comments +16 |- }, + 16 |+ , +17 17 | } +18 18 | +19 19 | {**foo, **buzz, **{bar: 10}} # PIE800 + +PIE800.py:19:19: PIE800 [*] Unnecessary spread `**` + | +17 | } +18 | +19 | {**foo, **buzz, **{bar: 10}} # PIE800 + | ^^^^^^^^^ PIE800 +20 | +21 | {**foo, "bar": True } # OK + | + = help: Remove unnecessary dict + +ℹ Safe fix +16 16 | }, +17 17 | } +18 18 | +19 |-{**foo, **buzz, **{bar: 10}} # PIE800 + 19 |+{**foo, **buzz, bar: 10} # PIE800 +20 20 | +21 21 | {**foo, "bar": True } # OK +22 22 | + +