From 1acd749bff62670e9496043c626a3d3c5505e2a6 Mon Sep 17 00:00:00 2001 From: Alan Du Date: Mon, 13 Nov 2023 23:05:41 -0500 Subject: [PATCH 1/4] Add autofix for PIE800 Whenever we see a **{...} inside another dictionary literal, just delete the `**{` and `}` to inline the key-value pairs --- .../ruff_linter/src/rules/flake8_pie/mod.rs | 1 + .../flake8_pie/rules/unnecessary_spread.rs | 28 ++++++- ...__flake8_pie__tests__PIE800_PIE800.py.snap | 4 + ...pie__tests__preview__PIE800_PIE800.py.snap | 82 +++++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__preview__PIE800_PIE800.py.snap 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..f27934ec4f712 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,8 @@ use ruff_python_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_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -32,10 +31,16 @@ 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 @@ -45,7 +50,22 @@ pub(crate) fn unnecessary_spread(checker: &mut Checker, keys: &[Option], v // 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()); + let mut diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); + if checker.settings.preview.is_enabled() { + let range = value.range(); + // unwrap -- `item.0 == None` iff this is a spread operator + // which means there *must* be a `**` here + let doublestar = checker.locator().up_to(range.start()).rfind("**").unwrap(); + diagnostic.set_fix(Fix::safe_edits( + // delete the `**{` + Edit::deletion( + TextSize::from(doublestar as u32), + range.start() + TextSize::from(1), + ), + // delete the `}` + [Edit::deletion(range.end() - TextSize::from(1), range.end())], + )); + } checker.diagnostics.push(diagnostic); } } 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..7825ad433c6e9 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 @@ -8,6 +8,7 @@ PIE800.py:1:14: PIE800 Unnecessary spread `**` 2 | 3 | foo({**foo, **{"bar": True}}) # PIE800 | + = help: Remove unnecessary dict PIE800.py:3:15: PIE800 Unnecessary spread `**` | @@ -18,6 +19,7 @@ PIE800.py:3:15: PIE800 Unnecessary spread `**` 4 | 5 | {**foo, **{"bar": 10}} # PIE800 | + = help: Remove unnecessary dict PIE800.py:5:11: PIE800 Unnecessary spread `**` | @@ -28,6 +30,7 @@ PIE800.py:5:11: PIE800 Unnecessary spread `**` 6 | 7 | {**foo, **buzz, **{bar: 10}} # PIE800 | + = help: Remove unnecessary dict PIE800.py:7:19: PIE800 Unnecessary spread `**` | @@ -38,5 +41,6 @@ PIE800.py:7:19: PIE800 Unnecessary spread `**` 8 | 9 | {**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..0fe76288763ba --- /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,82 @@ +--- +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 | foo({**foo, **{"bar": True}}) # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +1 |-{"foo": 1, **{"bar": 1}} # PIE800 + 1 |+{"foo": 1, "bar": 1} # PIE800 +2 2 | +3 3 | foo({**foo, **{"bar": True}}) # PIE800 +4 4 | + +PIE800.py:3:15: PIE800 [*] Unnecessary spread `**` + | +1 | {"foo": 1, **{"bar": 1}} # PIE800 +2 | +3 | foo({**foo, **{"bar": True}}) # PIE800 + | ^^^^^^^^^^^^^ PIE800 +4 | +5 | {**foo, **{"bar": 10}} # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +1 1 | {"foo": 1, **{"bar": 1}} # PIE800 +2 2 | +3 |-foo({**foo, **{"bar": True}}) # PIE800 + 3 |+foo({**foo, "bar": True}) # PIE800 +4 4 | +5 5 | {**foo, **{"bar": 10}} # PIE800 +6 6 | + +PIE800.py:5:11: PIE800 [*] Unnecessary spread `**` + | +3 | foo({**foo, **{"bar": True}}) # PIE800 +4 | +5 | {**foo, **{"bar": 10}} # PIE800 + | ^^^^^^^^^^^ PIE800 +6 | +7 | {**foo, **buzz, **{bar: 10}} # PIE800 + | + = help: Remove unnecessary dict + +ℹ Safe fix +2 2 | +3 3 | foo({**foo, **{"bar": True}}) # PIE800 +4 4 | +5 |-{**foo, **{"bar": 10}} # PIE800 + 5 |+{**foo, "bar": 10} # PIE800 +6 6 | +7 7 | {**foo, **buzz, **{bar: 10}} # PIE800 +8 8 | + +PIE800.py:7:19: PIE800 [*] Unnecessary spread `**` + | +5 | {**foo, **{"bar": 10}} # PIE800 +6 | +7 | {**foo, **buzz, **{bar: 10}} # PIE800 + | ^^^^^^^^^ PIE800 +8 | +9 | {**foo, "bar": True } # OK + | + = help: Remove unnecessary dict + +ℹ Safe fix +4 4 | +5 5 | {**foo, **{"bar": 10}} # PIE800 +6 6 | +7 |-{**foo, **buzz, **{bar: 10}} # PIE800 + 7 |+{**foo, **buzz, bar: 10} # PIE800 +8 8 | +9 9 | {**foo, "bar": True } # OK +10 10 | + + From c003bbfd685e671c5ed195e941e90b63eb142a2d Mon Sep 17 00:00:00 2001 From: Alan Du Date: Mon, 13 Nov 2023 23:09:56 -0500 Subject: [PATCH 2/4] Use BackwardsTokenizer --- .../test/fixtures/flake8_pie/PIE800.py | 12 ++ .../flake8_pie/rules/unnecessary_spread.rs | 63 ++++++++--- ...__flake8_pie__tests__PIE800_PIE800.py.snap | 56 ++++++--- ...pie__tests__preview__PIE800_PIE800.py.snap | 107 +++++++++++++----- 4 files changed, 181 insertions(+), 57 deletions(-) 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/rules/flake8_pie/rules/unnecessary_spread.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs index f27934ec4f712..d3d47988d4f17 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 @@ -2,6 +2,7 @@ use ruff_python_ast::Expr; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_trivia::{BackwardsTokenizer, SimpleTokenKind}; use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -49,22 +50,56 @@ pub(crate) fn unnecessary_spread(checker: &mut Checker, keys: &[Option], v 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 { + if let Expr::Dict(dict) = value { let mut diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); if checker.settings.preview.is_enabled() { - let range = value.range(); - // unwrap -- `item.0 == None` iff this is a spread operator - // which means there *must* be a `**` here - let doublestar = checker.locator().up_to(range.start()).rfind("**").unwrap(); - diagnostic.set_fix(Fix::safe_edits( - // delete the `**{` - Edit::deletion( - TextSize::from(doublestar as u32), - range.start() + TextSize::from(1), - ), - // delete the `}` - [Edit::deletion(range.end() - TextSize::from(1), range.end())], - )); + // Delete the `**{` + let tokenizer = BackwardsTokenizer::up_to( + dict.range.start(), + checker.locator().contents(), + &[], + ); + let mut start = None; + for tok in tokenizer { + if let SimpleTokenKind::DoubleStar = tok.kind() { + start = Some(tok.range.start()); + break; + } + } + // unwrap is ok, b/c item.0 can't be None without a DoubleStar + let first = + Edit::deletion(start.unwrap(), dict.range.start() + TextSize::from(1)); + + // Delete the `}` (and possibly a trailing comma) but preserve comments + let mut edits = Vec::with_capacity(1); + let mut end = dict.range.end(); + + let tokenizer = BackwardsTokenizer::up_to( + dict.range.end() - TextSize::from(1), + checker.locator().contents(), + &[], + ); + for tok in tokenizer { + match tok.kind() { + SimpleTokenKind::Comment => { + if tok.range.end() != end { + edits.push(Edit::deletion(tok.range.end(), end)); + } + end = tok.range.start(); + } + SimpleTokenKind::Comma + | SimpleTokenKind::Whitespace + | SimpleTokenKind::Newline + | SimpleTokenKind::Continuation => {} + _ => { + if tok.range.end() != end { + edits.push(Edit::deletion(tok.range.end(), end)); + } + break; + } + } + } + diagnostic.set_fix(Fix::safe_edits(first, edits)); } checker.diagnostics.push(diagnostic); } 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 7825ad433c6e9..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,41 +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 index 0fe76288763ba..d6fea2db4e700 100644 --- 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 @@ -6,7 +6,7 @@ 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 @@ -14,69 +14,120 @@ PIE800.py:1:14: PIE800 [*] Unnecessary spread `**` 1 |-{"foo": 1, **{"bar": 1}} # PIE800 1 |+{"foo": 1, "bar": 1} # PIE800 2 2 | -3 3 | foo({**foo, **{"bar": True}}) # PIE800 +3 3 | {**{"bar": 10}, "a": "b"} # PIE800 4 4 | -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 ℹ Safe fix 1 1 | {"foo": 1, **{"bar": 1}} # PIE800 2 2 | -3 |-foo({**foo, **{"bar": True}}) # PIE800 - 3 |+foo({**foo, "bar": True}) # PIE800 +3 |-{**{"bar": 10}, "a": "b"} # PIE800 + 3 |+{"bar": 10, "a": "b"} # PIE800 4 4 | -5 5 | {**foo, **{"bar": 10}} # PIE800 +5 5 | foo({**foo, **{"bar": True}}) # PIE800 6 6 | -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 ℹ Safe fix 2 2 | -3 3 | foo({**foo, **{"bar": True}}) # PIE800 +3 3 | {**{"bar": 10}, "a": "b"} # PIE800 4 4 | -5 |-{**foo, **{"bar": 10}} # PIE800 - 5 |+{**foo, "bar": 10} # PIE800 +5 |-foo({**foo, **{"bar": True}}) # PIE800 + 5 |+foo({**foo, "bar": True}) # PIE800 6 6 | -7 7 | {**foo, **buzz, **{bar: 10}} # PIE800 +7 7 | {**foo, **{"bar": 10}} # PIE800 8 8 | -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 ℹ Safe fix 4 4 | -5 5 | {**foo, **{"bar": 10}} # PIE800 +5 5 | foo({**foo, **{"bar": True}}) # PIE800 6 6 | -7 |-{**foo, **buzz, **{bar: 10}} # PIE800 - 7 |+{**foo, **buzz, bar: 10} # PIE800 +7 |-{**foo, **{"bar": 10}} # PIE800 + 7 |+{**foo, "bar": 10} # PIE800 8 8 | -9 9 | {**foo, "bar": True } # OK -10 10 | +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 14 | "bar": 10, # the +15 |- # comments +16 |- }, + 15 |+ # comments, +17 16 | } +18 17 | +19 18 | {**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 | From bb30495d556cc8bdc914dd6cf5fa95e9fb51ac8a Mon Sep 17 00:00:00 2001 From: Alan Du Date: Tue, 14 Nov 2023 22:59:57 -0500 Subject: [PATCH 3/4] Use SimpleTokenizer instead of BackwardTokenizer --- .../src/checkers/ast/analyze/expression.rs | 10 +- .../flake8_pie/rules/unnecessary_spread.rs | 98 +++++++++---------- ...pie__tests__preview__PIE800_PIE800.py.snap | 13 +-- 3 files changed, 56 insertions(+), 65 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 106cbecd47909..c43e260440add 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/rules/unnecessary_spread.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_spread.rs index d3d47988d4f17..8e844cd04ded3 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,8 +1,8 @@ -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_trivia::{BackwardsTokenizer, SimpleTokenKind}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -45,64 +45,60 @@ impl Violation for UnnecessarySpread { } /// 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) { + let mut prev_end = dict.range.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(dict) = value { + if let Expr::Dict(inner) = value { let mut diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); if checker.settings.preview.is_enabled() { - // Delete the `**{` - let tokenizer = BackwardsTokenizer::up_to( - dict.range.start(), - checker.locator().contents(), - &[], - ); - let mut start = None; - for tok in tokenizer { - if let SimpleTokenKind::DoubleStar = tok.kind() { - start = Some(tok.range.start()); - break; - } - } - // unwrap is ok, b/c item.0 can't be None without a DoubleStar - let first = - Edit::deletion(start.unwrap(), dict.range.start() + TextSize::from(1)); + diagnostic.set_fix(unnecessary_spread_fix(checker, inner, prev_end)); + } + checker.diagnostics.push(diagnostic); + } + } + prev_end = item.1.end(); + } +} - // Delete the `}` (and possibly a trailing comma) but preserve comments - let mut edits = Vec::with_capacity(1); - let mut end = dict.range.end(); +fn unnecessary_spread_fix(checker: &mut Checker, inner: &ast::ExprDict, prev_end: TextSize) -> Fix { + let tokenizer = SimpleTokenizer::starts_at(prev_end, checker.locator().contents()); + let mut start = None; + for tok in tokenizer { + if let SimpleTokenKind::DoubleStar = tok.kind() { + start = Some(tok.range.start()); + break; + } + } + // unwrap is ok, b/c item.0 can't be None without a DoubleStar + let doublestar = start.unwrap(); - let tokenizer = BackwardsTokenizer::up_to( - dict.range.end() - TextSize::from(1), - checker.locator().contents(), - &[], - ); - for tok in tokenizer { - match tok.kind() { - SimpleTokenKind::Comment => { - if tok.range.end() != end { - edits.push(Edit::deletion(tok.range.end(), end)); - } - end = tok.range.start(); - } - SimpleTokenKind::Comma - | SimpleTokenKind::Whitespace - | SimpleTokenKind::Newline - | SimpleTokenKind::Continuation => {} - _ => { - if tok.range.end() != end { - edits.push(Edit::deletion(tok.range.end(), end)); - } - break; - } - } - } - diagnostic.set_fix(Fix::safe_edits(first, edits)); + if let Some(last) = inner.values.last() { + let tokenizer = + SimpleTokenizer::starts_at(last.range().end(), checker.locator().contents()); + let mut edits = vec![]; + for tok in tokenizer.skip_trivia() { + match tok.kind() { + SimpleTokenKind::Comma => { + edits.push(Edit::range_deletion(tok.range())); } - checker.diagnostics.push(diagnostic); + SimpleTokenKind::RBrace => { + edits.push(Edit::range_deletion(tok.range)); + break; + } + _ => {} } } + Fix::safe_edits( + // Delete the first `**{` + Edit::deletion(doublestar, inner.start() + TextSize::from(1)), + // Delete the trailing `}` + edits, + ) + } else { + // Can just delete the entire thing + Fix::safe_edit(Edit::deletion(doublestar, inner.end())) } } 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 index d6fea2db4e700..217e4c71f3a21 100644 --- 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 @@ -101,13 +101,14 @@ PIE800.py:12:7: PIE800 [*] Unnecessary spread `**` 12 |- **{ 12 |+ 13 13 | # all -14 14 | "bar": 10, # the -15 |- # comments +14 |- "bar": 10, # the + 14 |+ "bar": 10 # the +15 15 | # comments 16 |- }, - 15 |+ # comments, -17 16 | } -18 17 | -19 18 | {**foo, **buzz, **{bar: 10}} # PIE800 + 16 |+ , +17 17 | } +18 18 | +19 19 | {**foo, **buzz, **{bar: 10}} # PIE800 PIE800.py:19:19: PIE800 [*] Unnecessary spread `**` | From 0a728b19074ae09522cb622f4abb71067e5b8c34 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 15 Nov 2023 13:05:13 -0500 Subject: [PATCH 4/4] Add some docs, tweaks --- .../flake8_pie/rules/unnecessary_spread.rs | 50 ++++++++++--------- 1 file changed, 27 insertions(+), 23 deletions(-) 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 8e844cd04ded3..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 @@ -3,6 +3,7 @@ use ruff_python_ast::{self as ast, Expr}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -46,7 +47,8 @@ impl Violation for UnnecessarySpread { /// PIE800 pub(crate) fn unnecessary_spread(checker: &mut Checker, dict: &ast::ExprDict) { - let mut prev_end = dict.range.start() + TextSize::from(1); + // 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 `**` @@ -54,7 +56,9 @@ pub(crate) fn unnecessary_spread(checker: &mut Checker, dict: &ast::ExprDict) { if let Expr::Dict(inner) = value { let mut diagnostic = Diagnostic::new(UnnecessarySpread, value.range()); if checker.settings.preview.is_enabled() { - diagnostic.set_fix(unnecessary_spread_fix(checker, inner, prev_end)); + if let Some(fix) = unnecessary_spread_fix(inner, prev_end, checker.locator()) { + diagnostic.set_fix(fix); + } } checker.diagnostics.push(diagnostic); } @@ -63,42 +67,42 @@ pub(crate) fn unnecessary_spread(checker: &mut Checker, dict: &ast::ExprDict) { } } -fn unnecessary_spread_fix(checker: &mut Checker, inner: &ast::ExprDict, prev_end: TextSize) -> Fix { - let tokenizer = SimpleTokenizer::starts_at(prev_end, checker.locator().contents()); - let mut start = None; - for tok in tokenizer { - if let SimpleTokenKind::DoubleStar = tok.kind() { - start = Some(tok.range.start()); - break; - } - } - // unwrap is ok, b/c item.0 can't be None without a DoubleStar - let doublestar = start.unwrap(); +/// 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) = inner.values.last() { - let tokenizer = - SimpleTokenizer::starts_at(last.range().end(), checker.locator().contents()); + if let Some(last) = dict.values.last() { + // Ex) `**{a: 1, b: 2}` let mut edits = vec![]; - for tok in tokenizer.skip_trivia() { + 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)); + edits.push(Edit::range_deletion(tok.range())); break; } _ => {} } } - Fix::safe_edits( + Some(Fix::safe_edits( // Delete the first `**{` - Edit::deletion(doublestar, inner.start() + TextSize::from(1)), + Edit::deletion(doublestar.start(), dict.start() + TextSize::from(1)), // Delete the trailing `}` edits, - ) + )) } else { - // Can just delete the entire thing - Fix::safe_edit(Edit::deletion(doublestar, inner.end())) + // Ex) `**{}` + Some(Fix::safe_edit(Edit::deletion( + doublestar.start(), + dict.end(), + ))) } }