Skip to content

Commit

Permalink
PIE804: Prevent keyword arguments duplication (#8450)
Browse files Browse the repository at this point in the history
  • Loading branch information
@T-256
T-256 authored Dec 12, 2023
1 parent 6c0068e commit cb201bc
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 44 deletions.
5 changes: 4 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,8 @@
foo(**{"": True})
foo(**{f"buzz__{bar}": True})
abc(**{"for": 3})

foo(**{},)

# Duplicated key names won't be fixed, to avoid syntax errors.
abc(**{'a': b}, **{'a': c}) # PIE804
abc(a=1, **{'a': c}, **{'b': c}) # PIE804
95 changes: 71 additions & 24 deletions crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use std::hash::BuildHasherDefault;

use itertools::Itertools;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_python_ast::{self as ast, Expr};
use rustc_hash::FxHashSet;

use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_text_size::Ranged;

use ruff_python_ast::{self as ast, Expr};
use ruff_python_stdlib::identifiers::is_identifier;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::fix::edits::{remove_argument, Parentheses};
Expand Down Expand Up @@ -41,36 +43,39 @@ use crate::fix::edits::{remove_argument, Parentheses};
#[violation]
pub struct UnnecessaryDictKwargs;

impl AlwaysFixableViolation for UnnecessaryDictKwargs {
impl Violation for UnnecessaryDictKwargs {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Unnecessary `dict` kwargs")
}

fn fix_title(&self) -> String {
format!("Remove unnecessary kwargs")
fn fix_title(&self) -> Option<String> {
Some(format!("Remove unnecessary kwargs"))
}
}

/// PIE804
pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCall) {
for kw in &call.arguments.keywords {
// keyword is a spread operator (indicated by None)
if kw.arg.is_some() {
let mut duplicate_keywords = None;
for keyword in &call.arguments.keywords {
// keyword is a spread operator (indicated by None).
if keyword.arg.is_some() {
continue;
}

let Expr::Dict(ast::ExprDict { keys, values, .. }) = &kw.value else {
let Expr::Dict(ast::ExprDict { keys, values, .. }) = &keyword.value else {
continue;
};

// Ex) `foo(**{**bar})`
if matches!(keys.as_slice(), [None]) {
let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, call.range());
let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, keyword.range());

diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
format!("**{}", checker.locator().slice(values[0].range())),
kw.range(),
keyword.range(),
)));

checker.diagnostics.push(diagnostic);
Expand All @@ -87,35 +92,77 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal
continue;
}

let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, call.range());
let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, keyword.range());

if values.is_empty() {
diagnostic.try_set_fix(|| {
remove_argument(
kw,
keyword,
&call.arguments,
Parentheses::Preserve,
checker.locator().contents(),
)
.map(Fix::safe_edit)
});
} else {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
kwargs
// Compute the set of duplicate keywords (lazily).
if duplicate_keywords.is_none() {
duplicate_keywords = Some(duplicates(call));
}

// Avoid fixing if doing so could introduce a duplicate keyword argument.
if let Some(duplicate_keywords) = duplicate_keywords.as_ref() {
if kwargs
.iter()
.zip(values.iter())
.map(|(kwarg, value)| {
format!("{}={}", kwarg, checker.locator().slice(value.range()))
})
.join(", "),
kw.range(),
)));
.all(|kwarg| !duplicate_keywords.contains(kwarg))
{
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
kwargs
.iter()
.zip(values.iter())
.map(|(kwarg, value)| {
format!("{}={}", kwarg, checker.locator().slice(value.range()))
})
.join(", "),
keyword.range(),
)));
}
}
}

checker.diagnostics.push(diagnostic);
}
}

/// Determine the set of keywords that appear in multiple positions (either directly, as in
/// `func(x=1)`, or indirectly, as in `func(**{"x": 1})`).
fn duplicates(call: &ast::ExprCall) -> FxHashSet<&str> {
let mut seen = FxHashSet::with_capacity_and_hasher(
call.arguments.keywords.len(),
BuildHasherDefault::default(),
);
let mut duplicates = FxHashSet::with_capacity_and_hasher(
call.arguments.keywords.len(),
BuildHasherDefault::default(),
);
for keyword in &call.arguments.keywords {
if let Some(name) = &keyword.arg {
if !seen.insert(name.as_str()) {
duplicates.insert(name.as_str());
}
} else if let Expr::Dict(ast::ExprDict { keys, .. }) = &keyword.value {
for key in keys {
if let Some(name) = key.as_ref().and_then(as_kwarg) {
if !seen.insert(name) {
duplicates.insert(name);
}
}
}
}
}
duplicates
}

