[ruff] Handle extra arguments to deque (RUF037) (#18614)

ntBre · web-flow · commit 96171f41c23d · 2025-06-12T09:07:17.000-04:00
## Summary Fixes #18612 by: - Bailing out without a fix in the case of `*args`, which I don't think we can fix reliably - Using an `Edit::deletion` from `remove_argument` instead of an `Edit::range_replacement` in the presence of unrecognized keyword arguments I thought we could always switch to the `Edit::deletion` approach initially, but it caused problems when `maxlen` was passed positionally, which we didn't have any existing tests for. The replacement fix can easily delete comments, so I also marked the fix unsafe in these cases and updated the docs accordingly. ## Test Plan New test cases derived from the issue. ## Stabilization These are pretty significant changes, much like those to PYI059 in #18611 (and based a bit on the implementation there!), so I think it probably makes sense to un-stabilize this for the 0.12 release, but I'm open to other thoughts there.
diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF037.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF037.py
@@ -59,3 +59,35 @@ def f():
 
 def f():
     x = 0 or(deque)([])
+
+
+# regression tests for https://github.com/astral-sh/ruff/issues/18612
+def f():
+    deque([], *[10])  # RUF037 but no fix
+    deque([], **{"maxlen": 10})  # RUF037
+    deque([], foo=1)  # RUF037
+
+
+# Somewhat related to the issue, both okay because we can't generally look
+# inside *args or **kwargs
+def f():
+    deque(*([], 10))  # Ok
+    deque(**{"iterable": [], "maxlen": 10})  # Ok
+
+# The fix was actually always unsafe in the presence of comments. all of these
+# are deleted
+def f():
+    deque(  # a comment in deque, deleted
+        [  # a comment _in_ the list, deleted
+        ],  # a comment after the list, deleted
+        maxlen=10,  # a comment on maxlen, deleted
+        )  # only this is preserved
+
+
+# `maxlen` can also be passed positionally
+def f():
+    deque([], 10)
+
+
+def f():
+    deque([], iterable=[])
diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_literal_within_deque_call.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_literal_within_deque_call.rs
@@ -1,10 +1,12 @@
+use ruff_diagnostics::{Applicability, Edit};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::parenthesize::parenthesized_range;
 use ruff_python_ast::{self as ast, Expr};
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
-use crate::{AlwaysFixableViolation, Edit, Fix};
+use crate::fix::edits::{Parentheses, remove_argument};
+use crate::{Fix, FixAvailability, Violation};
 
 /// ## What it does
 /// Checks for usages of `collections.deque` that have an empty iterable as the first argument.
@@ -30,26 +32,37 @@ use crate::{AlwaysFixableViolation, Edit, Fix};
 /// queue = deque(maxlen=10)
 /// ```
 ///
+/// ## Fix safety
+///
+/// The fix is marked as unsafe whenever it would delete comments present in the `deque` call or if
+/// there are unrecognized arguments other than `iterable` and `maxlen`.
+///
+/// ## Fix availability
+///
+/// This rule's fix is unavailable if any starred arguments are present after the initial iterable.
+///
 /// ## References
 /// - [Python documentation: `collections.deque`](https://docs.python.org/3/library/collections.html#collections.deque)
 #[derive(ViolationMetadata)]
 pub(crate) struct UnnecessaryEmptyIterableWithinDequeCall {
     has_maxlen: bool,
 }
 
-impl AlwaysFixableViolation for UnnecessaryEmptyIterableWithinDequeCall {
+impl Violation for UnnecessaryEmptyIterableWithinDequeCall {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
     #[derive_message_formats]
     fn message(&self) -> String {
         "Unnecessary empty iterable within a deque call".to_string()
     }
 
-    fn fix_title(&self) -> String {
+    fn fix_title(&self) -> Option<String> {
         let title = if self.has_maxlen {
             "Replace with `deque(maxlen=...)`"
         } else {
             "Replace with `deque()`"
         };
-        title.to_string()
+        Some(title.to_string())
     }
 }
 
@@ -66,13 +79,13 @@ pub(crate) fn unnecessary_literal_within_deque_call(checker: &Checker, deque: &a
         return;
     }
 
-    let Some(iterable) = arguments.find_argument_value("iterable", 0) else {
+    let Some(iterable) = arguments.find_argument("iterable", 0) else {
         return;
     };
 
     let maxlen = arguments.find_argument_value("maxlen", 1);
 
-    let is_empty_literal = match iterable {
+    let is_empty_literal = match iterable.value() {
         Expr::Dict(dict) => dict.is_empty(),
         Expr::List(list) => list.is_empty(),
         Expr::Tuple(tuple) => tuple.is_empty(),
@@ -100,29 +113,61 @@ pub(crate) fn unnecessary_literal_within_deque_call(checker: &Checker, deque: &a
         deque.range,
     );
 
-    diagnostic.set_fix(fix_unnecessary_literal_in_deque(checker, deque, maxlen));
+    // Return without a fix in the presence of a starred argument because we can't accurately
+    // generate the fix. If all of the arguments are unpacked (e.g. `deque(*([], 10))`), we will
+    // have already returned after the first `find_argument_value` call.
+    if deque.arguments.args.iter().any(Expr::is_starred_expr) {
+        return;
+    }
+
+    diagnostic.try_set_fix(|| fix_unnecessary_literal_in_deque(checker, iterable, deque, maxlen));
 }
 
 fn fix_unnecessary_literal_in_deque(
     checker: &Checker,
+    iterable: ast::ArgOrKeyword,
     deque: &ast::ExprCall,
     maxlen: Option<&Expr>,
-) -> Fix {
-    let deque_name = checker.locator().slice(
-        parenthesized_range(
-            deque.func.as_ref().into(),
-            deque.into(),
+) -> anyhow::Result<Fix> {
+    // if `maxlen` is `Some`, we know there were exactly two arguments, and we can replace the whole
+    // call. otherwise, we only delete the `iterable` argument and leave the others untouched.
+    let edit = if let Some(maxlen) = maxlen {
+        let deque_name = checker.locator().slice(
+            parenthesized_range(
+                deque.func.as_ref().into(),
+                deque.into(),
+                checker.comment_ranges(),
+                checker.source(),
+            )
+            .unwrap_or(deque.func.range()),
+        );
+        let len_str = checker.locator().slice(maxlen);
+        let deque_str = format!("{deque_name}(maxlen={len_str})");
+        Edit::range_replacement(deque_str, deque.range)
+    } else {
+        let range = parenthesized_range(
+            iterable.value().into(),
+            (&deque.arguments).into(),
             checker.comment_ranges(),
             checker.source(),
         )
-        .unwrap_or(deque.func.range()),
-    );
-    let deque_str = match maxlen {
-        Some(maxlen) => {
-            let len_str = checker.locator().slice(maxlen);
-            format!("{deque_name}(maxlen={len_str})")
-        }
-        None => format!("{deque_name}()"),
+        .unwrap_or(iterable.range());
+        remove_argument(
+            &range,
+            &deque.arguments,
+            Parentheses::Preserve,
+            checker.source(),
+        )?
+    };
+    let has_comments = checker.comment_ranges().intersects(edit.range());
+    // we've already checked maxlen.is_some() && args != 2 above, so this is the only problematic
+    // case left
+    let unknown_arguments = maxlen.is_none() && deque.arguments.len() != 1;
+    let applicability = if has_comments || unknown_arguments {
+        Applicability::Unsafe
+    } else {
+        Applicability::Safe
     };
-    Fix::safe_edit(Edit::range_replacement(deque_str, deque.range))
+
+    Ok(Fix::applicable_edit(edit, applicability))
 }
diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF037_RUF037.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF037_RUF037.py.snap
@@ -141,3 +141,117 @@ RUF037.py:61:13: RUF037 [*] Unnecessary empty iterable within a deque call
 60 60 | def f():
 61    |-    x = 0 or(deque)([])
    61 |+    x = 0 or(deque)()
+62 62 | 
+63 63 | 
+64 64 | # regression tests for https://github.com/astral-sh/ruff/issues/18612
+
+RUF037.py:66:5: RUF037 Unnecessary empty iterable within a deque call
+   |
+64 | # regression tests for https://github.com/astral-sh/ruff/issues/18612
+65 | def f():
+66 |     deque([], *[10])  # RUF037 but no fix
+   |     ^^^^^^^^^^^^^^^^ RUF037
+67 |     deque([], **{"maxlen": 10})  # RUF037
+68 |     deque([], foo=1)  # RUF037
+   |
+   = help: Replace with `deque()`
+
+RUF037.py:67:5: RUF037 [*] Unnecessary empty iterable within a deque call
+   |
+65 | def f():
+66 |     deque([], *[10])  # RUF037 but no fix
+67 |     deque([], **{"maxlen": 10})  # RUF037
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF037
+68 |     deque([], foo=1)  # RUF037
+   |
+   = help: Replace with `deque()`
+
+ℹ Unsafe fix
+64 64 | # regression tests for https://github.com/astral-sh/ruff/issues/18612
+65 65 | def f():
+66 66 |     deque([], *[10])  # RUF037 but no fix
+67    |-    deque([], **{"maxlen": 10})  # RUF037
+   67 |+    deque(**{"maxlen": 10})  # RUF037
+68 68 |     deque([], foo=1)  # RUF037
+69 69 | 
+70 70 | 
+
+RUF037.py:68:5: RUF037 [*] Unnecessary empty iterable within a deque call
+   |
+66 |     deque([], *[10])  # RUF037 but no fix
+67 |     deque([], **{"maxlen": 10})  # RUF037
+68 |     deque([], foo=1)  # RUF037
+   |     ^^^^^^^^^^^^^^^^ RUF037
+   |
+   = help: Replace with `deque()`
+
+ℹ Unsafe fix
+65 65 | def f():
+66 66 |     deque([], *[10])  # RUF037 but no fix
+67 67 |     deque([], **{"maxlen": 10})  # RUF037
+68    |-    deque([], foo=1)  # RUF037
+   68 |+    deque(foo=1)  # RUF037
+69 69 | 
+70 70 | 
+71 71 | # Somewhat related to the issue, both okay because we can't generally look
+
+RUF037.py:80:5: RUF037 [*] Unnecessary empty iterable within a deque call
+   |
+78 |   # are deleted
+79 |   def f():
+80 | /     deque(  # a comment in deque, deleted
+81 | |         [  # a comment _in_ the list, deleted
+82 | |         ],  # a comment after the list, deleted
+83 | |         maxlen=10,  # a comment on maxlen, deleted
+84 | |         )  # only this is preserved
+   | |_________^ RUF037
+   |
+   = help: Replace with `deque(maxlen=...)`
+
+ℹ Unsafe fix
+77 77 | # The fix was actually always unsafe in the presence of comments. all of these
+78 78 | # are deleted
+79 79 | def f():
+80    |-    deque(  # a comment in deque, deleted
+81    |-        [  # a comment _in_ the list, deleted
+82    |-        ],  # a comment after the list, deleted
+83    |-        maxlen=10,  # a comment on maxlen, deleted
+84    |-        )  # only this is preserved
+   80 |+    deque(maxlen=10)  # only this is preserved
+85 81 | 
+86 82 | 
+87 83 | # `maxlen` can also be passed positionally
+
+RUF037.py:89:5: RUF037 [*] Unnecessary empty iterable within a deque call
+   |
+87 | # `maxlen` can also be passed positionally
+88 | def f():
+89 |     deque([], 10)
+   |     ^^^^^^^^^^^^^ RUF037
+   |
+   = help: Replace with `deque(maxlen=...)`
+
+ℹ Safe fix
+86 86 | 
+87 87 | # `maxlen` can also be passed positionally
+88 88 | def f():
+89    |-    deque([], 10)
+   89 |+    deque(maxlen=10)
+90 90 | 
+91 91 | 
+92 92 | def f():
+
+RUF037.py:93:5: RUF037 [*] Unnecessary empty iterable within a deque call
+   |
+92 | def f():
+93 |     deque([], iterable=[])
+   |     ^^^^^^^^^^^^^^^^^^^^^^ RUF037
+   |
+   = help: Replace with `deque()`
+
+ℹ Unsafe fix
+90 90 | 
+91 91 | 
+92 92 | def f():
+93    |-    deque([], iterable=[])
+   93 |+    deque([])
