Adjust own-line comment placement between branches #21185

dylwil3 · 2025-11-01T16:10:56Z

This PR attempts to improve the placement of own-line comments between branches in the setting where the comment is more indented than the preceding node.

There are two main changes.

First change: Preceding node has leading content

If the preceding node has leading content, we now regard the comment as automatically less indented than the preceding node, and format accordingly.

For example,

if True: preceding_node
# leading on `else`, not trailing on `preceding_node`
else: ...

This is more compatible with black, although there is a (presumably very uncommon) edge case:

if True:
    this;that
    # leading on `else`, but trailing in `black`
else: ...

I'm sort of okay with this - presumably if one wanted a comment for those semi-colon separated statements, one should have put it above them, and one wanted a comment only for that then it ought to have been on the same line?

Second change: searching for last child in body

While searching for the (recursively) last child in the body of the preceding branch, we implicitly assumed that the preceding node had to have a body to begin the recursion. But actually, in the base case, the preceding node is the last child in the body of the preceding branch. So, for example:

if True:
    something
    last_child_but_no_body
    # leading on else for `main` but trailing in this PR
else: ...

More examples

The table below is an attempt to summarize the changes in behavior. The rows alternate between an example snippet with while and the same example with if - in the former case we do not have an else node and in the latter we do.

Notice that:

On main our handling of if vs. while is not consistent, whereas it is consistent in the present PR
We disagree with black in all cases except that last example on main, but agree in all cases for the present PR (though see above for a wonky edge case where we disagree).

Original	`main`	This PR	`black`
while True: pass # comment else: pass	while True: pass else: # comment pass	while True: pass # comment else: pass	while True: pass # comment else: pass
if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass
while True: pass # comment else: pass	while True: pass # comment else: pass	while True: pass # comment else: pass	while True: pass # comment else: pass
if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass
while True: pass # comment else: pass	while True: pass else: # comment pass	while True: pass # comment else: pass	while True: pass # comment else: pass
if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass	if True: pass # comment else: pass

github-actions · 2025-11-01T16:22:13Z

`ruff-ecosystem` results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

crates/ruff_python_formatter/src/comments/placement.rs

MichaReiser

This looks much better to me and it's nice that this matches black in almost all cases.

