Skip to content

Commit 04a3ec3

Browse files
dylwil3dylwil3
authored
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). <table> <tr> <th>Original &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</th> <th><code>main</code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</th> <th>This PR&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</th> <th><code>black</code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</th> </tr> <tr> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass else: # comment pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> </tr> <tr> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> </tr> <tr> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> </tr> <tr> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> </tr> <tr> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass else: # comment pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> while True: pass # comment else: pass </pre> </td> </tr> <tr> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> <td> <pre lang="python"> if True: pass # comment else: pass </pre> </td> </tr> </table>
1 parent 1a86e13 commit 04a3ec3

File tree

9 files changed

+359
-10
lines changed

9 files changed

+359
-10
lines changed

‎crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,39 @@ def f():
294294
# d
295295
# e
296296
#f
297+
298+
# Compare behavior with `while`/`else` comment placement
299+
300+
if True: pass
301+
# 1
302+
else:
303+
pass
304+
305+
if True:
306+
pass
307+
# 2
308+
else:
309+
pass
310+
311+
if True: pass
312+
# 3
313+
else:
314+
pass
315+
316+
if True: pass
317+
# 4
318+
else:
319+
pass
320+
321+
def foo():
322+
if True:
323+
pass
324+
# 5
325+
else:
326+
pass
327+
328+
if True:
329+
first;second
330+
# 6
331+
else:
332+
pass

‎crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,3 +28,37 @@
2828
and anotherCondition or aThirdCondition # trailing third condition
2929
): # comment
3030
print("Do something")
31+
32+
while True: pass
33+
# 1
34+
else:
35+
pass
36+
37+
while True:
38+
pass
39+
# 2
40+
else:
41+
pass
42+
43+
while True: pass
44+
# 3
45+
else:
46+
pass
47+
48+
while True: pass
49+
# 4
50+
else:
51+
pass
52+
53+
def foo():
54+
while True:
55+
pass
56+
# 5
57+
else:
58+
pass
59+
60+
while True:
61+
first;second
62+
# 6
63+
else:
64+
pass

‎crates/ruff_python_formatter/src/comments/mod.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,4 +1042,33 @@ else: # trailing comment
10421042

10431043
assert_debug_snapshot!(comments.debug(test_case.source_code));
10441044
}
1045+
1046+
#[test]
1047+
fn while_else_indented_comment_between_branches() {
1048+
let source = r"while True: pass
1049+
# comment
1050+
else:
1051+
pass
1052+
";
1053+
let test_case = CommentsTestCase::from_code(source);
1054+
1055+
let comments = test_case.to_comments();
1056+
1057+
assert_debug_snapshot!(comments.debug(test_case.source_code));
1058+
}
1059+
1060+
#[test]
1061+
fn while_else_very_indented_comment_between_branches() {
1062+
let source = r"while True:
1063+
pass
1064+
# comment
1065+
else:
1066+
pass
1067+
";
1068+
let test_case = CommentsTestCase::from_code(source);
1069+
1070+
let comments = test_case.to_comments();
1071+
1072+
assert_debug_snapshot!(comments.debug(test_case.source_code));
1073+
}
10451074
}

‎crates/ruff_python_formatter/src/comments/placement.rs‎

Lines changed: 65 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use ruff_python_trivia::{
88
find_only_token_in_range, first_non_trivia_token, indentation_at_offset,
99
};
1010
use ruff_source_file::LineRanges;
11-
use ruff_text_size::{Ranged, TextLen, TextRange};
11+
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
1212
use std::cmp::Ordering;
1313

1414
use crate::comments::visitor::{CommentPlacement, DecoratedComment};
@@ -602,9 +602,35 @@ fn handle_own_line_comment_between_branches<'a>(
602602
// following branch or if it a trailing comment of the previous body's last statement.
603603
let comment_indentation = comment_indentation_after(preceding, comment.range(), source);
604604

605-
let preceding_indentation = indentation(source, &preceding)
606-
.unwrap_or_default()
607-
.text_len();
605+
let preceding_indentation = indentation(source, &preceding).map_or_else(
606+
// If `indentation` returns `None`, then there is leading
607+
// content before the preceding node. In this case, we
608+
// always treat the comment as being less-indented than the
609+
// preceding. For example:
610+
//
611+
// ```python
612+
// if True: pass
613+
// # leading on `else`
614+
// else:
615+
// pass
616+
// ```
617+
// Note we even do this if the comment is very indented
618+
// (which matches `black`'s behavior as of 2025.11.11)
619+
//
620+
// ```python
621+
// if True: pass
622+
// # leading on `else`
623+
// else:
624+
// pass
625+
// ```
626+
|| {
627+
comment_indentation
628+
// This can be any positive number - we just
629+
// want to hit the `Less` branch below
630+
+ TextSize::new(1)
631+
},
632+
ruff_text_size::TextLen::text_len,
633+
);
608634

