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/rules/unnecessary_generator_list.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs index 8de73493b0136..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,6 +1,8 @@ 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; @@ -9,37 +11,53 @@ use super::helpers; /// ## What it does /// Checks for unnecessary generators that can be rewritten as `list` -/// comprehensions. +/// 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 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 /// 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 { + short_circuit: bool, +} impl AlwaysFixableViolation for UnnecessaryGeneratorList { #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary generator (rewrite as a `list` comprehension)") + if self.short_circuit { + 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.short_circuit { + "Rewrite using `list()`".to_string() + } else { + "Rewrite as a `list` comprehension".to_string() + } } } @@ -56,28 +74,59 @@ 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), - ); + let Some(ExprGenerator { + elt, generators, .. + }) = argument.as_generator_expr() + else { + return; + }; + + // 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]`. + 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); } 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):