mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-03 18:28:24 +00:00
[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.
This commit is contained in:
parent
8123dab05a
commit
96171f41c2
3 changed files with 212 additions and 21 deletions
|
@ -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=[])
|
||||
|
|
|
@ -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<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(),
|
||||
)?
|
||||
};
|
||||
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))
|
||||
}
|
||||
|
|
|
@ -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([])
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue