PIE804: Prevent keyword arguments duplication (#8450)

T-256 · web-flow · commit cb201bc4a5ab · 2023-12-12T23:19:55.000Z
diff --git 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
@@ -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<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);
@@ -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 {
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
@@ -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
 
 
