Skip to content

Commit

Permalink
Avoid fix for any duplicates
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Dec 12, 2023
1 parent 829b687 commit 3f2fd98
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 56 deletions.
1 change: 1 addition & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@

# 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
99 changes: 62 additions & 37 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
Expand Up @@ -3,7 +3,7 @@ use std::hash::BuildHasherDefault;
use itertools::Itertools;
use rustc_hash::FxHashSet;

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_stdlib::identifiers::is_identifier;
Expand Down Expand Up @@ -37,47 +37,31 @@ use crate::fix::edits::{remove_argument, Parentheses};
/// print(foo(bar=2)) # prints 3
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as safe, but there are some scenarios
/// that fix causes the TypeError at runtime:
///
/// ```python
/// def foo(bar):
/// return bar + 1
///
///
/// # TypeError: foo() got multiple values for keyword argument 'bar'
/// foo(bar=2, **{"bar": 3})
/// ```
///
/// ## References
/// - [Python documentation: Dictionary displays](https://docs.python.org/3/reference/expressions.html#dictionary-displays)
/// - [Python documentation: Calls](https://docs.python.org/3/reference/expressions.html#calls)
#[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) {
let mut seen = FxHashSet::with_capacity_and_hasher(
call.arguments.keywords.len(),
BuildHasherDefault::default(),
);

let mut duplicate_keywords = None;
for keyword in &call.arguments.keywords {
// keyword is a spread operator (indicated by None)
if let Some(name) = &keyword.arg {
seen.insert(name.as_str());
// keyword is a spread operator (indicated by None).
if keyword.arg.is_some() {
continue;
}

Expand All @@ -87,7 +71,7 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal

// 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())),
Expand All @@ -103,13 +87,12 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal
let kwargs = keys
.iter()
.filter_map(|key| key.as_ref().and_then(as_kwarg))
.filter(|name| seen.insert(name))
.collect::<Vec<_>>();
if kwargs.len() != keys.len() {
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(|| {
Expand All @@ -122,22 +105,64 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal
.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(", "),
keyword.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
66 changes: 47 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,12 +121,12 @@ PIE804.py:11:1: PIE804 [*] Unnecessary `dict` kwargs
13 13 | foo(**{**data, "foo": "buzz"})
14 14 | foo(**buzz)

PIE804.py:22:1: PIE804 [*] Unnecessary `dict` kwargs
PIE804.py:22:5: PIE804 [*] Unnecessary `dict` kwargs
|
20 | foo(**{f"buzz__{bar}": True})
21 | abc(**{"for": 3})
22 | foo(**{},)
| ^^^^^^^^^^ PIE804
| ^^^^ PIE804
23 |
24 | # Duplicated key names won't be fixed, to avoid syntax errors.
|
Expand All @@ -142,19 +142,47 @@ PIE804.py:22:1: PIE804 [*] Unnecessary `dict` kwargs
24 24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 25 | abc(**{'a': b}, **{'a': c}) # PIE804

PIE804.py:25:1: PIE804 [*] Unnecessary `dict` kwargs
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
| ^^^^^^^^^^ 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
22 22 | foo(**{},)
23 23 |
24 24 | # Duplicated key names won't be fixed, to avoid syntax errors.
25 |-abc(**{'a': b}, **{'a': c}) # PIE804
25 |+abc(a=b, **{'a': c}) # PIE804
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 3f2fd98

Please sign in to comment.