609635
// Compare to the last statement in the body
610636
match comment_indentation.cmp(&preceding_indentation) {
@@ -678,8 +704,41 @@ fn handle_own_line_comment_after_branch<'a>(
678704
preceding: AnyNodeRef<'a>,
679705
source: &str,
680706
) -> CommentPlacement<'a> {
681-
let Some(last_child) = preceding.last_child_in_body() else {
682-
return CommentPlacement::Default(comment);
707+
// If the preceding node has a body, we want the last child - e.g.
708+
//
709+
// ```python
710+
// if True:
711+
// def foo():
712+
// something
713+
// last_child
714+
// # comment
715+
// else:
716+
// pass
717+
// ```
718+
//
719+
// Otherwise, the preceding node may be the last statement in the body
720+
// of the preceding branch, in which case we can take it as our
721+
// `last_child` here - e.g.
722+
//
723+
// ```python
724+
// if True:
725+
// something
726+
// last_child
727+
// # comment
728+
// else:
729+
// pass
730+
// ```
731+
let last_child = match preceding.last_child_in_body() {
732+
Some(last) => last,
733+
None if comment.following_node().is_some_and(|following| {
734+
following.is_first_statement_in_alternate_body(comment.enclosing_node())
735+
}) =>
736+
{
737+
preceding
738+
}
739+
_ => {
740+
return CommentPlacement::Default(comment);
741+
}
683742
};
684743

685744
// We only care about the length because indentations with mixed spaces and tabs are only valid if

‎crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_indented_comment_between_branches.snap‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: crates/ruff_python_formatter/src/comments/mod.rs
3+
expression: comments.debug(test_case.source_code)
4+
---
5+
{
6+
Node {
7+
kind: StmtWhile,
8+
range: 0..45,
9+
source: `while True: pass⏎`,
10+
}: {
11+
"leading": [],
12+
"dangling": [
13+
SourceComment {
14+
text: "# comment",
15+
position: OwnLine,
16+
formatted: false,
17+
},
18+
],
19+
"trailing": [],
20+
},
21+
}

‎crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_else_very_indented_comment_between_branches.snap‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: crates/ruff_python_formatter/src/comments/mod.rs
3+
expression: comments.debug(test_case.source_code)
4+
---
5+
{
6+
Node {
7+
kind: StmtPass,
8+
range: 16..20,
9+
source: `pass`,
10+
}: {
11+
"leading": [],
12+
"dangling": [],
13+
"trailing": [
14+
SourceComment {
15+
text: "# comment",
16+
position: OwnLine,
17+
formatted: false,
18+
},
19+
],
20+
},
21+
}

‎crates/ruff_python_formatter/tests/snapshots/format@form_feed.py.snap‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
---
22
source: crates/ruff_python_formatter/tests/fixtures.rs
33
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/form_feed.py
4-
snapshot_kind: text
54
---
65
## Input
76
```python
@@ -18,7 +17,7 @@ else:
1817
# Regression test for: https://github.com/astral-sh/ruff/issues/7624
1918
if symbol is not None:
2019
request["market"] = market["id"]
21-
# "remaining_volume": "0.0",
20+
# "remaining_volume": "0.0",
2221
else:
2322
pass
2423
```

‎crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
---
22
source: crates/ruff_python_formatter/tests/fixtures.rs
33
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py
4-
snapshot_kind: text
54
---
65
## Input
76
```python
@@ -301,6 +300,42 @@ if parent_body:
301300
# d
302301
# e
303302
#f
303+
304+
# Compare behavior with `while`/`else` comment placement
305+
306+
if True: pass
307+
# 1
308+
else:
309+
pass
310+
311+
if True:
312+
pass
313+
# 2
314+
else:
315+
pass
316+
317+
if True: pass
318+
# 3
319+
else:
320+
pass
321+
322+
if True: pass
323+
# 4
324+
else:
325+
pass
326+
327+
def foo():
328+
if True:
329+
pass
330+
# 5
331+
else:
332+
pass
333+
334+
if True:
335+
first;second
336+
# 6
337+
else:
338+
pass
304339
```
305340

306341
## Output
@@ -607,6 +642,48 @@ if parent_body:
607642
# d
608643
# e
609644
# f
645+
646+
# Compare behavior with `while`/`else` comment placement
647+
648+
if True:
649+
pass
650+
# 1
651+
else:
652+
pass
653+
654+
if True:
655+
pass
656+
# 2
657+
else:
658+
pass
659+
660+
if True:
661+
pass
662+
# 3
663+
else:
664+
pass
665+
666+
if True:
667+
pass
668+
# 4
669+
else:
670+
pass
671+
672+
673+
def foo():
674+
if True:
675+
pass
676+
# 5
677+
else:
678+
pass
679+
680+
681+
if True:
682+
first
683+
second
684+
# 6
685+
else:
686+
pass
610687
```
611688

612689

0 commit comments

Comments
 (0)