[ruff] Fix RUF033 breaking with named default expressions

[robsdedude]

Summary

The generated fix for RUF033 would cause a syntax error for named expressions as parameter defaults.

from dataclasses import InitVar, dataclass
@dataclass
class Foo:
    def __post_init__(self, bar: int = (x := 1)) -> None:
        pass

would be turned into

from dataclasses import InitVar, dataclass
@dataclass
class Foo:
    x: InitVar[int] = x := 1
    def __post_init__(self, bar: int = (x := 1)) -> None:
        pass

instead of the syntactically correct

# ...
x: InitVar[int] = (x := 1)
# ...

Test Plan

Test reproducer (plus some extra tests) have been added to the test suite.

Related

Fixes: #18950

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[ntBre]

Thanks! I think the results look right, but I have a couple of suggestions for the implementation.

[ntBre]

I don't think we need to switch all the way to using the code generator here. Can't we just check if the expression is_expr_named and add parens if so? Or I think we could even check the parenthesized_range and preserve the parens it came with.

It's nice to prefer range replacements because the generator strips out any comments, as well as being much more verbose.

[robsdedude]

I managed to rewrite it using parenthesized_range. It's passing the same tests. But if feels crazy fragile. Looking at the code I cannot convince myself of its correctness and that I haven't missed any edge case. I don't feel comfortable with my name being attached to that code. Sorry.

[ntBre]

If you feel that strongly about it, we can keep the old implementation. I don't really see how it's more fragile, and we have similar checks in other rules, but this was only a slight preference on my part, not a blocker. I'd much rather fix the bug and give you credit for your work here. I'll approve and merge this if you want to revert/drop the last commit and reopen the PR.

[robsdedude]

Where I think this is fragile: if the Syntax ever changes, string replacements like these bake in too many assumptions. That's how this bug happened in the first place: a new syntax construct (walrus operator) was introduced and the replacement logic wasn't up to the task.

Moreover, parenthesized_range calls parentheses_iterator, which in turn is documented with

/// Returns an iterator over the ranges of the optional parentheses surrounding an expression.

Yeah no... these parens are surely not optional 😬 It's quite unfortunate, I find, that the AST does not always include the parens where they're necessary in the expression's range. But neither does Python's own ast module 🤷

I slept a night over it and I'll re-open the PR. If not, I would've given you permission to take my code and commit it under your name. So regardless of how this ends some fix can be merged.

[ntBre]

Where I think this is fragile: if the Syntax ever changes, string replacements like these bake in too many assumptions. That's how this bug happened in the first place: a new syntax construct (walrus operator) was introduced and the replacement logic wasn't up to the task.

Ahh, I see where you're coming from. I think that explains the other assertion too. I guess I wasn't really thinking about it in the same way since the walrus operator and Python 3.8 predate Ruff by ~3 years. So it wasn't so much that something new came out and broke us, just that the initial rule implementation didn't account for everything that already existed.

Yeah no... these parens are surely not optional

I can also see what you mean here, but I always read this as referring to a Rust Option, like the return type of parenthesized_range, as in the expression may or may not have parentheses rather than may or may not require parentheses.

Anyway, I'm happy with either implementation. I'll give this another quick review now.

[ntBre]

Thanks again! Just one nit about a test case name, but this is otherwise good to go from my side.

[ntBre]

Thank you! I'll merge after the patch release finishes.