From 5e2482824c029473f14f83677f0ab06471a5bc33 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:44:33 -0600 Subject: [PATCH] [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. --- .../fixtures/flake8_comprehensions/C419.py | 4 + .../fixtures/flake8_comprehensions/C419_1.py | 8 ++ .../fixtures/flake8_comprehensions/C419_2.py | 3 + .../src/checkers/ast/analyze/expression.rs | 4 +- crates/ruff_linter/src/codes.rs | 2 +- .../src/rules/flake8_comprehensions/fixes.rs | 2 +- .../src/rules/flake8_comprehensions/mod.rs | 22 ++++- .../rules/flake8_comprehensions/rules/mod.rs | 4 +- ...s => unnecessary_comprehension_in_call.rs} | 46 +++++---- ...8_comprehensions__tests__C419_C419.py.snap | 96 +++++++++---------- ...comprehensions__tests__C419_C419_2.py.snap | 4 + ...sions__tests__preview__C419_C419_1.py.snap | 55 +++++++++++ 12 files changed, 178 insertions(+), 72 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_1.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_2.py rename crates/ruff_linter/src/rules/flake8_comprehensions/rules/{unnecessary_comprehension_any_all.rs => unnecessary_comprehension_in_call.rs} (59%) create mode 100644 crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419_2.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py index 4a9671b1ee020..b0a15cf2d6aac 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py @@ -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: diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_1.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_1.py new file mode 100644 index 0000000000000..b0a521e2363d2 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_1.py @@ -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) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_2.py new file mode 100644 index 0000000000000..f8848634547f1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_2.py @@ -0,0 +1,3 @@ +# no lint if shadowed +def all(x): pass +all([x.id for x in bar]) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 5a64941417858..b3fc11d7f7e15 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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, ); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 60950ee89288c..75cb2966bc30f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index 1febae119798b..295d8463b8015 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -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, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs index 74c2a69ac323f..f1c765dff0646 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs @@ -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!( diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/mod.rs index 2c24ecfc2fa47..ff54ed3c38137 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/mod.rs @@ -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; diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs similarity index 59% rename from crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs rename to crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index de538c6f8f184..901fb1e54abdf 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -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); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap index 4fd150bc02551..026bbd7fe75ef 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap @@ -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 | ) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419_2.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419_2.py.snap new file mode 100644 index 0000000000000..d9845b4ae92f9 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419_2.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap new file mode 100644 index 0000000000000..559a6bed9ef02 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap @@ -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)