From cb201bc4a5ab1a6e312b375f820d48bdcc4bf9a3 Mon Sep 17 00:00:00 2001 From: T-256 <132141463+T-256@users.noreply.github.com> Date: Wed, 13 Dec 2023 02:49:55 +0330 Subject: [PATCH] `PIE804`: Prevent keyword arguments duplication (#8450) --- .../test/fixtures/flake8_pie/PIE804.py | 5 +- .../rules/unnecessary_dict_kwargs.rs | 95 ++++++++++++++----- ...__flake8_pie__tests__PIE804_PIE804.py.snap | 86 +++++++++++++---- 3 files changed, 142 insertions(+), 44 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py index 03d88e0ef8e22..a5b3674422e1a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py @@ -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 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 ab4c7ceb9ddcc..5f0bf0abb48d0 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,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}; @@ -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 { + 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); @@ -87,12 +92,12 @@ 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(), @@ -100,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(", "), - 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 { 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 0f608fbedb040..15d993a0befee 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,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 | @@ -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 | @@ -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 | @@ -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 | @@ -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(**{}) | @@ -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"}) | @@ -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