mirror of https://github.com/astral-sh/ruff
[flake8_comprehensions] add sum/min/max to unnecessary comprehension check (C419) (#10759)
Fixes #3259 ## Summary Renames `UnnecessaryComprehensionAnyAll` to `UnnecessaryComprehensionInCall` and extends the check to `sum`, `min`, and `max`, in addition to `any` and `all`. ## Test Plan Updated snapshot test. Built docs locally and verified the docs for this rule still render correctly.
This commit is contained in:
Carl Meyer
GitHub
parent
e0a8fb607a
commit
5e2482824c
|
|
@ -13,6 +13,10 @@ all(x.id for x in bar)
|
|||
all(x.id for x in bar)
|
||||
any(x.id for x in bar)
|
||||
all((x.id for x in bar))
|
||||
# we don't lint on these in stable yet
|
||||
sum([x.val for x in bar])
|
||||
min([x.val for x in bar])
|
||||
max([x.val for x in bar])
|
||||
|
||||
|
||||
async def f() -> bool:
|
||||
|
|
|
|||
|
|
@ -0,0 +1,8 @@
|
|||
sum([x.val for x in bar])
|
||||
min([x.val for x in bar])
|
||||
max([x.val for x in bar])
|
||||
|
||||
# Ok
|
||||
sum(x.val for x in bar)
|
||||
min(x.val for x in bar)
|
||||
max(x.val for x in bar)
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
# no lint if shadowed
|
||||
def all(x): pass
|
||||
all([x.id for x in bar])
|
||||
|
|
@ -705,8 +705,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
|
|||
args,
|
||||
);
|
||||
}
|
||||
if checker.enabled(Rule::UnnecessaryComprehensionAnyAll) {
|
||||
flake8_comprehensions::rules::unnecessary_comprehension_any_all(
|
||||
if checker.enabled(Rule::UnnecessaryComprehensionInCall) {
|
||||
flake8_comprehensions::rules::unnecessary_comprehension_in_call(
|
||||
checker, expr, func, args, keywords,
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -398,7 +398,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
|
|||
(Flake8Comprehensions, "16") => (RuleGroup::Stable, rules::flake8_comprehensions::rules::UnnecessaryComprehension),
|
||||
(Flake8Comprehensions, "17") => (RuleGroup::Stable, rules::flake8_comprehensions::rules::UnnecessaryMap),
|
||||
(Flake8Comprehensions, "18") => (RuleGroup::Stable, rules::flake8_comprehensions::rules::UnnecessaryLiteralWithinDictCall),
|
||||
(Flake8Comprehensions, "19") => (RuleGroup::Stable, rules::flake8_comprehensions::rules::UnnecessaryComprehensionAnyAll),
|
||||
(Flake8Comprehensions, "19") => (RuleGroup::Stable, rules::flake8_comprehensions::rules::UnnecessaryComprehensionInCall),
|
||||
|
||||
// flake8-debugger
|
||||
(Flake8Debugger, "0") => (RuleGroup::Stable, rules::flake8_debugger::rules::Debugger),
|
||||
|
|
|
|||
|
|
@ -793,7 +793,7 @@ pub(crate) fn fix_unnecessary_map(
|
|||
}
|
||||
|
||||
/// (C419) Convert `[i for i in a]` into `i for i in a`
|
||||
pub(crate) fn fix_unnecessary_comprehension_any_all(
|
||||
pub(crate) fn fix_unnecessary_comprehension_in_call(
|
||||
expr: &Expr,
|
||||
locator: &Locator,
|
||||
stylist: &Stylist,
|
||||
|
|
|
|||
|
|
@ -12,13 +12,15 @@ mod tests {
|
|||
|
||||
use crate::assert_messages;
|
||||
use crate::registry::Rule;
|
||||
use crate::settings::types::PreviewMode;
|
||||
use crate::settings::LinterSettings;
|
||||
use crate::test::test_path;
|
||||
|
||||
#[test_case(Rule::UnnecessaryCallAroundSorted, Path::new("C413.py"))]
|
||||
#[test_case(Rule::UnnecessaryCollectionCall, Path::new("C408.py"))]
|
||||
#[test_case(Rule::UnnecessaryComprehension, Path::new("C416.py"))]
|
||||
#[test_case(Rule::UnnecessaryComprehensionAnyAll, Path::new("C419.py"))]
|
||||
#[test_case(Rule::UnnecessaryComprehensionInCall, Path::new("C419.py"))]
|
||||
#[test_case(Rule::UnnecessaryComprehensionInCall, Path::new("C419_2.py"))]
|
||||
#[test_case(Rule::UnnecessaryDoubleCastOrProcess, Path::new("C414.py"))]
|
||||
#[test_case(Rule::UnnecessaryGeneratorDict, Path::new("C402.py"))]
|
||||
#[test_case(Rule::UnnecessaryGeneratorList, Path::new("C400.py"))]
|
||||
|
|
@ -43,6 +45,24 @@ mod tests {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(Rule::UnnecessaryComprehensionInCall, Path::new("C419_1.py"))]
|
||||
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
|
||||
let snapshot = format!(
|
||||
"preview__{}_{}",
|
||||
rule_code.noqa_code(),
|
||||
path.to_string_lossy()
|
||||
);
|
||||
let diagnostics = test_path(
|
||||
Path::new("flake8_comprehensions").join(path).as_path(),
|
||||
&LinterSettings {
|
||||
preview: PreviewMode::Enabled,
|
||||
..LinterSettings::for_rule(rule_code)
|
||||
},
|
||||
)?;
|
||||
assert_messages!(snapshot, diagnostics);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(Rule::UnnecessaryCollectionCall, Path::new("C408.py"))]
|
||||
fn allow_dict_calls_with_keyword_arguments(rule_code: Rule, path: &Path) -> Result<()> {
|
||||
let snapshot = format!(
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
pub(crate) use unnecessary_call_around_sorted::*;
|
||||
pub(crate) use unnecessary_collection_call::*;
|
||||
pub(crate) use unnecessary_comprehension::*;
|
||||
pub(crate) use unnecessary_comprehension_any_all::*;
|
||||
pub(crate) use unnecessary_comprehension_in_call::*;
|
||||
pub(crate) use unnecessary_double_cast_or_process::*;
|
||||
pub(crate) use unnecessary_generator_dict::*;
|
||||
pub(crate) use unnecessary_generator_list::*;
|
||||
|
|
@ -21,7 +21,7 @@ mod helpers;
|
|||
mod unnecessary_call_around_sorted;
|
||||
mod unnecessary_collection_call;
|
||||
mod unnecessary_comprehension;
|
||||
mod unnecessary_comprehension_any_all;
|
||||
mod unnecessary_comprehension_in_call;
|
||||
mod unnecessary_double_cast_or_process;
|
||||
mod unnecessary_generator_dict;
|
||||
mod unnecessary_generator_list;
|
||||
|
|
|
|||
|
|
@ -11,15 +11,18 @@ use crate::checkers::ast::Checker;
|
|||
use crate::rules::flake8_comprehensions::fixes;
|
||||
|
||||
/// ## What it does
|
||||
/// Checks for unnecessary list comprehensions passed to `any` and `all`.
|
||||
/// Checks for unnecessary list comprehensions passed to builtin functions that take an iterable.
|
||||
///
|
||||
/// ## Why is this bad?
|
||||
/// `any` and `all` take any iterators, including generators. Converting a generator to a list
|
||||
/// by way of a list comprehension is unnecessary and requires iterating all values, even if `any`
|
||||
/// or `all` could short-circuit early.
|
||||
/// Many builtin functions (this rule currently covers `any`, `all`, `min`, `max`, and `sum`) take
|
||||
/// any iterable, including a generator. Constructing a temporary list via list comprehension is
|
||||
/// unnecessary and wastes memory for large iterables.
|
||||
///
|
||||
/// For example, compare the performance of `all` with a list comprehension against that
|
||||
/// of a generator in a case where an early short-circuit is possible (almost 40x faster):
|
||||
/// `any` and `all` can also short-circuit iteration, saving a lot of time. The unnecessary
|
||||
/// comprehension forces a full iteration of the input iterable, giving up the benefits of
|
||||
/// short-circuiting. For example, compare the performance of `all` with a list comprehension
|
||||
/// against that of a generator in a case where an early short-circuit is possible (almost 40x
|
||||
/// faster):
|
||||
///
|
||||
/// ```console
|
||||
/// In [1]: %timeit all([i for i in range(1000)])
|
||||
|
|
@ -29,22 +32,30 @@ use crate::rules::flake8_comprehensions::fixes;
|
|||
/// 212 ns ± 0.892 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
|
||||
/// ```
|
||||
///
|
||||
/// This performance difference is due to short-circuiting; if the entire iterable has to be
|
||||
/// traversed, the comprehension version may even be a bit faster (list allocation overhead is not
|
||||
/// necessarily greater than generator overhead).
|
||||
/// This performance improvement is due to short-circuiting. If the entire iterable has to be
|
||||
/// traversed, the comprehension version may even be a bit faster: list allocation overhead is not
|
||||
/// necessarily greater than generator overhead.
|
||||
///
|
||||
/// The generator version is more memory-efficient.
|
||||
/// Applying this rule simplifies the code and will usually save memory, but in the absence of
|
||||
/// short-circuiting it may not improve performance. (It may even slightly regress performance,
|
||||
/// though the difference will usually be small.)
|
||||
///
|
||||
/// ## Examples
|
||||
/// ```python
|
||||
/// any([x.id for x in bar])
|
||||
/// all([x.id for x in bar])
|
||||
/// sum([x.val for x in bar])
|
||||
/// min([x.val for x in bar])
|
||||
/// max([x.val for x in bar])
|
||||
/// ```
|
||||
///
|
||||
/// Use instead:
|
||||
/// ```python
|
||||
/// any(x.id for x in bar)
|
||||
/// all(x.id for x in bar)
|
||||
/// sum(x.val for x in bar)
|
||||
/// min(x.val for x in bar)
|
||||
/// max(x.val for x in bar)
|
||||
/// ```
|
||||
///
|
||||
/// ## Fix safety
|
||||
|
|
@ -53,9 +64,9 @@ use crate::rules::flake8_comprehensions::fixes;
|
|||
/// rewriting some comprehensions.
|
||||
///
|
||||
#[violation]
|
||||
pub struct UnnecessaryComprehensionAnyAll;
|
||||
pub struct UnnecessaryComprehensionInCall;
|
||||
|
||||
impl Violation for UnnecessaryComprehensionAnyAll {
|
||||
impl Violation for UnnecessaryComprehensionInCall {
|
||||
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
|
||||
|
||||
#[derive_message_formats]
|
||||
|
|
@ -69,7 +80,7 @@ impl Violation for UnnecessaryComprehensionAnyAll {
|
|||
}
|
||||
|
||||
/// C419
|
||||
pub(crate) fn unnecessary_comprehension_any_all(
|
||||
pub(crate) fn unnecessary_comprehension_in_call(
|
||||
checker: &mut Checker,
|
||||
expr: &Expr,
|
||||
func: &Expr,
|
||||
|
|
@ -79,10 +90,13 @@ pub(crate) fn unnecessary_comprehension_any_all(
|
|||
if !keywords.is_empty() {
|
||||
return;
|
||||
}
|
||||
|
||||
let Expr::Name(ast::ExprName { id, .. }) = func else {
|
||||
return;
|
||||
};
|
||||
if !matches!(id.as_str(), "all" | "any") {
|
||||
if !(matches!(id.as_str(), "any" | "all")
|
||||
|| (checker.settings.preview.is_enabled() && matches!(id.as_str(), "sum" | "min" | "max")))
|
||||
{
|
||||
return;
|
||||
}
|
||||
let [arg] = args else {
|
||||
|
|
@ -100,9 +114,9 @@ pub(crate) fn unnecessary_comprehension_any_all(
|
|||
return;
|
||||
}
|
||||
|
||||
let mut diagnostic = Diagnostic::new(UnnecessaryComprehensionAnyAll, arg.range());
|
||||
let mut diagnostic = Diagnostic::new(UnnecessaryComprehensionInCall, arg.range());
|
||||
diagnostic.try_set_fix(|| {
|
||||
fixes::fix_unnecessary_comprehension_any_all(expr, checker.locator(), checker.stylist())
|
||||
fixes::fix_unnecessary_comprehension_in_call(expr, checker.locator(), checker.stylist())
|
||||
});
|
||||
checker.diagnostics.push(diagnostic);
|
||||
}
|
||||
|
|
@ -98,67 +98,65 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension
|
|||
11 11 | # OK
|
||||
12 12 | all(x.id for x in bar)
|
||||
|
||||
C419.py:24:5: C419 [*] Unnecessary list comprehension
|
||||
C419.py:28:5: C419 [*] Unnecessary list comprehension
|
||||
|
|
||||
22 | # Special comment handling
|
||||
23 | any(
|
||||
24 | [ # lbracket comment
|
||||
26 | # Special comment handling
|
||||
27 | any(
|
||||
28 | [ # lbracket comment
|
||||
| _____^
|
||||
25 | | # second line comment
|
||||
26 | | i.bit_count()
|
||||
27 | | # random middle comment
|
||||
28 | | for i in range(5) # rbracket comment
|
||||
29 | | ] # rpar comment
|
||||
29 | | # second line comment
|
||||
30 | | i.bit_count()
|
||||
31 | | # random middle comment
|
||||
32 | | for i in range(5) # rbracket comment
|
||||
33 | | ] # rpar comment
|
||||
| |_____^ C419
|
||||
30 | # trailing comment
|
||||
31 | )
|
||||
34 | # trailing comment
|
||||
35 | )
|
||||
|
|
||||
= help: Remove unnecessary list comprehension
|
||||
|
||||
ℹ Unsafe fix
|
||||
21 21 |
|
||||
22 22 | # Special comment handling
|
||||
23 23 | any(
|
||||
24 |- [ # lbracket comment
|
||||
25 |- # second line comment
|
||||
26 |- i.bit_count()
|
||||
24 |+ # lbracket comment
|
||||
25 |+ # second line comment
|
||||
26 |+ i.bit_count()
|
||||
27 27 | # random middle comment
|
||||
28 |- for i in range(5) # rbracket comment
|
||||
29 |- ] # rpar comment
|
||||
28 |+ for i in range(5) # rbracket comment # rpar comment
|
||||
30 29 | # trailing comment
|
||||
31 30 | )
|
||||
32 31 |
|
||||
25 25 |
|
||||
26 26 | # Special comment handling
|
||||
27 27 | any(
|
||||
28 |- [ # lbracket comment
|
||||
29 |- # second line comment
|
||||
30 |- i.bit_count()
|
||||
28 |+ # lbracket comment
|
||||
29 |+ # second line comment
|
||||
30 |+ i.bit_count()
|
||||
31 31 | # random middle comment
|
||||
32 |- for i in range(5) # rbracket comment
|
||||
33 |- ] # rpar comment
|
||||
32 |+ for i in range(5) # rbracket comment # rpar comment
|
||||
34 33 | # trailing comment
|
||||
35 34 | )
|
||||
36 35 |
|
||||
|
||||
C419.py:35:5: C419 [*] Unnecessary list comprehension
|
||||
C419.py:39:5: C419 [*] Unnecessary list comprehension
|
||||
|
|
||||
33 | # Weird case where the function call, opening bracket, and comment are all
|
||||
34 | # on the same line.
|
||||
35 | any([ # lbracket comment
|
||||
37 | # Weird case where the function call, opening bracket, and comment are all
|
||||
38 | # on the same line.
|
||||
39 | any([ # lbracket comment
|
||||
| _____^
|
||||
36 | | # second line comment
|
||||
37 | | i.bit_count() for i in range(5) # rbracket comment
|
||||
38 | | ] # rpar comment
|
||||
40 | | # second line comment
|
||||
41 | | i.bit_count() for i in range(5) # rbracket comment
|
||||
42 | | ] # rpar comment
|
||||
| |_____^ C419
|
||||
39 | )
|
||||
43 | )
|
||||
|
|
||||
= help: Remove unnecessary list comprehension
|
||||
|
||||
ℹ Unsafe fix
|
||||
32 32 |
|
||||
33 33 | # Weird case where the function call, opening bracket, and comment are all
|
||||
34 34 | # on the same line.
|
||||
35 |-any([ # lbracket comment
|
||||
36 |- # second line comment
|
||||
37 |- i.bit_count() for i in range(5) # rbracket comment
|
||||
38 |- ] # rpar comment
|
||||
35 |+any(
|
||||
36 |+# lbracket comment
|
||||
37 |+# second line comment
|
||||
38 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment
|
||||
39 39 | )
|
||||
|
||||
|
||||
36 36 |
|
||||
37 37 | # Weird case where the function call, opening bracket, and comment are all
|
||||
38 38 | # on the same line.
|
||||
39 |-any([ # lbracket comment
|
||||
40 |- # second line comment
|
||||
41 |- i.bit_count() for i in range(5) # rbracket comment
|
||||
42 |- ] # rpar comment
|
||||
39 |+any(
|
||||
40 |+# lbracket comment
|
||||
41 |+# second line comment
|
||||
42 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment
|
||||
43 43 | )
|
||||
|
|
|
|||
|
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
|
||||
---
|
||||
|
||||
|
|
@ -0,0 +1,55 @@
|
|||
---
|
||||
source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
|
||||
---
|
||||
C419_1.py:1:5: C419 [*] Unnecessary list comprehension
|
||||
|
|
||||
1 | sum([x.val for x in bar])
|
||||
| ^^^^^^^^^^^^^^^^^^^^ C419
|
||||
2 | min([x.val for x in bar])
|
||||
3 | max([x.val for x in bar])
|
||||
|
|
||||
= help: Remove unnecessary list comprehension
|
||||
|
||||
ℹ Unsafe fix
|
||||
1 |-sum([x.val for x in bar])
|
||||
1 |+sum(x.val for x in bar)
|
||||
2 2 | min([x.val for x in bar])
|
||||
3 3 | max([x.val for x in bar])
|
||||
4 4 |
|
||||
|
||||
C419_1.py:2:5: C419 [*] Unnecessary list comprehension
|
||||
|
|
||||
1 | sum([x.val for x in bar])
|
||||
2 | min([x.val for x in bar])
|
||||
| ^^^^^^^^^^^^^^^^^^^^ C419
|
||||
3 | max([x.val for x in bar])
|
||||
|
|
||||
= help: Remove unnecessary list comprehension
|
||||
|
||||
ℹ Unsafe fix
|
||||
1 1 | sum([x.val for x in bar])
|
||||
2 |-min([x.val for x in bar])
|
||||
2 |+min(x.val for x in bar)
|
||||
3 3 | max([x.val for x in bar])
|
||||
4 4 |
|
||||
5 5 | # Ok
|
||||
|
||||
C419_1.py:3:5: C419 [*] Unnecessary list comprehension
|
||||
|
|
||||
1 | sum([x.val for x in bar])
|
||||
2 | min([x.val for x in bar])
|
||||
3 | max([x.val for x in bar])
|
||||
| ^^^^^^^^^^^^^^^^^^^^ C419
|
||||
4 |
|
||||
5 | # Ok
|
||||
|
|
||||
= help: Remove unnecessary list comprehension
|
||||
|
||||
ℹ Unsafe fix
|
||||
1 1 | sum([x.val for x in bar])
|
||||
2 2 | min([x.val for x in bar])
|
||||
3 |-max([x.val for x in bar])
|
||||
3 |+max(x.val for x in bar)
|
||||
4 4 |
|
||||
5 5 | # Ok
|
||||
6 6 | sum(x.val for x in bar)
|
||||
Loading…
Reference in New Issue