Fix panic when formatting comments in unary expressions #21501

ntBre · 2025-11-17T16:15:19Z

Summary

This is another attempt at #21410 that fixes #19226.

@MichaReiser helped me get something working in a very helpful pairing session. I pushed one additional commit moving the comments back from leading comments to trailing comments, which I think retains more of the input formatting.

I was inspired by Dylan's PR (#21185) to make one of these tables:

Input

Main

PR

if (
    not
    # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (
    # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (
    not
    # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

if (
    # unary comment
    # operand comment
    not (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

if (
    not # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (  # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa  # comment
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

hopefully it helps even though the snippets are much wider here.

The two main differences are (1) that we now retain own-line comments between the unary operator and its operand instead of moving these to leading comments on the operator itself, and (2) that we move end-of-line comments between the operator and operand to dangling end-of-line comments on the operand (the last example in the table).

Test Plan

Existing tests, plus new ones based on the issue. As I noted below, I also ran the output from main on the unary.py file back through this branch to check that we don't reformat code from main. This made me feel a bit better about not preview-gating the changes in this PR.

> git show main:crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py | ruff format - | ./target/debug/ruff format --diff -
> echo $?
0

see #21410 (comment), but the short summary is that `if` (and likely other) statement formatting code that uses `maybe_parenthesize` checks if the condition has any leading or trailing comments, so if we try to smuggle the comments in as dangling comments, it thinks the expression won't break, so it doesn't add parentheses when formatting a case like this: ```py if ( not # comment a): pass ``` and we end up with a syntax error: ```py if not a: pass ``` There may be some other way around this, but this is why I'm giving up for now. It really feels like we want another CommentPlacement variant or some kind of dangling tag, like Micha mentioned when I met with him.

this is very close, but now I have an extra newline in a few cases

missing the leading comments is causing the whole thing to get indented deep in the if formatting

Co-authored-by: Micha Reiser <micha@reiser.io>

astral-sh-bot · 2025-11-17T16:22:57Z

`ruff-ecosystem` results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

...python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap

MichaReiser

I haven't thought it through but does this change require preview gating?

I think what will help us to make this decision is if you update your summary and describe how the formatting, specifically the comment placement, changes compared to main.

MichaReiser · 2025-11-17T16:32:07Z

crates/ruff_python_formatter/src/comments/placement.rs

-        .map_or(unary_op.operand.start(), |lparen| lparen.start());
-    if comment.end() < up_to {
-        CommentPlacement::leading(unary_op, comment)
+    let up_to = operand_start(unary_op, source);


Can you update the method description to match our new behavior

MichaReiser · 2025-11-17T16:32:48Z

crates/ruff_python_formatter/src/expression/expr_unary_op.rs

+                .iter()
+                .any(|comment| comment.start() < range.start())
+        });
        if comments.has_leading(operand.as_ref())


Let's assign the leading commnts to a variable to avoid retrieving them twice

MichaReiser · 2025-11-17T16:33:40Z

crates/ruff_python_formatter/src/expression/expr_unary_op.rs

        ) {
-            OptionalParentheses::Never
-        } else if context.comments().has(self.operand.as_ref()) {
+            return OptionalParentheses::Never;


Do we need to change the logic here too to match the logic for when we insert a hard line break in the unary formatting?

Do you mean something like this?

if !context.comments().has_leading(self.operand.as_ref()) || is_expression_parenthesized( self.operand.as_ref().into(), context.comments().ranges(), context.source(), ) { return OptionalParentheses::Never; }

I played with a few variations on this and kept running into instabilities. It seems to be working okay without matching the check exactly, like on main.

No, more like this:

let parenthesized_operand_range = parenthesized_range( operand.into(), item.into(), comments.ranges(), f.context().source(), ); let leading_operand_comments = comments.leading(operand.as_ref()); let has_leading_comments_before_parens = parenthesized_operand_range.is_some_and(|range| { leading_operand_comments .iter() .any(|comment| comment.start() < range.start()) }); if !leading_operand_comments.is_empty() && !is_expression_parenthesized( operand.as_ref().into(), f.context().comments().ranges(), f.context().source(), ) || has_leading_comments_before_parens

It's important that it exactly mirrors the case when we insert a hard line break in the formatting code because any line break will lead to invalid syntax if the if formatting doesn't add parentheses.

Here's an example where your PR produces invalid syntax:

if ( not # comment (a)): pass

We should add more tests that exercise the new leading comment placement (may even be true for the trailing comment placement, are there more combinations that you could test?)

MichaReiser · 2025-11-17T16:35:28Z

crates/ruff_python_formatter/src/expression/expr_unary_op.rs

+        let operand_start = operand_start(self, context.source());
+        if context
+            .comments()
+            .dangling(self)
+            .iter()
+            .any(|comment| comment.end() < operand_start)


I think we can simplify this to returning Multiline when there's any dangling comment.

Does this need to take precedence over the Never case when the operand is parenthesized?

It seems to work in both orders, at least with our current tests.

ntBre · 2025-11-17T17:09:41Z

I haven't thought it through but does this change require preview gating?

I think what will help us to make this decision is if you update your summary and describe how the formatting, specifically the comment placement, changes compared to main.

I did think a little bit about this. I think it would be a bit difficult to preview-gate this since the panic fix and the new formatting seem intertwined.

I verified that taking the formatted output from main (the ## Output section from the unary.py snapshot) and running it against this branch shows no changes. So we won't change any code that we previously formatted, at least.

I will also expand the summary, though!

operand_start was now used in only one place again, and the intermediate variable in NeedsParentheses was no longer needed

MichaReiser · 2025-11-18T09:43:58Z

Thanks for updating the summary. I think it should be safe to not preview gate this change because:

It changes the placement of trailing operator comments, but main always moved trailing operator comments off the operator.
It changes the placement of leading operator comments, but main always made leading operator comments leading unary comments.

That means, any ruff formatted code can't contain any comment for which we now preserve the position

MichaReiser

Thank you

MichaReiser · 2025-11-18T15:08:53Z

crates/ruff_python_formatter/src/expression/expr_unary_op.rs

+        }
+
+        if needs_line_break(self, context) {
+            return OptionalParentheses::Multiline;


I think we can even return Always here because we know it breaks over multiple lines and will need parentheses

Ah right, thanks! And thanks for all of your help here!

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

ntBre and others added 9 commits November 17, 2025 10:39

looking very reasonable

e7a838a

grab the tests from the other PR, update needs_parentheses

7b1da06

this is very close, but now I have an extra newline in a few cases

remove commented code

d9486aa

heavy debugging, I think I found the root cause

e4dc4b2

missing the leading comments is causing the whole thing to get indented deep in the if formatting

revert debugging code, get something working?

2d9b3fe

Co-authored-by: Micha Reiser <micha@reiser.io>

update comments and range variable name

c7fc121

avoid computing operand_start for each dangling comment

60317c8

try dangling as trailing

7e193df

ntBre added bug Something isn't working formatter Related to the formatter labels Nov 17, 2025

ntBre commented Nov 17, 2025

View reviewed changes

...python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap Show resolved Hide resolved

ntBre marked this pull request as ready for review November 17, 2025 16:28

ntBre requested a review from MichaReiser as a code owner November 17, 2025 16:28

MichaReiser reviewed Nov 17, 2025

View reviewed changes

ntBre added 3 commits November 17, 2025 11:46

update handle_unary_op_comment docs

31d43c5

reuse leading_operand_comments

a50e006

simplify has_dangling check, move it before Never case

bd612c4

revert some now-unnecessary changes

91cb799

operand_start was now used in only one place again, and the intermediate variable in NeedsParentheses was no longer needed

ntBre requested a review from MichaReiser November 17, 2025 19:55

ntBre added 3 commits November 18, 2025 08:46

convert back to guarded returns

c0d4d91

factor out needs_line_break, fix parenthesize bug

58cd40f

add a few more tests

d89e1c2

MichaReiser approved these changes Nov 18, 2025

View reviewed changes

ntBre and others added 2 commits November 18, 2025 10:29

needs_line_break => OptionalParentheses::Always

9d86a5c

add co-author

9e16151

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

ntBre merged commit cbc6863 into main Nov 18, 2025
37 checks passed

ntBre deleted the brent/unary-3 branch November 18, 2025 15:48

This was referenced Nov 18, 2025

Fix panic when formatting comments in unary expressions #21410

Closed

[ruff] Fix panic when comments appear between unary operators and operands #20494

Closed

Ruff 2026 Style Guide #20482

Open

Fix panic when formatting comments in unary expressions #21501

Fix panic when formatting comments in unary expressions #21501

Conversation

ntBre commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

astral-sh-bot bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Formatter (stable)

Formatter (preview)

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Nov 17, 2025

Uh oh!

MichaReiser commented Nov 18, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ntBre commented Nov 17, 2025 •

edited

Loading

astral-sh-bot bot commented Nov 17, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser Nov 18, 2025 •

edited

Loading