[flake8-comprehensions] Unnecessary list comprehension (rewrite as a set comprehension) (C403) - Handle extraneous parentheses around list comprehension#15877
Conversation
…s a `set` comprehension) (`C403`) - Handle extraneous parentheses around list comprehension
|
ntBre
left a comment
There was a problem hiding this comment.
Thanks for working on this! I added some suggestions, but I think the approach with parenthesized_range is perfect.
| let generator = checker.generator().expr(argument); | ||
| let generator = generator[1..generator.len() - 1].to_string(); | ||
| let replacement = Edit::range_replacement(generator, range); |
There was a problem hiding this comment.
It might be nice to avoid using the generator here to preserve comments in the comprehension like the non-parenthesized code. For example,
s = set(# outer set comment
( # inner paren comment - not preserved
(([ # comment in the comprehension
x for x in range(3)]))))We'll still lose the comment in the inner parentheses, but that seems like the weirdest place to put a comment of the three. It might be good to add a test like this with the comments and also one with multiple levels of parentheses like:
s = set(((([x for x in range(3)]))))I started to suggest an approach that would not have worked with multiple parentheses, but your approach handles that nicely, so it would be good to capture that in a test.
Back to avoiding the generator, you can slice the original source text (instead of round-tripping through the generator like checker.generator().expr(argument)) with code like checker.source()[range].to_string(). After that change, the new code started to look very similar to the old code, so I ended up with something like this. What do you think?
diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs
index c518a8b11..cd7d0901a 100644
--- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs
+++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs
@@ -59,44 +59,37 @@ pub(crate) fn unnecessary_list_comprehension_set(checker: &mut Checker, call: &a
if argument.is_list_comp_expr() {
let diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, call.range());
let fix = {
+ let one = TextSize::from(1);
+
// Replace `set(` with `{`.
let call_start = Edit::replacement(
pad_start("{", call.range(), checker.locator(), checker.semantic()),
call.start(),
- call.arguments.start() + TextSize::from(1),
+ call.arguments.start() + one,
);
// Replace `)` with `}`.
let call_end = Edit::replacement(
pad_end("}", call.range(), checker.locator(), checker.semantic()),
- call.arguments.end() - TextSize::from(1),
+ call.arguments.end() - one,
call.end(),
);
- // If the list comprehension is parenthesized, remove the parentheses in addition to
- // removing the brackets.
- if let Some(range) = parenthesized_range(
+ // If the list comprehension is parenthesized, replace the whole parenthesized range.
+ let replacement_range = parenthesized_range(
argument.into(),
(&call.arguments).into(),
checker.comment_ranges(),
checker.locator().contents(),
- ) {
- // The generator produces the brackets that need to be removed
- let generator = checker.generator().expr(argument);
- let generator = generator[1..generator.len() - 1].to_string();
- let replacement = Edit::range_replacement(generator, range);
- Fix::unsafe_edits(call_start, [call_end, replacement])
- } else {
- // Delete the open bracket (`[`).
- let argument_start =
- Edit::deletion(argument.start(), argument.start() + TextSize::from(1));
+ )
+ .unwrap_or_else(|| argument.range());
- // Delete the close bracket (`]`).
- let argument_end =
- Edit::deletion(argument.end() - TextSize::from(1), argument.end());
+ // remove the leading `[` and trailing `]`
+ let span = argument.range().add_start(one).sub_end(one);
+ let replacement =
+ Edit::range_replacement(checker.source()[span].to_string(), replacement_range);
- Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end])
- }
+ Fix::unsafe_edits(call_start, [call_end, replacement])
};
checker.diagnostics.push(diagnostic.with_fix(fix));
}
There was a problem hiding this comment.
This isn't related to your changes, but while we're here, I'd probably slightly prefer to make
into an early return to avoid a level of nesting, and similarly move the code out of the fix = { block. The last line of the unwrapped block could just be let fix = Fix::unsafe_edits(call_start, [call_end, replacement]);.
There was a problem hiding this comment.
Fantastic! Thank you. I didn't know about slicing the source like that. Makes things a lot simpler.
There was a problem hiding this comment.
No problem, I just learned about that trick myself!
…nerator to slicing the source, simplifying the code.
ntBre
left a comment
There was a problem hiding this comment.
LGTM! Thanks again for your work on this!
* main: (66 commits) [red-knot] Use ternary decision diagrams (TDDs) for visibility constraints (#15861) [`pyupgrade`] Rename private type parameters in PEP 695 generics (`UP049`) (#15862) Simplify the `StringFlags` trait (#15944) [`flake8-pyi`] Make `PYI019` autofixable for `.py` files in preview mode as well as stubs (#15889) Docs (`linter.md`): clarify that Python files are always searched for in subdirectories (#15882) [`flake8-pyi`] Make PEP-695 functions with multiple type parameters fixable by PYI019 again (#15938) [red-knot] Use unambiguous invalid-syntax-construct for suppression comment test (#15933) Make `Binding::range()` point to the range of a type parameter's name, not the full type parameter (#15935) Update black deviations (#15928) [red-knot] MDTest: Fix line numbers in error messages (#15932) Preserve triple quotes and prefixes for strings (#15818) [red-knot] Hand-written MDTest parser (#15926) [`pylint`] Fix missing parens in unsafe fix for `unnecessary-dunder-call` (`PLC2801`) (#15762) nit: docs for ignore & select (#15883) [airflow] `BashOperator` has been moved to `airflow.providers.standard.operators.bash.BashOperator` (AIR302) (#15922) [`flake8-logging`] `.exception()` and `exc_info=` outside exception handlers (`LOG004`, `LOG014`) (#15799) [red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line (#15890) [red-knot] Internal refactoring of visibility constraints API (#15913) [red-knot] Implicit instance attributes (#15811) [`flake8-comprehensions`] Handle extraneous parentheses around list comprehension (`C403`) (#15877) ...
Summary
Given the following code:
the current implementation of C403 results in
{(x for x in range(5))}which is a set containing a generator rather than the result of the generator.
This change removes the extraneous parentheses so that the resulting code is:
{x for x in range(5)}Test Plan
cargo nextest runandcargo insta test