From f110afacf05cf732f5cb247fca953cc2a686e1e8 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 10:27:35 -0600 Subject: [PATCH 1/8] [flake8_comprehensions] add sum/min/max to unnecessary comprehension check (C419) --- .../fixtures/flake8_comprehensions/C419.py | 6 + .../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 | 2 +- .../rules/flake8_comprehensions/rules/mod.rs | 4 +- ...s => unnecessary_comprehension_in_call.rs} | 42 +++-- ...8_comprehensions__tests__C419_C419.py.snap | 172 ++++++++++++------ 8 files changed, 155 insertions(+), 79 deletions(-) rename crates/ruff_linter/src/rules/flake8_comprehensions/rules/{unnecessary_comprehension_any_all.rs => unnecessary_comprehension_in_call.rs} (60%) 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..6d383f049026a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py @@ -7,12 +7,18 @@ [x.id for x in bar], # second comment ) # third comment any({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]) # OK 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)) +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/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..5a5a3b79cd158 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs @@ -18,7 +18,7 @@ mod tests { #[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::UnnecessaryDoubleCastOrProcess, Path::new("C414.py"))] #[test_case(Rule::UnnecessaryGeneratorDict, Path::new("C402.py"))] #[test_case(Rule::UnnecessaryGeneratorList, Path::new("C400.py"))] 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 60% 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..8e5db63068dbd 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,19 @@ 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. Converting a generator to a list by way of a list +/// comprehension is unnecessary and wastes memory for large iterables by fully materializing a +/// list rather than handling values one at a time. /// -/// 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 +33,28 @@ 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 +/// 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). -/// -/// The generator version is more memory-efficient. +/// necessarily greater than generator overhead). So applying this rule simplifies the code and +/// will usually save memory, but in the absence of short-circuiting it may not improve (and may +/// even slightly regress, though the difference will be small) performance. /// /// ## 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 +63,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 +79,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, @@ -82,7 +92,7 @@ pub(crate) fn unnecessary_comprehension_any_all( let Expr::Name(ast::ExprName { id, .. }) = func else { return; }; - if !matches!(id.as_str(), "all" | "any") { + if !matches!(id.as_str(), "all" | "any" | "sum" | "min" | "max") { return; } let [arg] = args else { @@ -100,9 +110,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..b92f07952da69 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 @@ -75,7 +75,7 @@ C419.py:7:5: C419 [*] Unnecessary list comprehension 7 |+ x.id for x in bar # second comment 8 8 | ) # third comment 9 9 | any({x.id for x in bar}) -10 10 | +10 10 | sum([x.val for x in bar]) C419.py:9:5: C419 [*] Unnecessary list comprehension | @@ -83,8 +83,8 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 8 | ) # third comment 9 | any({x.id for x in bar}) | ^^^^^^^^^^^^^^^^^^^ C419 -10 | -11 | # OK +10 | sum([x.val for x in bar]) +11 | min([x.val for x in bar]) | = help: Remove unnecessary list comprehension @@ -94,71 +94,131 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 8 8 | ) # third comment 9 |-any({x.id for x in bar}) 9 |+any(x.id for x in bar) -10 10 | -11 11 | # OK -12 12 | all(x.id for x in bar) +10 10 | sum([x.val for x in bar]) +11 11 | min([x.val for x in bar]) +12 12 | max([x.val for x in bar]) -C419.py:24:5: C419 [*] Unnecessary list comprehension +C419.py:10:5: C419 [*] Unnecessary list comprehension | -22 | # Special comment handling -23 | any( -24 | [ # 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 - | |_____^ C419 -30 | # trailing comment -31 | ) + 8 | ) # third comment + 9 | any({x.id for x in bar}) +10 | sum([x.val for x in bar]) + | ^^^^^^^^^^^^^^^^^^^^ C419 +11 | min([x.val for x in bar]) +12 | max([x.val for x in bar]) | = 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 | - -C419.py:35:5: C419 [*] Unnecessary list comprehension +7 7 | [x.id for x in bar], # second comment +8 8 | ) # third comment +9 9 | any({x.id for x in bar}) +10 |-sum([x.val for x in bar]) + 10 |+sum(x.val for x in bar) +11 11 | min([x.val for x in bar]) +12 12 | max([x.val for x in bar]) +13 13 | + +C419.py:11: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 + 9 | any({x.id for x in bar}) +10 | sum([x.val for x in bar]) +11 | min([x.val for x in bar]) + | ^^^^^^^^^^^^^^^^^^^^ C419 +12 | max([x.val for x in bar]) + | + = help: Remove unnecessary list comprehension + +ℹ Unsafe fix +8 8 | ) # third comment +9 9 | any({x.id for x in bar}) +10 10 | sum([x.val for x in bar]) +11 |-min([x.val for x in bar]) + 11 |+min(x.val for x in bar) +12 12 | max([x.val for x in bar]) +13 13 | +14 14 | # OK + +C419.py:12:5: C419 [*] Unnecessary list comprehension + | +10 | sum([x.val for x in bar]) +11 | min([x.val for x in bar]) +12 | max([x.val for x in bar]) + | ^^^^^^^^^^^^^^^^^^^^ C419 +13 | +14 | # OK + | + = help: Remove unnecessary list comprehension + +ℹ Unsafe fix +9 9 | any({x.id for x in bar}) +10 10 | sum([x.val for x in bar]) +11 11 | min([x.val for x in bar]) +12 |-max([x.val for x in bar]) + 12 |+max(x.val for x in bar) +13 13 | +14 14 | # OK +15 15 | all(x.id for x in bar) + +C419.py:30:5: C419 [*] Unnecessary list comprehension + | +28 | # Special comment handling +29 | any( +30 | [ # lbracket comment | _____^ -36 | | # second line comment -37 | | i.bit_count() for i in range(5) # rbracket comment -38 | | ] # rpar comment +31 | | # second line comment +32 | | i.bit_count() +33 | | # random middle comment +34 | | for i in range(5) # rbracket comment +35 | | ] # rpar comment | |_____^ C419 -39 | ) +36 | # trailing comment +37 | ) | = 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 | ) +27 27 | +28 28 | # Special comment handling +29 29 | any( +30 |- [ # lbracket comment +31 |- # second line comment +32 |- i.bit_count() + 30 |+ # lbracket comment + 31 |+ # second line comment + 32 |+ i.bit_count() +33 33 | # random middle comment +34 |- for i in range(5) # rbracket comment +35 |- ] # rpar comment + 34 |+ for i in range(5) # rbracket comment # rpar comment +36 35 | # trailing comment +37 36 | ) +38 37 | +C419.py:41:5: C419 [*] Unnecessary list comprehension + | +39 | # Weird case where the function call, opening bracket, and comment are all +40 | # on the same line. +41 | any([ # lbracket comment + | _____^ +42 | | # second line comment +43 | | i.bit_count() for i in range(5) # rbracket comment +44 | | ] # rpar comment + | |_____^ C419 +45 | ) + | + = help: Remove unnecessary list comprehension +ℹ Unsafe fix +38 38 | +39 39 | # Weird case where the function call, opening bracket, and comment are all +40 40 | # on the same line. +41 |-any([ # lbracket comment +42 |- # second line comment +43 |- i.bit_count() for i in range(5) # rbracket comment +44 |- ] # rpar comment + 41 |+any( + 42 |+# lbracket comment + 43 |+# second line comment + 44 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment +45 45 | ) From 8d2d1049df53ec48ecc53ce3b84e01bc40572274 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 10:41:18 -0600 Subject: [PATCH 2/8] remove stray bracket --- .../rules/unnecessary_comprehension_in_call.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 8e5db63068dbd..3b959678510b0 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -53,7 +53,7 @@ use crate::rules::flake8_comprehensions::fixes; /// 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]) +/// min(x.val for x in bar) /// max(x.val for x in bar) /// ``` /// From bed402c9da552d85790f765f4de30867f1abd819 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 13:34:00 -0600 Subject: [PATCH 3/8] move sum/min/max to preview --- .../fixtures/flake8_comprehensions/C419.py | 13 +- .../fixtures/flake8_comprehensions/C419_1.py | 8 + .../src/rules/flake8_comprehensions/mod.rs | 19 ++ .../unnecessary_comprehension_in_call.rs | 5 +- ...8_comprehensions__tests__C419_C419.py.snap | 170 ++++++------------ ...sions__tests__preview__C419_C419_1.py.snap | 55 ++++++ 6 files changed, 147 insertions(+), 123 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_1.py 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 6d383f049026a..dbab803b20510 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py @@ -7,18 +7,19 @@ [x.id for x in bar], # second comment ) # third comment any({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]) # OK 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)) -sum(x.val for x in bar) -min(x.val for x in bar) -max(x.val for x in bar) +# no lint if shadowed +def all(x): pass +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/src/rules/flake8_comprehensions/mod.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs index 5a5a3b79cd158..ffa6b6a2bda4e 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs @@ -12,6 +12,7 @@ mod tests { use crate::assert_messages; use crate::registry::Rule; + use crate::settings::types::PreviewMode; use crate::settings::LinterSettings; use crate::test::test_path; @@ -43,6 +44,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/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 3b959678510b0..eadee12b925fc 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -89,10 +89,13 @@ pub(crate) fn unnecessary_comprehension_in_call( if !keywords.is_empty() { return; } + let Expr::Name(ast::ExprName { id, .. }) = func else { return; }; - if !matches!(id.as_str(), "all" | "any" | "sum" | "min" | "max") { + if !(matches!(id.as_str(), "any" | "all") + || (checker.settings.preview.is_enabled() && matches!(id.as_str(), "sum" | "min" | "max"))) + { return; } let [arg] = args else { 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 b92f07952da69..ca0eb0cf4817f 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 @@ -75,7 +75,7 @@ C419.py:7:5: C419 [*] Unnecessary list comprehension 7 |+ x.id for x in bar # second comment 8 8 | ) # third comment 9 9 | any({x.id for x in bar}) -10 10 | sum([x.val for x in bar]) +10 10 | C419.py:9:5: C419 [*] Unnecessary list comprehension | @@ -83,8 +83,8 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 8 | ) # third comment 9 | any({x.id for x in bar}) | ^^^^^^^^^^^^^^^^^^^ C419 -10 | sum([x.val for x in bar]) -11 | min([x.val for x in bar]) +10 | +11 | # OK | = help: Remove unnecessary list comprehension @@ -94,131 +94,69 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 8 8 | ) # third comment 9 |-any({x.id for x in bar}) 9 |+any(x.id for x in bar) -10 10 | sum([x.val for x in bar]) -11 11 | min([x.val for x in bar]) -12 12 | max([x.val for x in bar]) +10 10 | +11 11 | # OK +12 12 | all(x.id for x in bar) -C419.py:10:5: C419 [*] Unnecessary list comprehension +C419.py:31:5: C419 [*] Unnecessary list comprehension | - 8 | ) # third comment - 9 | any({x.id for x in bar}) -10 | sum([x.val for x in bar]) - | ^^^^^^^^^^^^^^^^^^^^ C419 -11 | min([x.val for x in bar]) -12 | max([x.val for x in bar]) - | - = help: Remove unnecessary list comprehension - -ℹ Unsafe fix -7 7 | [x.id for x in bar], # second comment -8 8 | ) # third comment -9 9 | any({x.id for x in bar}) -10 |-sum([x.val for x in bar]) - 10 |+sum(x.val for x in bar) -11 11 | min([x.val for x in bar]) -12 12 | max([x.val for x in bar]) -13 13 | - -C419.py:11:5: C419 [*] Unnecessary list comprehension - | - 9 | any({x.id for x in bar}) -10 | sum([x.val for x in bar]) -11 | min([x.val for x in bar]) - | ^^^^^^^^^^^^^^^^^^^^ C419 -12 | max([x.val for x in bar]) - | - = help: Remove unnecessary list comprehension - -ℹ Unsafe fix -8 8 | ) # third comment -9 9 | any({x.id for x in bar}) -10 10 | sum([x.val for x in bar]) -11 |-min([x.val for x in bar]) - 11 |+min(x.val for x in bar) -12 12 | max([x.val for x in bar]) -13 13 | -14 14 | # OK - -C419.py:12:5: C419 [*] Unnecessary list comprehension - | -10 | sum([x.val for x in bar]) -11 | min([x.val for x in bar]) -12 | max([x.val for x in bar]) - | ^^^^^^^^^^^^^^^^^^^^ C419 -13 | -14 | # OK - | - = help: Remove unnecessary list comprehension - -ℹ Unsafe fix -9 9 | any({x.id for x in bar}) -10 10 | sum([x.val for x in bar]) -11 11 | min([x.val for x in bar]) -12 |-max([x.val for x in bar]) - 12 |+max(x.val for x in bar) -13 13 | -14 14 | # OK -15 15 | all(x.id for x in bar) - -C419.py:30:5: C419 [*] Unnecessary list comprehension - | -28 | # Special comment handling -29 | any( -30 | [ # lbracket comment +29 | # Special comment handling +30 | any( +31 | [ # lbracket comment | _____^ -31 | | # second line comment -32 | | i.bit_count() -33 | | # random middle comment -34 | | for i in range(5) # rbracket comment -35 | | ] # rpar comment +32 | | # second line comment +33 | | i.bit_count() +34 | | # random middle comment +35 | | for i in range(5) # rbracket comment +36 | | ] # rpar comment | |_____^ C419 -36 | # trailing comment -37 | ) +37 | # trailing comment +38 | ) | = help: Remove unnecessary list comprehension ℹ Unsafe fix -27 27 | -28 28 | # Special comment handling -29 29 | any( -30 |- [ # lbracket comment -31 |- # second line comment -32 |- i.bit_count() - 30 |+ # lbracket comment - 31 |+ # second line comment - 32 |+ i.bit_count() -33 33 | # random middle comment -34 |- for i in range(5) # rbracket comment -35 |- ] # rpar comment - 34 |+ for i in range(5) # rbracket comment # rpar comment -36 35 | # trailing comment -37 36 | ) -38 37 | - -C419.py:41:5: C419 [*] Unnecessary list comprehension +28 28 | +29 29 | # Special comment handling +30 30 | any( +31 |- [ # lbracket comment +32 |- # second line comment +33 |- i.bit_count() + 31 |+ # lbracket comment + 32 |+ # second line comment + 33 |+ i.bit_count() +34 34 | # random middle comment +35 |- for i in range(5) # rbracket comment +36 |- ] # rpar comment + 35 |+ for i in range(5) # rbracket comment # rpar comment +37 36 | # trailing comment +38 37 | ) +39 38 | + +C419.py:42:5: C419 [*] Unnecessary list comprehension | -39 | # Weird case where the function call, opening bracket, and comment are all -40 | # on the same line. -41 | any([ # lbracket comment +40 | # Weird case where the function call, opening bracket, and comment are all +41 | # on the same line. +42 | any([ # lbracket comment | _____^ -42 | | # second line comment -43 | | i.bit_count() for i in range(5) # rbracket comment -44 | | ] # rpar comment +43 | | # second line comment +44 | | i.bit_count() for i in range(5) # rbracket comment +45 | | ] # rpar comment | |_____^ C419 -45 | ) +46 | ) | = help: Remove unnecessary list comprehension ℹ Unsafe fix -38 38 | -39 39 | # Weird case where the function call, opening bracket, and comment are all -40 40 | # on the same line. -41 |-any([ # lbracket comment -42 |- # second line comment -43 |- i.bit_count() for i in range(5) # rbracket comment -44 |- ] # rpar comment - 41 |+any( - 42 |+# lbracket comment - 43 |+# second line comment - 44 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment -45 45 | ) +39 39 | +40 40 | # Weird case where the function call, opening bracket, and comment are all +41 41 | # on the same line. +42 |-any([ # lbracket comment +43 |- # second line comment +44 |- i.bit_count() for i in range(5) # rbracket comment +45 |- ] # rpar comment + 42 |+any( + 43 |+# lbracket comment + 44 |+# second line comment + 45 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment +46 46 | ) 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) From 48eddcad60d8dd884a44fe62ff5768588c28e805 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:09:50 -0600 Subject: [PATCH 4/8] move intentional shadowing to its own test file --- .../fixtures/flake8_comprehensions/C419.py | 3 - .../fixtures/flake8_comprehensions/C419_2.py | 3 + ...8_comprehensions__tests__C419_C419.py.snap | 94 +++++++++---------- 3 files changed, 50 insertions(+), 50 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419_2.py 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 dbab803b20510..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,9 +13,6 @@ all(x.id for x in bar) any(x.id for x in bar) all((x.id for x in bar)) -# no lint if shadowed -def all(x): pass -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]) 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/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 ca0eb0cf4817f..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,65 +98,65 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 11 11 | # OK 12 12 | all(x.id for x in bar) -C419.py:31:5: C419 [*] Unnecessary list comprehension +C419.py:28:5: C419 [*] Unnecessary list comprehension | -29 | # Special comment handling -30 | any( -31 | [ # lbracket comment +26 | # Special comment handling +27 | any( +28 | [ # lbracket comment | _____^ -32 | | # second line comment -33 | | i.bit_count() -34 | | # random middle comment -35 | | for i in range(5) # rbracket comment -36 | | ] # 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 -37 | # trailing comment -38 | ) +34 | # trailing comment +35 | ) | = help: Remove unnecessary list comprehension ℹ Unsafe fix -28 28 | -29 29 | # Special comment handling -30 30 | any( -31 |- [ # lbracket comment -32 |- # second line comment -33 |- i.bit_count() - 31 |+ # lbracket comment - 32 |+ # second line comment - 33 |+ i.bit_count() -34 34 | # random middle comment -35 |- for i in range(5) # rbracket comment -36 |- ] # rpar comment - 35 |+ for i in range(5) # rbracket comment # rpar comment -37 36 | # trailing comment -38 37 | ) -39 38 | +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:42:5: C419 [*] Unnecessary list comprehension +C419.py:39:5: C419 [*] Unnecessary list comprehension | -40 | # Weird case where the function call, opening bracket, and comment are all -41 | # on the same line. -42 | 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 | _____^ -43 | | # second line comment -44 | | i.bit_count() for i in range(5) # rbracket comment -45 | | ] # rpar comment +40 | | # second line comment +41 | | i.bit_count() for i in range(5) # rbracket comment +42 | | ] # rpar comment | |_____^ C419 -46 | ) +43 | ) | = help: Remove unnecessary list comprehension ℹ Unsafe fix -39 39 | -40 40 | # Weird case where the function call, opening bracket, and comment are all -41 41 | # on the same line. -42 |-any([ # lbracket comment -43 |- # second line comment -44 |- i.bit_count() for i in range(5) # rbracket comment -45 |- ] # rpar comment - 42 |+any( - 43 |+# lbracket comment - 44 |+# second line comment - 45 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment -46 46 | ) +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 | ) From 1a0e067cb93a52dcbd8adcf84894963e18eb368c Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:11:58 -0600 Subject: [PATCH 5/8] actually use the new test file --- crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs | 1 + ...__rules__flake8_comprehensions__tests__C419_C419_2.py.snap | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419_2.py.snap diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs index ffa6b6a2bda4e..f1c765dff0646 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs @@ -20,6 +20,7 @@ mod tests { #[test_case(Rule::UnnecessaryCollectionCall, Path::new("C408.py"))] #[test_case(Rule::UnnecessaryComprehension, Path::new("C416.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"))] 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 +--- + From 9bb1617ca8e095d68661ecc61a95eeec4fe98f24 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:23:17 -0600 Subject: [PATCH 6/8] doc improvements --- .../rules/unnecessary_comprehension_in_call.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index eadee12b925fc..28a45f1b17138 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -15,7 +15,7 @@ use crate::rules::flake8_comprehensions::fixes; /// /// ## Why is this bad? /// Many builtin functions (this rule currently covers `any`, `all`, `min`, `max`, and `sum`) take -/// any iterable, including a generator. Converting a generator to a list by way of a list +/// any iterable, including a generator. Constructing a temporary list by way of a list /// comprehension is unnecessary and wastes memory for large iterables by fully materializing a /// list rather than handling values one at a time. /// @@ -33,11 +33,13 @@ 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). So applying this rule simplifies the code and -/// will usually save memory, but in the absence of short-circuiting it may not improve (and may -/// even slightly regress, though the difference will be small) performance. +/// 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. +/// +/// 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 From 226a543dbb1404d3f2c5175559b2b32b07a353ed Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:24:34 -0600 Subject: [PATCH 7/8] another doc tweak --- .../rules/unnecessary_comprehension_in_call.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 28a45f1b17138..061305c31a85c 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -15,9 +15,9 @@ use crate::rules::flake8_comprehensions::fixes; /// /// ## Why is this bad? /// Many builtin functions (this rule currently covers `any`, `all`, `min`, `max`, and `sum`) take -/// any iterable, including a generator. Constructing a temporary list by way of a list -/// comprehension is unnecessary and wastes memory for large iterables by fully materializing a -/// list rather than handling values one at a time. +/// any iterable, including a generator. Constructing a temporary list via list comprehension is +/// unnecessary and wastes memory for large iterables by fully materializing a list rather than +/// handling values one at a time. /// /// `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 From 2ed8db83236aad83d47e8d7a9766198e53eb78c2 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 3 Apr 2024 14:26:00 -0600 Subject: [PATCH 8/8] remove unnecessary clause --- .../rules/unnecessary_comprehension_in_call.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 061305c31a85c..901fb1e54abdf 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -16,8 +16,7 @@ use crate::rules::flake8_comprehensions::fixes; /// ## Why is this bad? /// 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 by fully materializing a list rather than -/// handling values one at a time. +/// unnecessary and wastes memory for large iterables. /// /// `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