Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add autofix for PIE804 #7884

Merged
merged 3 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restructured it such that, when we validate the keyword arguments, we also return the valid keyword argument name. That lets us avoid a bunch of unwraps below. In other words: when we validate, we return the validated data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo. Love it! Thanks!

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
}
Original file line number Diff line number Diff line change
@@ -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 |
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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"})