From 826868da5b26ba7e2e9373ed43983d561c9ec61d Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 10 Oct 2023 21:07:34 -0400 Subject: [PATCH] add autofix for `PIE804` (#7884) --- .../rules/unnecessary_dict_kwargs.rs | 79 ++++++++++++++----- ...__flake8_pie__tests__PIE804_PIE804.py.snap | 61 ++++++++++++-- 2 files changed, 117 insertions(+), 23 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs index 550892b3f379a..e26ebcb461e46 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs @@ -1,13 +1,14 @@ +use itertools::Itertools; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_python_ast::{self as ast, Constant, Expr, Keyword}; -use ruff_diagnostics::Diagnostic; -use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; use ruff_text_size::Ranged; use ruff_python_stdlib::identifiers::is_identifier; use crate::checkers::ast::Checker; +use crate::registry::AsRule; /// ## What it does /// Checks for unnecessary `dict` kwargs. @@ -40,41 +41,83 @@ use crate::checkers::ast::Checker; #[violation] pub struct UnnecessaryDictKwargs; -impl Violation for UnnecessaryDictKwargs { +impl AlwaysFixableViolation for UnnecessaryDictKwargs { #[derive_message_formats] fn message(&self) -> String { format!("Unnecessary `dict` kwargs") } + + fn fix_title(&self) -> String { + format!("Remove unnecessary kwargs") + } } /// PIE804 pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, expr: &Expr, kwargs: &[Keyword]) { for kw in kwargs { // keyword is a spread operator (indicated by None) - if kw.arg.is_none() { - if let Expr::Dict(ast::ExprDict { keys, .. }) = &kw.value { - // ensure foo(**{"bar-bar": 1}) doesn't error - if keys.iter().all(|expr| expr.as_ref().is_some_and( is_valid_kwarg_name)) || - // handle case of foo(**{**bar}) - (keys.len() == 1 && keys[0].is_none()) - { - let diagnostic = Diagnostic::new(UnnecessaryDictKwargs, expr.range()); - checker.diagnostics.push(diagnostic); - } + if kw.arg.is_some() { + continue; + } + + let Expr::Dict(ast::ExprDict { keys, values, .. }) = &kw.value else { + continue; + }; + + // Ex) `foo(**{**bar})` + if matches!(keys.as_slice(), [None]) { + let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, expr.range()); + + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + format!("**{}", checker.locator().slice(values[0].range())), + kw.range(), + ))); } + + checker.diagnostics.push(diagnostic); + continue; } + + // Ensure that every keyword is a valid keyword argument (e.g., avoid errors for cases like + // `foo(**{"bar-bar": 1})`). + let kwargs = keys + .iter() + .filter_map(|key| key.as_ref().and_then(as_kwarg)) + .collect::>(); + if kwargs.len() != keys.len() { + continue; + } + + let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, expr.range()); + + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + kwargs + .iter() + .zip(values.iter()) + .map(|(kwarg, value)| { + format!("{}={}", kwarg.value, checker.locator().slice(value.range())) + }) + .join(", "), + kw.range(), + ))); + } + + checker.diagnostics.push(diagnostic); } } -/// Return `true` if a key is a valid keyword argument name. -fn is_valid_kwarg_name(key: &Expr) -> bool { +/// Return `Some` if a key is a valid keyword argument name, or `None` otherwise. +fn as_kwarg(key: &Expr) -> Option<&ast::StringConstant> { if let Expr::Constant(ast::ExprConstant { value: Constant::Str(value), .. }) = key { - is_identifier(value) - } else { - false + if is_identifier(value) { + return Some(value); + } } + None } diff --git a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap index 7f7b0c95210fe..67f8e3831c107 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap @@ -1,15 +1,23 @@ --- source: crates/ruff_linter/src/rules/flake8_pie/mod.rs --- -PIE804.py:1:1: PIE804 Unnecessary `dict` kwargs +PIE804.py:1:1: PIE804 [*] Unnecessary `dict` kwargs | 1 | foo(**{"bar": True}) # PIE804 | ^^^^^^^^^^^^^^^^^^^^ PIE804 2 | 3 | foo(**{"r2d2": True}) # PIE804 | + = help: Remove unnecessary kwargs -PIE804.py:3:1: PIE804 Unnecessary `dict` kwargs +ℹ Fix +1 |-foo(**{"bar": True}) # PIE804 + 1 |+foo(bar=True) # PIE804 +2 2 | +3 3 | foo(**{"r2d2": True}) # PIE804 +4 4 | + +PIE804.py:3:1: PIE804 [*] Unnecessary `dict` kwargs | 1 | foo(**{"bar": True}) # PIE804 2 | @@ -18,8 +26,18 @@ PIE804.py:3:1: PIE804 Unnecessary `dict` kwargs 4 | 5 | Foo.objects.create(**{"bar": True}) # PIE804 | + = help: Remove unnecessary kwargs + +ℹ Fix +1 1 | foo(**{"bar": True}) # PIE804 +2 2 | +3 |-foo(**{"r2d2": True}) # PIE804 + 3 |+foo(r2d2=True) # PIE804 +4 4 | +5 5 | Foo.objects.create(**{"bar": True}) # PIE804 +6 6 | -PIE804.py:5:1: PIE804 Unnecessary `dict` kwargs +PIE804.py:5:1: PIE804 [*] Unnecessary `dict` kwargs | 3 | foo(**{"r2d2": True}) # PIE804 4 | @@ -28,8 +46,19 @@ PIE804.py:5:1: PIE804 Unnecessary `dict` kwargs 6 | 7 | Foo.objects.create(**{"_id": some_id}) # PIE804 | + = help: Remove unnecessary kwargs + +ℹ Fix +2 2 | +3 3 | foo(**{"r2d2": True}) # PIE804 +4 4 | +5 |-Foo.objects.create(**{"bar": True}) # PIE804 + 5 |+Foo.objects.create(bar=True) # PIE804 +6 6 | +7 7 | Foo.objects.create(**{"_id": some_id}) # PIE804 +8 8 | -PIE804.py:7:1: PIE804 Unnecessary `dict` kwargs +PIE804.py:7:1: PIE804 [*] Unnecessary `dict` kwargs | 5 | Foo.objects.create(**{"bar": True}) # PIE804 6 | @@ -38,13 +67,35 @@ PIE804.py:7:1: PIE804 Unnecessary `dict` kwargs 8 | 9 | Foo.objects.create(**{**bar}) # PIE804 | + = help: Remove unnecessary kwargs -PIE804.py:9:1: PIE804 Unnecessary `dict` kwargs +ℹ Fix +4 4 | +5 5 | Foo.objects.create(**{"bar": True}) # PIE804 +6 6 | +7 |-Foo.objects.create(**{"_id": some_id}) # PIE804 + 7 |+Foo.objects.create(_id=some_id) # PIE804 +8 8 | +9 9 | Foo.objects.create(**{**bar}) # PIE804 +10 10 | + +PIE804.py:9:1: PIE804 [*] Unnecessary `dict` kwargs | 7 | Foo.objects.create(**{"_id": some_id}) # PIE804 8 | 9 | Foo.objects.create(**{**bar}) # PIE804 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE804 | + = help: Remove unnecessary kwargs + +ℹ Fix +6 6 | +7 7 | Foo.objects.create(**{"_id": some_id}) # PIE804 +8 8 | +9 |-Foo.objects.create(**{**bar}) # PIE804 + 9 |+Foo.objects.create(**bar) # PIE804 +10 10 | +11 11 | +12 12 | foo(**{**data, "foo": "buzz"})