From a3d8cf421b83238e00d1def4db57cbd37379bb49 Mon Sep 17 00:00:00 2001 From: boolean-light <61526956+boolean-light@users.noreply.github.com> Date: Fri, 15 Mar 2024 16:45:16 +0900 Subject: [PATCH 1/3] Extended C400 and short-circuited fixation --- .../rules/unnecessary_generator_list.rs | 101 ++++++++++++++---- 1 file changed, 78 insertions(+), 23 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs index 8de73493b0136..46c3427aa005a 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs @@ -1,3 +1,4 @@ +use ast::{comparable::ComparableExpr, ExprGenerator}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; @@ -9,37 +10,49 @@ use super::helpers; /// ## What it does /// Checks for unnecessary generators that can be rewritten as `list` -/// comprehensions. +/// comprehensions or conversion to `list` using the builtin function. /// /// ## Why is this bad? /// It is unnecessary to use `list` around a generator expression, since /// there are equivalent comprehensions for these types. Using a -/// comprehension is clearer and more idiomatic. +/// comprehension/conversion to `list` is clearer and more idiomatic. /// /// ## Examples /// ```python /// list(f(x) for x in foo) +/// list(x for x in foo) /// ``` /// /// Use instead: /// ```python /// [f(x) for x in foo] +/// list(foo) /// ``` /// /// ## Fix safety /// This rule's fix is marked as unsafe, as it may occasionally drop comments /// when rewriting the call. In most cases, though, comments will be preserved. #[violation] -pub struct UnnecessaryGeneratorList; +pub struct UnnecessaryGeneratorList { + shortened_to_list_conversion: bool, +} impl AlwaysFixableViolation for UnnecessaryGeneratorList { #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary generator (rewrite as a `list` comprehension)") + if self.shortened_to_list_conversion { + format!("Unnecessary generator (rewrite using `list()`") + } else { + format!("Unnecessary generator (rewrite as a `list` comprehension)") + } } fn fix_title(&self) -> String { - "Rewrite as a `list` comprehension".to_string() + if self.shortened_to_list_conversion { + "Rewrite using `list()`".to_string() + } else { + "Rewrite as a `list` comprehension".to_string() + } } } @@ -56,28 +69,70 @@ pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::Expr if !checker.semantic().is_builtin("list") { return; } - if argument.is_generator_expr() { - let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorList, call.range()); - - // Convert `list(x for x in y)` to `[x for x in y]`. - diagnostic.set_fix({ - // Replace `list(` with `[`. - let call_start = Edit::replacement( - "[".to_string(), - call.start(), - call.arguments.start() + TextSize::from(1), + if let Some(generator_expr) = argument.clone().generator_expr() { + // Short-circuit the case for list(x for x in y) -> list(y) + let mut shortcircuit_completed = false; + 'shortcircuit: { + let mut diagnostic = Diagnostic::new( + UnnecessaryGeneratorList { + shortened_to_list_conversion: true, + }, + call.range(), ); - // Replace `)` with `]`. - let call_end = Edit::replacement( - "]".to_string(), - call.arguments.end() - TextSize::from(1), - call.end(), + let ExprGenerator { + range: _, + elt, + generators, + parenthesized: _, + } = generator_expr; + + let [generator] = &generators[..] else { + break 'shortcircuit; + }; + if !generator.ifs.is_empty() || generator.is_async { + break 'shortcircuit; + }; + if ComparableExpr::from(&elt) != ComparableExpr::from(&generator.target) { + break 'shortcircuit; + }; + + let iterator = format!("list({})", checker.locator().slice(generator.iter.range())); + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + iterator, + call.range(), + ))); + checker.diagnostics.push(diagnostic); + shortcircuit_completed = true; + } + + // Convert `list(f(x) for x in y)` to `[f(x) for x in y]`. + if !shortcircuit_completed { + let mut diagnostic = Diagnostic::new( + UnnecessaryGeneratorList { + shortened_to_list_conversion: false, + }, + call.range(), ); + diagnostic.set_fix({ + // Replace `list(` with `[`. + let call_start = Edit::replacement( + "[".to_string(), + call.start(), + call.arguments.start() + TextSize::from(1), + ); + + // Replace `)` with `]`. + let call_end = Edit::replacement( + "]".to_string(), + call.arguments.end() - TextSize::from(1), + call.end(), + ); - Fix::unsafe_edits(call_start, [call_end]) - }); + Fix::unsafe_edits(call_start, [call_end]) + }); - checker.diagnostics.push(diagnostic); + checker.diagnostics.push(diagnostic); + } } } From 79d5a64e6d859dcc3f2d2289fddc6b634e7c61c0 Mon Sep 17 00:00:00 2001 From: boolean-light <61526956+boolean-light@users.noreply.github.com> Date: Fri, 15 Mar 2024 17:00:52 +0900 Subject: [PATCH 2/3] Added new tests --- .../fixtures/flake8_comprehensions/C400.py | 11 ++- ...8_comprehensions__tests__C400_C400.py.snap | 96 ++++++++++++++----- 2 files changed, 82 insertions(+), 25 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py index ce76df9a8f7e3..16e29e4fb3364 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py @@ -1,11 +1,20 @@ +# Cannot combine with C416. Should use list comprehension here. +even_nums = list(2 * x for x in range(3)) +odd_nums = list( + 2 * x + 1 for x in range(3) +) + + +# Short-circuit case, combine with C416 and should produce x = list(range(3)) x = list(x for x in range(3)) x = list( x for x in range(3) ) - +# Not built-in list. def list(*args, **kwargs): return None +list(2 * x for x in range(3)) list(x for x in range(3)) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap index 56e0f8d95a866..ba92bc7d57530 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap @@ -1,42 +1,90 @@ --- source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs --- -C400.py:1:5: C400 [*] Unnecessary generator (rewrite as a `list` comprehension) +C400.py:2:13: C400 [*] Unnecessary generator (rewrite as a `list` comprehension) | -1 | x = list(x for x in range(3)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^ C400 -2 | x = list( -3 | x for x in range(3) +1 | # Cannot combine with C416. Should use list comprehension here. +2 | even_nums = list(2 * x for x in range(3)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C400 +3 | odd_nums = list( +4 | 2 * x + 1 for x in range(3) | = help: Rewrite as a `list` comprehension ℹ Unsafe fix -1 |-x = list(x for x in range(3)) - 1 |+x = [x for x in range(3)] -2 2 | x = list( -3 3 | x for x in range(3) -4 4 | ) +1 1 | # Cannot combine with C416. Should use list comprehension here. +2 |-even_nums = list(2 * x for x in range(3)) + 2 |+even_nums = [2 * x for x in range(3)] +3 3 | odd_nums = list( +4 4 | 2 * x + 1 for x in range(3) +5 5 | ) -C400.py:2:5: C400 [*] Unnecessary generator (rewrite as a `list` comprehension) +C400.py:3:12: C400 [*] Unnecessary generator (rewrite as a `list` comprehension) | -1 | x = list(x for x in range(3)) -2 | x = list( - | _____^ -3 | | x for x in range(3) -4 | | ) +1 | # Cannot combine with C416. Should use list comprehension here. +2 | even_nums = list(2 * x for x in range(3)) +3 | odd_nums = list( + | ____________^ +4 | | 2 * x + 1 for x in range(3) +5 | | ) | |_^ C400 | = help: Rewrite as a `list` comprehension ℹ Unsafe fix -1 1 | x = list(x for x in range(3)) -2 |-x = list( - 2 |+x = [ -3 3 | x for x in range(3) -4 |-) - 4 |+] -5 5 | +1 1 | # Cannot combine with C416. Should use list comprehension here. +2 2 | even_nums = list(2 * x for x in range(3)) +3 |-odd_nums = list( + 3 |+odd_nums = [ +4 4 | 2 * x + 1 for x in range(3) +5 |-) + 5 |+] 6 6 | -7 7 | def list(*args, **kwargs): +7 7 | +8 8 | # Short-circuit case, combine with C416 and should produce x = list(range(3)) +C400.py:9:5: C400 [*] Unnecessary generator (rewrite using `list()` + | + 8 | # Short-circuit case, combine with C416 and should produce x = list(range(3)) + 9 | x = list(x for x in range(3)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ C400 +10 | x = list( +11 | x for x in range(3) + | + = help: Rewrite using `list()` +ℹ Unsafe fix +6 6 | +7 7 | +8 8 | # Short-circuit case, combine with C416 and should produce x = list(range(3)) +9 |-x = list(x for x in range(3)) + 9 |+x = list(range(3)) +10 10 | x = list( +11 11 | x for x in range(3) +12 12 | ) + +C400.py:10:5: C400 [*] Unnecessary generator (rewrite using `list()` + | + 8 | # Short-circuit case, combine with C416 and should produce x = list(range(3)) + 9 | x = list(x for x in range(3)) +10 | x = list( + | _____^ +11 | | x for x in range(3) +12 | | ) + | |_^ C400 +13 | +14 | # Not built-in list. + | + = help: Rewrite using `list()` + +ℹ Unsafe fix +7 7 | +8 8 | # Short-circuit case, combine with C416 and should produce x = list(range(3)) +9 9 | x = list(x for x in range(3)) +10 |-x = list( +11 |- x for x in range(3) +12 |-) + 10 |+x = list(range(3)) +13 11 | +14 12 | # Not built-in list. +15 13 | def list(*args, **kwargs): From 1ca7107162ecb65bbb1098306765e33cce1d18c0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 15 Mar 2024 10:20:58 -0400 Subject: [PATCH 3/3] Tweaks --- .../rules/unnecessary_generator_list.rs | 124 +++++++++--------- 1 file changed, 59 insertions(+), 65 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs index 46c3427aa005a..7b9bb5e4bffd1 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs @@ -1,7 +1,8 @@ -use ast::{comparable::ComparableExpr, ExprGenerator}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; +use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::ExprGenerator; use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; @@ -10,12 +11,16 @@ use super::helpers; /// ## What it does /// Checks for unnecessary generators that can be rewritten as `list` -/// comprehensions or conversion to `list` using the builtin function. +/// comprehensions (or with `list` directly). /// /// ## Why is this bad? /// It is unnecessary to use `list` around a generator expression, since /// there are equivalent comprehensions for these types. Using a -/// comprehension/conversion to `list` is clearer and more idiomatic. +/// comprehension is clearer and more idiomatic. +/// +/// Further, if the comprehension can be removed entirely, as in the case of +/// `list(x for x in foo)`, it's better to use `list(foo)` directly, since it's +/// even more direct. /// /// ## Examples /// ```python @@ -34,13 +39,13 @@ use super::helpers; /// when rewriting the call. In most cases, though, comments will be preserved. #[violation] pub struct UnnecessaryGeneratorList { - shortened_to_list_conversion: bool, + short_circuit: bool, } impl AlwaysFixableViolation for UnnecessaryGeneratorList { #[derive_message_formats] fn message(&self) -> String { - if self.shortened_to_list_conversion { + if self.short_circuit { format!("Unnecessary generator (rewrite using `list()`") } else { format!("Unnecessary generator (rewrite as a `list` comprehension)") @@ -48,7 +53,7 @@ impl AlwaysFixableViolation for UnnecessaryGeneratorList { } fn fix_title(&self) -> String { - if self.shortened_to_list_conversion { + if self.short_circuit { "Rewrite using `list()`".to_string() } else { "Rewrite as a `list` comprehension".to_string() @@ -69,70 +74,59 @@ pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::Expr if !checker.semantic().is_builtin("list") { return; } - if let Some(generator_expr) = argument.clone().generator_expr() { - // Short-circuit the case for list(x for x in y) -> list(y) - let mut shortcircuit_completed = false; - 'shortcircuit: { - let mut diagnostic = Diagnostic::new( - UnnecessaryGeneratorList { - shortened_to_list_conversion: true, - }, - call.range(), - ); - - let ExprGenerator { - range: _, - elt, - generators, - parenthesized: _, - } = generator_expr; - let [generator] = &generators[..] else { - break 'shortcircuit; - }; - if !generator.ifs.is_empty() || generator.is_async { - break 'shortcircuit; - }; - if ComparableExpr::from(&elt) != ComparableExpr::from(&generator.target) { - break 'shortcircuit; - }; + let Some(ExprGenerator { + elt, generators, .. + }) = argument.as_generator_expr() + else { + return; + }; - let iterator = format!("list({})", checker.locator().slice(generator.iter.range())); - diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( - iterator, - call.range(), - ))); - checker.diagnostics.push(diagnostic); - shortcircuit_completed = true; + // Short-circuit: given `list(x for x in y)`, generate `list(y)` (in lieu of `[x for x in y]`). + if let [generator] = generators.as_slice() { + if generator.ifs.is_empty() && !generator.is_async { + if ComparableExpr::from(elt) == ComparableExpr::from(&generator.target) { + let mut diagnostic = Diagnostic::new( + UnnecessaryGeneratorList { + short_circuit: true, + }, + call.range(), + ); + let iterator = format!("list({})", checker.locator().slice(generator.iter.range())); + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + iterator, + call.range(), + ))); + checker.diagnostics.push(diagnostic); + return; + } } + } - // Convert `list(f(x) for x in y)` to `[f(x) for x in y]`. - if !shortcircuit_completed { - let mut diagnostic = Diagnostic::new( - UnnecessaryGeneratorList { - shortened_to_list_conversion: false, - }, - call.range(), - ); - diagnostic.set_fix({ - // Replace `list(` with `[`. - let call_start = Edit::replacement( - "[".to_string(), - call.start(), - call.arguments.start() + TextSize::from(1), - ); + // Convert `list(f(x) for x in y)` to `[f(x) for x in y]`. + let mut diagnostic = Diagnostic::new( + UnnecessaryGeneratorList { + short_circuit: false, + }, + call.range(), + ); + diagnostic.set_fix({ + // Replace `list(` with `[`. + let call_start = Edit::replacement( + "[".to_string(), + call.start(), + call.arguments.start() + TextSize::from(1), + ); - // Replace `)` with `]`. - let call_end = Edit::replacement( - "]".to_string(), - call.arguments.end() - TextSize::from(1), - call.end(), - ); + // Replace `)` with `]`. + let call_end = Edit::replacement( + "]".to_string(), + call.arguments.end() - TextSize::from(1), + call.end(), + ); - Fix::unsafe_edits(call_start, [call_end]) - }); + Fix::unsafe_edits(call_start, [call_end]) + }); - checker.diagnostics.push(diagnostic); - } - } + checker.diagnostics.push(diagnostic); }