Skip to content

Commit

Permalink
[flake8_comprehensions] Handled special case for C400 which also …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@boolean-light
boolean-light authored Mar 15, 2024
1 parent 9675e18 commit 7e652e8
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 49 deletions.
11 changes: 10 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py
View file
Original file line number Diff line number Diff line change
@@ -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))
97 changes: 73 additions & 24 deletions crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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()
}
}
}

Expand All @@ -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);
}
96 changes: 72 additions & 24 deletions ...prehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap
View file
Original file line number Diff line number Diff line change
@@ -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):

0 comments on commit 7e652e8

Please sign in to comment.