However, there are a couple of tests that now fail because of instable formatting (the invariant format(source) == format(format(source)) doesn't hold anymore). I think it's because the fallback to preceding is too liberal.

crates/ruff_python_formatter/src/comments/placement.rs

MichaReiser · 2025-11-11T18:04:10Z

crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap

    print("Do something")
+
+while True: pass
+    # comment


I often find it useful to number the comments. It makes it easier to match the input/output (and debug panics if we forgot to format a comment)

Here I didn't do that because there is only one comment in each example - do you want me to the number them across the whole fixture? Among these examples? Or just make them all #1?

I would give them unique numbers within the # Compare behavior with while/else comment placement section

MichaReiser · 2025-11-11T18:08:34Z

crates/ruff_python_formatter/src/comments/placement.rs

+    // else:
+    //     pass
+    // ```
+    let last_child = preceding.last_child_in_body().unwrap_or(preceding);


This seems risky. There isn't really anything enforcing that we're within a block statement. I think we, at least, want to assert that the enclosing node is a block statement.

dylwil3 · 2025-11-12T00:25:56Z

Sorry about the failing tests! I made the beginner mistake of running tests, fixing snapshots, but then not re-running tests after that. Whoops!

MichaReiser

Thank you. This overall looks good. I just don't feel 100 confident about the new Non if matches case. Did you pick them individually or did you list all compound statements just to be sure? If it's the latter, than I'd prefer to only pick the once that are indeed necessary.

MichaReiser · 2025-11-15T18:13:57Z

crates/ruff_python_formatter/src/comments/placement.rs

+        None if matches!(
+            comment.enclosing_node(),
+            AnyNodeRef::StmtIf(_)
+                | AnyNodeRef::StmtWhile(_)
+                | AnyNodeRef::StmtFor(_)
+                | AnyNodeRef::StmtMatch(_)
+                | AnyNodeRef::ElifElseClause(_)
+                | AnyNodeRef::StmtTry(_)
+                | AnyNodeRef::MatchCase(_)
+                | AnyNodeRef::ExceptHandlerExceptHandler(_)
+        ) =>


Could we use if following.is_first_statement_in_body() or even is_first_statement_in_alternate_body

I'm surprised that we need MatchCase and ExceptHandler here, as those nodes don't have alternate branches? How did you pick the nodes listed here? Did you add tests demonstrating that they're all necessary?

Yes, sorry - I added all of those because that was my interpretation of

There isn't really anything enforcing that we're within a block statement. I think we, at least, want to assert that the enclosing node is a block statement.

But you're right that is_first_statement_in_alternate_body is more targeted, so I've changed it to that, thanks!

MichaReiser · 2025-11-15T18:16:13Z

crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap

    print("Do something")
+
+while True: pass
+    # comment


I would give them unique numbers within the # Compare behavior with while/else comment placement section

* origin/main: (26 commits) Mention `force-exclude` in "Configuration > Python file discovery" (#21500) Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418) [ty] suppress invalid suggestions in import statements (#21484) Limit `eglot-format` hook to eglot-managed Python buffers (#21459) Adjust own-line comment placement between branches (#21185) [ty] Subscript assignment diagnostics follow-up (#21452) [ty] Inlay hint call argument location (#20349) [ty] Use `CompactStr` for `StringLiteralType` (#21497) Update CodSpeedHQ/action action to v4.3.4 (#21488) Update salsa digest to a885bb4 (#21486) Update dependency ruff to v0.14.5 (#21489) Update astral-sh/setup-uv action to v7.1.3 (#21487) Update Rust crate get-size2 to v0.7.2 (#21490) Update Rust crate indicatif to v0.18.3 (#21491) Update Rust crate quick-junit to v0.5.2 (#21492) Update taiki-e/install-action action to v2.62.52 (#21493) Fix analyze graph tests on windows (#21481) `analyze`: Add option to skip over imports in `TYPE_CHECKING` blocks (#21472) [ty] Dataclasses: `__hash__` semantics and `unsafe_hash` (#21470) [ty] Dataclass transform: complete set of parameters (#21474) ...

* dcreager/coolable: (31 commits) mdformat don't panic Mention `force-exclude` in "Configuration > Python file discovery" (#21500) Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418) [ty] suppress invalid suggestions in import statements (#21484) known discrepancy TODO α-rename todo equiv too Limit `eglot-format` hook to eglot-managed Python buffers (#21459) Adjust own-line comment placement between branches (#21185) [ty] Subscript assignment diagnostics follow-up (#21452) [ty] Inlay hint call argument location (#20349) [ty] Use `CompactStr` for `StringLiteralType` (#21497) Update CodSpeedHQ/action action to v4.3.4 (#21488) Update salsa digest to a885bb4 (#21486) Update dependency ruff to v0.14.5 (#21489) Update astral-sh/setup-uv action to v7.1.3 (#21487) Update Rust crate get-size2 to v0.7.2 (#21490) Update Rust crate indicatif to v0.18.3 (#21491) Update Rust crate quick-junit to v0.5.2 (#21492) ...

@MichaReiser

## 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: <table> <thead> <tr> <th scope="col">Input</th> <th scope="col">Main</th> <th scope="col">PR</th> </tr> </thead> <tbody> <tr> <td><pre lang="python"> if ( not # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> <td><pre lang="python"> if ( # comment not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> <td><pre lang="python"> if ( not # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> </tr> <tr> <td><pre lang="python"> if ( # unary comment not # operand comment ( # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ) ): pass </pre></td> <td><pre lang="python"> if ( # unary comment # operand comment not ( # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ) ): pass </pre></td> <td><pre lang="python"> if ( # unary comment not # operand comment ( # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ) ): pass </pre></td> </tr> <tr> <td><pre lang="python"> if ( not # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> <td><pre lang="python"> if ( # comment not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> <td><pre lang="python"> if ( not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass </pre></td> </tr> </tbody> </table> 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. ```shell > git show main:crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py | ruff format - | ./target/debug/ruff format --diff - > echo $? 0 ``` --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

dylwil3 added bug Something isn't working formatter Related to the formatter labels Nov 1, 2025

dylwil3 force-pushed the comment-placement-else-finally branch from c50d6c3 to 0e3bdc1 Compare November 1, 2025 17:04

dylwil3 force-pushed the comment-placement-else-finally branch from 0e3bdc1 to 3aa47b8 Compare November 10, 2025 15:50

dylwil3 changed the title ~~Adjust comment placement before else/finally clauses~~ Adjust indented comment placement before else/finally clauses Nov 10, 2025

dylwil3 added 4 commits November 10, 2025 15:26

add fixtures

d5a113b

record current behavior in tests

37dcd5d

handle non-node branches in comments in between statements

6b489cf

record new behavior

394793e

dylwil3 force-pushed the comment-placement-else-finally branch from 3aa47b8 to 394793e Compare November 10, 2025 21:29

dylwil3 commented Nov 10, 2025

View reviewed changes

crates/ruff_python_formatter/src/comments/placement.rs Outdated Show resolved Hide resolved

dylwil3 marked this pull request as ready for review November 10, 2025 21:51

dylwil3 requested a review from MichaReiser as a code owner November 10, 2025 21:51

MichaReiser reviewed Nov 11, 2025

View reviewed changes

crates/ruff_python_formatter/src/comments/placement.rs Outdated Show resolved Hide resolved

dylwil3 added 5 commits November 11, 2025 10:18

undo first attempt fix

a70472d

take preceding as last_child if no body

22506b6

regard preceding with leading content as more indented

6231301

add one more fixture

d72f846

update snapshots

9199a85

dylwil3 changed the title ~~Adjust indented comment placement before else/finally clauses~~ Adjust comment placement between branches Nov 11, 2025

dylwil3 changed the title ~~Adjust comment placement between branches~~ Adjust own-line comment placement between branches Nov 11, 2025

clippy

f7fc873

MichaReiser requested changes Nov 11, 2025

View reviewed changes

dylwil3 added 2 commits November 11, 2025 14:40

clarify why we are adding TextSize(1)

0251bd1

less aggressively use preceding node as last_child

8b18899

dylwil3 requested a review from MichaReiser November 14, 2025 18:12

MichaReiser approved these changes Nov 15, 2025

View reviewed changes

narrow cases when we use preceding

e8332fb

number comments

839e40e

dylwil3 merged commit 04a3ec3 into astral-sh:main Nov 17, 2025
37 checks passed

ntBre mentioned this pull request Nov 17, 2025

Fix panic when formatting comments in unary expressions #21501

Merged

Adjust own-line comment placement between branches #21185

Adjust own-line comment placement between branches #21185

Uh oh!

Conversation

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

Uh oh!

First change: Preceding node has leading content

Second change: searching for last child in body

More examples

Uh oh!

github-actions bot commented Nov 1, 2025 • edited by astral-sh-bot bot 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!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaReiser Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

dylwil3 Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

dylwil3 commented Nov 12, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

dylwil3 Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dylwil3 commented Nov 1, 2025 •

edited

Loading

github-actions bot commented Nov 1, 2025 •

edited by astral-sh-bot bot

Loading

`ruff-ecosystem` results