[`flake8_comprehensions`] Handled special case for `C400` which also matches `C416` (#10419)

## Summary

Short-circuit implementation mentioned in #10403.

I implemented this by extending C400:
- Made `UnnecessaryGeneratorList` have information of whether the the
short-circuiting occurred (to put diagnostic)
- Add additional check for whether in `unnecessary_generator_list`
function.

Please give me suggestions if you think this isn't the best way to
handle this :)

## Test Plan

Extended `C400.py` a little, and written the cases where:
- Code could be converted to one single conversion to `list` e.g.
`list(x for x in range(3))` -> `list(range(3))`
- Code couldn't be converted to one single conversion to `list` e.g.
`list(2 * x for x in range(3))` -> `[2 * x for x in range(3)]`
- `list` function is not built-in, and should not modify the code in any
way.
This commit is contained in:
hikaru-kajita 2024-03-15 23:34:18 +09:00 committed by GitHub
parent 9675e1867a
commit 7e652e8fcb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 156 additions and 50 deletions

View File

@ -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))
x = list( x = list(
x for x in range(3) x for x in range(3)
) )
# Not built-in list.
def list(*args, **kwargs): def list(*args, **kwargs):
return None return None
list(2 * x for x in range(3))
list(x for x in range(3)) list(x for x in range(3))

View File

@ -1,6 +1,8 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast; use ruff_python_ast as ast;
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::ExprGenerator;
use ruff_text_size::{Ranged, TextSize}; use ruff_text_size::{Ranged, TextSize};
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -9,37 +11,53 @@ use super::helpers;
/// ## What it does /// ## What it does
/// Checks for unnecessary generators that can be rewritten as `list` /// Checks for unnecessary generators that can be rewritten as `list`
/// comprehensions. /// comprehensions (or with `list` directly).
/// ///
/// ## Why is this bad? /// ## Why is this bad?
/// It is unnecessary to use `list` around a generator expression, since /// It is unnecessary to use `list` around a generator expression, since
/// there are equivalent comprehensions for these types. Using a /// there are equivalent comprehensions for these types. Using a
/// comprehension 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 /// ## Examples
/// ```python /// ```python
/// list(f(x) for x in foo) /// list(f(x) for x in foo)
/// list(x for x in foo)
/// ``` /// ```
/// ///
/// Use instead: /// Use instead:
/// ```python /// ```python
/// [f(x) for x in foo] /// [f(x) for x in foo]
/// list(foo)
/// ``` /// ```
/// ///
/// ## Fix safety /// ## Fix safety
/// This rule's fix is marked as unsafe, as it may occasionally drop comments /// 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. /// when rewriting the call. In most cases, though, comments will be preserved.
#[violation] #[violation]
pub struct UnnecessaryGeneratorList; pub struct UnnecessaryGeneratorList {
short_circuit: bool,
}
impl AlwaysFixableViolation for UnnecessaryGeneratorList { impl AlwaysFixableViolation for UnnecessaryGeneratorList {
#[derive_message_formats] #[derive_message_formats]
fn message(&self) -> String { 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 { 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") { if !checker.semantic().is_builtin("list") {
return; 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]`. let Some(ExprGenerator {
diagnostic.set_fix({ elt, generators, ..
// Replace `list(` with `[`. }) = argument.as_generator_expr()
let call_start = Edit::replacement( else {
"[".to_string(), return;
call.start(), };
call.arguments.start() + TextSize::from(1),
);
// Replace `)` with `]`. // Short-circuit: given `list(x for x in y)`, generate `list(y)` (in lieu of `[x for x in y]`).
let call_end = Edit::replacement( if let [generator] = generators.as_slice() {
"]".to_string(), if generator.ifs.is_empty() && !generator.is_async {
call.arguments.end() - TextSize::from(1), if ComparableExpr::from(elt) == ComparableExpr::from(&generator.target) {
call.end(), let mut diagnostic = Diagnostic::new(
); UnnecessaryGeneratorList {
short_circuit: true,
Fix::unsafe_edits(call_start, [call_end]) },
}); call.range(),
);
checker.diagnostics.push(diagnostic); 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(),
);
Fix::unsafe_edits(call_start, [call_end])
});
checker.diagnostics.push(diagnostic);
} }

View File

@ -1,42 +1,90 @@
--- ---
source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs 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)) 1 | # Cannot combine with C416. Should use list comprehension here.
| ^^^^^^^^^^^^^^^^^^^^^^^^^ C400 2 | even_nums = list(2 * x for x in range(3))
2 | x = list( | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C400
3 | x for x in range(3) 3 | odd_nums = list(
4 | 2 * x + 1 for x in range(3)
| |
= help: Rewrite as a `list` comprehension = help: Rewrite as a `list` comprehension
Unsafe fix Unsafe fix
1 |-x = list(x for x in range(3)) 1 1 | # Cannot combine with C416. Should use list comprehension here.
1 |+x = [x for x in range(3)] 2 |-even_nums = list(2 * x for x in range(3))
2 2 | x = list( 2 |+even_nums = [2 * x for x in range(3)]
3 3 | x for x in range(3) 3 3 | odd_nums = list(
4 4 | ) 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)) 1 | # Cannot combine with C416. Should use list comprehension here.
2 | x = list( 2 | even_nums = list(2 * x for x in range(3))
| _____^ 3 | odd_nums = list(
3 | | x for x in range(3) | ____________^
4 | | ) 4 | | 2 * x + 1 for x in range(3)
5 | | )
| |_^ C400 | |_^ C400
| |
= help: Rewrite as a `list` comprehension = help: Rewrite as a `list` comprehension
Unsafe fix Unsafe fix
1 1 | x = list(x for x in range(3)) 1 1 | # Cannot combine with C416. Should use list comprehension here.
2 |-x = list( 2 2 | even_nums = list(2 * x for x in range(3))
2 |+x = [ 3 |-odd_nums = list(
3 3 | x for x in range(3) 3 |+odd_nums = [
4 |-) 4 4 | 2 * x + 1 for x in range(3)
4 |+] 5 |-)
5 5 | 5 |+]
6 6 | 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):