Skip to content

Commit

Permalink
Add autofix for PIE800 (#8668)
Browse files Browse the repository at this point in the history
## Summary

This adds an autofix for PIE800 (unnecessary spread) -- whenever we see
a `**{...}` inside another dictionary literal, just delete the `**{` and
`}` to inline the key-value pairs. So `{"a": "b", **{"c": "d"}}` becomes
just `{"a": "b", "c": "d"}`.

I have enabled this just for preview mode.

## Test Plan

Updated the preview snapshot test.
  • Loading branch information
alanhdu authored Nov 15, 2023
1 parent 0e2ece5 commit 2083352
Show file tree
Hide file tree
Showing 6 changed files with 257 additions and 31 deletions.
12 changes: 12 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE800.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down
10 changes: 2 additions & 8 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -936,21 +936,15 @@ 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,
]) {
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: _ }) => {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_pie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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<String> {
Some(format!("Remove unnecessary dict"))
}
}

/// PIE800
pub(crate) fn unnecessary_spread(checker: &mut Checker, keys: &[Option<Expr>], 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<Fix> {
// 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(),
)))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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


Original file line number Diff line number Diff line change
@@ -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 |


0 comments on commit 2083352

Please sign in to comment.