/// Return `Some` if a key is a valid keyword argument name, or `None` otherwise.
fn as_kwarg(key: &Expr) -> Option<&str> {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = key {
Expand Down
86 changes: 67 additions & 19 deletions ...c/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
---
source: crates/ruff_linter/src/rules/flake8_pie/mod.rs
---
PIE804.py:1:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:1:5: PIE804 [*] Unnecessary `dict` kwargs
|
1 | foo(**{"bar": True}) # PIE804
| ^^^^^^^^^^^^^^^^^^^^ PIE804
| ^^^^^^^^^^^^^^^ PIE804
2 |
3 | foo(**{"r2d2": True}) # PIE804
|
Expand All @@ -17,12 +17,12 @@ PIE804.py:1:1: PIE804 [*] Unnecessary `dict` kwargs
3 3 | foo(**{"r2d2": True}) # PIE804
4 4 |

PIE804.py:3:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:3:5: PIE804 [*] Unnecessary `dict` kwargs
|
1 | foo(**{"bar": True}) # PIE804
2 |
3 | foo(**{"r2d2": True}) # PIE804
| ^^^^^^^^^^^^^^^^^^^^^ PIE804
| ^^^^^^^^^^^^^^^^ PIE804
4 |
5 | Foo.objects.create(**{"bar": True}) # PIE804
|
Expand All @@ -37,12 +37,12 @@ PIE804.py:3:1: PIE804 [*] Unnecessary `dict` kwargs
5 5 | Foo.objects.create(**{"bar": True}) # PIE804
6 6 |

PIE804.py:5:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:5:20: PIE804 [*] Unnecessary `dict` kwargs
|
3 | foo(**{"r2d2": True}) # PIE804
4 |
5 | Foo.objects.create(**{"bar": True}) # PIE804
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE804
| ^^^^^^^^^^^^^^^ PIE804
6 |
7 | Foo.objects.create(**{"_id": some_id}) # PIE804
|
Expand All @@ -58,12 +58,12 @@ PIE804.py:5:1: PIE804 [*] Unnecessary `dict` kwargs
7 7 | Foo.objects.create(**{"_id": some_id}) # PIE804
8 8 |

PIE804.py:7:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:7:20: PIE804 [*] Unnecessary `dict` kwargs
|
5 | Foo.objects.create(**{"bar": True}) # PIE804
6 |
7 | Foo.objects.create(**{"_id": some_id}) # PIE804
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE804
| ^^^^^^^^^^^^^^^^^^ PIE804
8 |
9 | Foo.objects.create(**{**bar}) # PIE804
|
Expand All @@ -79,12 +79,12 @@ PIE804.py:7:1: PIE804 [*] Unnecessary `dict` kwargs
9 9 | Foo.objects.create(**{**bar}) # PIE804
10 10 |

PIE804.py:9:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:9:20: PIE804 [*] Unnecessary `dict` kwargs
|
7 | Foo.objects.create(**{"_id": some_id}) # PIE804
8 |
9 | Foo.objects.create(**{**bar}) # PIE804
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE804
| ^^^^^^^^^ PIE804
10 |
11 | foo(**{})
|
Expand All @@ -100,12 +100,12 @@ PIE804.py:9:1: PIE804 [*] Unnecessary `dict` kwargs
11 11 | foo(**{})
12 12 |

PIE804.py:11:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:11:5: PIE804 [*] Unnecessary `dict` kwargs
|
9 | Foo.objects.create(**{**bar}) # PIE804
10 |
11 | foo(**{})
| ^^^^^^^^^ PIE804
| ^^^^ PIE804
12 |
13 | foo(**{**data, "foo": "buzz"})
|
Expand All @@ -121,20 +121,68 @@ PIE804.py:11:1: PIE804 [*] Unnecessary `dict` kwargs
13 13 | foo(**{**data, "foo": "buzz"})
14 14 | foo(**buzz)

PIE804.py:23:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:22:5: PIE804 [*] Unnecessary `dict` kwargs
|
20 | foo(**{f"buzz__{bar}": True})
21 | abc(**{"for": 3})
22 |
23 | foo(**{},)
| ^^^^^^^^^^ PIE804
22 | foo(**{},)
| ^^^^ PIE804
23 |
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
|
= help: Remove unnecessary kwargs

ℹ Safe fix
19 19 | foo(**{"": True})
20 20 | foo(**{f"buzz__{bar}": True})
21 21 | abc(**{"for": 3})
22 22 |
23 |-foo(**{},)
23 |+foo()
22 |-foo(**{},)
22 |+foo()
23 23 |
24 24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 25 | abc(**{'a': b}, **{'a': c}) # PIE804

PIE804.py:25:5: PIE804 Unnecessary `dict` kwargs
|
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 | abc(**{'a': b}, **{'a': c}) # PIE804
| ^^^^^^^^^^ PIE804
26 | abc(a=1, **{'a': c}, **{'b': c}) # PIE804
|
= help: Remove unnecessary kwargs

PIE804.py:25:17: PIE804 Unnecessary `dict` kwargs
|
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 | abc(**{'a': b}, **{'a': c}) # PIE804
| ^^^^^^^^^^ PIE804
26 | abc(a=1, **{'a': c}, **{'b': c}) # PIE804
|
= help: Remove unnecessary kwargs

PIE804.py:26:10: PIE804 Unnecessary `dict` kwargs
|
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 | abc(**{'a': b}, **{'a': c}) # PIE804
26 | abc(a=1, **{'a': c}, **{'b': c}) # PIE804
| ^^^^^^^^^^ PIE804
|
= help: Remove unnecessary kwargs

PIE804.py:26:22: PIE804 [*] Unnecessary `dict` kwargs
|
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 | abc(**{'a': b}, **{'a': c}) # PIE804
26 | abc(a=1, **{'a': c}, **{'b': c}) # PIE804
| ^^^^^^^^^^ PIE804
|
= help: Remove unnecessary kwargs

ℹ Safe fix
23 23 |
24 24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 25 | abc(**{'a': b}, **{'a': c}) # PIE804
26 |-abc(a=1, **{'a': c}, **{'b': c}) # PIE804
26 |+abc(a=1, **{'a': c}, b=c) # PIE804


0 comments on commit cb201bc

Please sign in to comment.