From 04a3ec36898bec20be6857606e804018ae5ccc5e Mon Sep 17 00:00:00 2001 From: Dylan Date: Mon, 17 Nov 2025 07:30:34 -0600 Subject: [PATCH] Adjust own-line comment placement between branches (#21185) 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, ```python 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: ```python 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: ```python 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: 1. On `main` our handling of `if` vs. `while` is not consistent, whereas it is consistent in the present PR 2. 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
--- .../test/fixtures/ruff/statement/if.py | 36 +++++++++ .../test/fixtures/ruff/statement/while.py | 34 ++++++++ .../ruff_python_formatter/src/comments/mod.rs | 29 +++++++ .../src/comments/placement.rs | 71 +++++++++++++++-- ...lse_indented_comment_between_branches.snap | 21 +++++ ...ery_indented_comment_between_branches.snap | 21 +++++ .../tests/snapshots/format@form_feed.py.snap | 3 +- .../snapshots/format@statement__if.py.snap | 79 ++++++++++++++++++- .../snapshots/format@statement__while.py.snap | 75 +++++++++++++++++- 9 files changed, 359 insertions(+), 10 deletions(-) create mode 100644 crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_indented_comment_between_branches.snap create mode 100644 crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_very_indented_comment_between_branches.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py index 063ef5fd96..a6afdc242f 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py @@ -294,3 +294,39 @@ if parent_body: # d # e #f + +# Compare behavior with `while`/`else` comment placement + +if True: pass + # 1 +else: + pass + +if True: + pass + # 2 +else: + pass + +if True: pass +# 3 +else: + pass + +if True: pass + # 4 +else: + pass + +def foo(): + if True: + pass +# 5 + else: + pass + +if True: + first;second + # 6 +else: + pass diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py index a09f26b550..ebe9922922 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py @@ -28,3 +28,37 @@ while ( and anotherCondition or aThirdCondition # trailing third condition ): # comment print("Do something") + +while True: pass + # 1 +else: + pass + +while True: + pass + # 2 +else: + pass + +while True: pass +# 3 +else: + pass + +while True: pass + # 4 +else: + pass + +def foo(): + while True: + pass +# 5 + else: + pass + +while True: + first;second + # 6 +else: + pass diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index 81a1b4bf25..07fc789f34 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -1042,4 +1042,33 @@ else: # trailing comment assert_debug_snapshot!(comments.debug(test_case.source_code)); } + + #[test] + fn while_else_indented_comment_between_branches() { + let source = r"while True: pass + # comment +else: + pass +"; + let test_case = CommentsTestCase::from_code(source); + + let comments = test_case.to_comments(); + + assert_debug_snapshot!(comments.debug(test_case.source_code)); + } + + #[test] + fn while_else_very_indented_comment_between_branches() { + let source = r"while True: + pass + # comment +else: + pass +"; + let test_case = CommentsTestCase::from_code(source); + + let comments = test_case.to_comments(); + + assert_debug_snapshot!(comments.debug(test_case.source_code)); + } } diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 234752ba53..2bd7402a31 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -8,7 +8,7 @@ use ruff_python_trivia::{ find_only_token_in_range, first_non_trivia_token, indentation_at_offset, }; use ruff_source_file::LineRanges; -use ruff_text_size::{Ranged, TextLen, TextRange}; +use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use std::cmp::Ordering; use crate::comments::visitor::{CommentPlacement, DecoratedComment}; @@ -602,9 +602,35 @@ fn handle_own_line_comment_between_branches<'a>( // following branch or if it a trailing comment of the previous body's last statement. let comment_indentation = comment_indentation_after(preceding, comment.range(), source); - let preceding_indentation = indentation(source, &preceding) - .unwrap_or_default() - .text_len(); + let preceding_indentation = indentation(source, &preceding).map_or_else( + // If `indentation` returns `None`, then there is leading + // content before the preceding node. In this case, we + // always treat the comment as being less-indented than the + // preceding. For example: + // + // ```python + // if True: pass + // # leading on `else` + // else: + // pass + // ``` + // Note we even do this if the comment is very indented + // (which matches `black`'s behavior as of 2025.11.11) + // + // ```python + // if True: pass + // # leading on `else` + // else: + // pass + // ``` + || { + comment_indentation + // This can be any positive number - we just + // want to hit the `Less` branch below + + TextSize::new(1) + }, + ruff_text_size::TextLen::text_len, + ); // Compare to the last statement in the body match comment_indentation.cmp(&preceding_indentation) { @@ -678,8 +704,41 @@ fn handle_own_line_comment_after_branch<'a>( preceding: AnyNodeRef<'a>, source: &str, ) -> CommentPlacement<'a> { - let Some(last_child) = preceding.last_child_in_body() else { - return CommentPlacement::Default(comment); + // If the preceding node has a body, we want the last child - e.g. + // + // ```python + // if True: + // def foo(): + // something + // last_child + // # comment + // else: + // pass + // ``` + // + // Otherwise, the preceding node may be the last statement in the body + // of the preceding branch, in which case we can take it as our + // `last_child` here - e.g. + // + // ```python + // if True: + // something + // last_child + // # comment + // else: + // pass + // ``` + let last_child = match preceding.last_child_in_body() { + Some(last) => last, + None if comment.following_node().is_some_and(|following| { + following.is_first_statement_in_alternate_body(comment.enclosing_node()) + }) => + { + preceding + } + _ => { + return CommentPlacement::Default(comment); + } }; // We only care about the length because indentations with mixed spaces and tabs are only valid if diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_indented_comment_between_branches.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_indented_comment_between_branches.snap new file mode 100644 index 0000000000..685f235695 --- /dev/null +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_indented_comment_between_branches.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff_python_formatter/src/comments/mod.rs +expression: comments.debug(test_case.source_code) +--- +{ + Node { + kind: StmtWhile, + range: 0..45, + source: `while True: pass⏎`, + }: { + "leading": [], + "dangling": [ + SourceComment { + text: "# comment", + position: OwnLine, + formatted: false, + }, + ], + "trailing": [], + }, +} diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_very_indented_comment_between_branches.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_very_indented_comment_between_branches.snap new file mode 100644 index 0000000000..7daeba56d1 --- /dev/null +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_very_indented_comment_between_branches.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff_python_formatter/src/comments/mod.rs +expression: comments.debug(test_case.source_code) +--- +{ + Node { + kind: StmtPass, + range: 16..20, + source: `pass`, + }: { + "leading": [], + "dangling": [], + "trailing": [ + SourceComment { + text: "# comment", + position: OwnLine, + formatted: false, + }, + ], + }, +} diff --git a/crates/ruff_python_formatter/tests/snapshots/format@form_feed.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@form_feed.py.snap index a461554f7d..9539965e42 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@form_feed.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@form_feed.py.snap @@ -1,7 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/form_feed.py -snapshot_kind: text --- ## Input ```python @@ -18,7 +17,7 @@ else: # Regression test for: https://github.com/astral-sh/ruff/issues/7624 if symbol is not None: request["market"] = market["id"] -# "remaining_volume": "0.0", + # "remaining_volume": "0.0", else: pass ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap index 7becf78515..4c561560bb 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap @@ -1,7 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py -snapshot_kind: text --- ## Input ```python @@ -301,6 +300,42 @@ if parent_body: # d # e #f + +# Compare behavior with `while`/`else` comment placement + +if True: pass + # 1 +else: + pass + +if True: + pass + # 2 +else: + pass + +if True: pass +# 3 +else: + pass + +if True: pass + # 4 +else: + pass + +def foo(): + if True: + pass +# 5 + else: + pass + +if True: + first;second + # 6 +else: + pass ``` ## Output @@ -607,6 +642,48 @@ if parent_body: # d # e # f + +# Compare behavior with `while`/`else` comment placement + +if True: + pass +# 1 +else: + pass + +if True: + pass + # 2 +else: + pass + +if True: + pass +# 3 +else: + pass + +if True: + pass +# 4 +else: + pass + + +def foo(): + if True: + pass + # 5 + else: + pass + + +if True: + first + second +# 6 +else: + pass ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap index 6217656dda..c5b78fadbd 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__while.py.snap @@ -1,7 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py -snapshot_kind: text --- ## Input ```python @@ -35,6 +34,40 @@ while ( and anotherCondition or aThirdCondition # trailing third condition ): # comment print("Do something") + +while True: pass + # 1 +else: + pass + +while True: + pass + # 2 +else: + pass + +while True: pass +# 3 +else: + pass + +while True: pass + # 4 +else: + pass + +def foo(): + while True: + pass +# 5 + else: + pass + +while True: + first;second + # 6 +else: + pass ``` ## Output @@ -70,4 +103,44 @@ while ( or aThirdCondition # trailing third condition ): # comment print("Do something") + +while True: + pass +# 1 +else: + pass + +while True: + pass + # 2 +else: + pass + +while True: + pass +# 3 +else: + pass + +while True: + pass +# 4 +else: + pass + + +def foo(): + while True: + pass + # 5 + else: + pass + + +while True: + first + second +# 6 +else: + pass ```