Repository navigation
[perflint] Avoid invalid comprehension suggestions (PERF401, PERF403) - #29366
Conversation
…`PERF401`, `PERF403`) The fixes copied the loop iterable into the comprehension as-is, adding parentheses only for an implicit tuple. A conditional expression or lambda iterable produced invalid code, so the fix was reverted with a syntax error. Iterables containing an assignment expression can't be moved into a comprehension at all, so no fix is offered for them. Fixes astral-sh#29365
|
| let for_iter_str = match &*for_stmt.iter { | ||
| Expr::Tuple(ast::ExprTuple { | ||
| parenthesized: false, | ||
| .. | ||
| }) | ||
| | Expr::Lambda(_) | ||
| | Expr::If(_) => format!("({})", locator.slice(for_stmt.iter.range())), | ||
| _ => locator.slice(for_stmt.iter.range()).to_string(), |
There was a problem hiding this comment.
Could we use this helper here and in the other rule?
There was a problem hiding this comment.
Switched both rules to the refurb helper and made it pub(crate). Existing snapshots didn't change. It did start duplicating a comment inside the iterable's parentheses, so I now ignore the parenthesized range when collecting comments. That's in the same commit.
| // if the loop target was an implicit tuple, add parentheses around it | ||
| // A comprehension's iterable can't be an unparenthesized tuple, lambda, or conditional | ||
| // expression, so add parentheses around it. Parentheses in the original source (e.g., | ||
| // `for x in (a if b else c)`) are outside the expression's range, so they're added back. |
There was a problem hiding this comment.
Assuming we use the helper, we'll need to adjust the comments again too.
There was a problem hiding this comment.
Removed them, the helper's doc comment already covers this.
| return Err(anyhow!( | ||
| "Cannot move an assignment expression into a comprehension iterable" | ||
| )); |
There was a problem hiding this comment.
This doesn't seem like the right place for this check. Returning an Err means this will be logged by diagnostic.try_set_fix, which I think makes more sense for unexpected errors than control flow. We should either return a Result<Option<Fix>> or perform this check before calling this function. The latter seems fine here and a bit easier.
There was a problem hiding this comment.
Moved the walrus check before the fix, so there's no Err anymore. For PERF403 I left it as is, since convert_to_dict_comprehension already returns Option<Fix>. Can move it up too if you prefer it consistent.
ntBre
added
the
bug
Use `parenthesize_loop_iter_if_necessary` from `refurb` to parenthesize the loop iterable in `PERF401` and `PERF403` instead of matching on the expression kind in each rule, and drop the comments that described that logic. The helper is made `pub(crate)` so that it can be shared. Because the helper keeps any parentheses around the iterable, and the comments inside them, exclude the parenthesized range when collecting the comments to move above the replaced statement. Otherwise such a comment was kept in place and also duplicated above the statement. For `PERF401`, check for an assignment expression in the iterable before building the fix rather than returning an `Err` from the fix function, since `try_set_fix` logs errors and this is an expected case, not a failure. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I pushed a couple of commits returning the range from the helper to avoid having to compute it again in each rule and also returning without a diagnostic rather than without a fix when a named expression is used in the comprehension iterable. If we can't offer an autofix, it seems like the user won't really be able to make a manual fix either, at least without more significantly changing the structure of their code.
ntBre
changed the title
perflint] Parenthesize conditional and lambda iterables in fixes (PERF401, PERF403)perflint] Avoid invalid comprehension suggestions (PERF401, PERF403)
Brings the fork from upstream main at 9520164 (after 0.16.10) to 9352615, the merge of astral-sh#29366, which stops PERF401 and PERF403 from suggesting invalid comprehensions: the 0.17.0 release plus the 64 commits after it, 83 upstream commits in all. Conflicts were limited to the version lines: Cargo.toml and pyproject.toml take upstream's versions, keeping the `=` pin on ruff_linter and the ruff-odoo package metadata, and uv.lock is upstream's relocked with uv 0.13.1. The fork's release counter is kept, so 0.16.10.35 becomes 0.17.0.35 in .bumpversion.cfg, the READMEs and the wrapper crate.
Fixes #29365.
When the loop iterable was a ternary (or a lambda), the PERF401/PERF403 fix pasted it into the comprehension as-is, which isn't valid Python, so Ruff rolled the fix back. Now the iterable gets parenthesized in those cases. If the iterable contains a walrus, I just skip the fix and keep the diagnostic, since that can't go in a comprehension iterable even with parens.
Test plan: added the examples from the issue plus ternary, lambda and walrus cases to the PERF401/PERF403 fixtures and updated the snapshots.
cargo test -p ruff_linter perflintpasses, and the repro from the issue now fixes cleanly.I used Claude to help with this change and reviewed and tested it myself.