From 96171f41c23d342c4a60bfaa9fd2ac52f4e7c304 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 12 Jun 2025 09:07:17 -0400 Subject: [PATCH] [`ruff`] Handle extra arguments to `deque` (`RUF037`) (#18614) ## Summary Fixes https://github.com/astral-sh/ruff/issues/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 https://github.com/astral-sh/ruff/pull/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. --- .../resources/test/fixtures/ruff/RUF037.py | 32 +++++ .../unnecessary_literal_within_deque_call.rs | 87 +++++++++---- ..._rules__ruff__tests__RUF037_RUF037.py.snap | 114 ++++++++++++++++++ 3 files changed, 212 insertions(+), 21 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF037.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF037.py index 2893daecc6..18807d4794 100644 --- 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 index 7050f43b9f..115fc5c136 100644 --- 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,6 +32,15 @@ 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)] @@ -37,19 +48,21 @@ 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 { 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 { + // 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(), + )? }; - Fix::safe_edit(Edit::range_replacement(deque_str, deque.range)) + 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 + }; + + 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 index 09d6f86171..3b8f271e28 100644 --- 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([])