Skip to content

Commit

Permalink
[flake8_comprehensions] add sum/min/max to unnecessary comprehension …
Browse files Browse the repository at this point in the history
…check (C419) (astral-sh#10759)

Fixes astral-sh#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.
  • Loading branch information
carljm authored and Glyphack committed Apr 12, 2024
1 parent 56be587 commit 2771cdd
Show file tree
Hide file tree
Showing 12 changed files with 178 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
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:
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# no lint if shadowed
def all(x): pass
all([x.id for x in bar])
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
22 changes: 21 additions & 1 deletion crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand All @@ -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!(
Expand Down
Original file line number Diff line number Diff line change
@@ -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::*;
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)])
Expand All @@ -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
Expand All @@ -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]
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 | )
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
---

Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit 2771cdd

Please sign in to comment.