Skip to content

Commit 954a48b

Browse files
authored
Fix instable formatting for trailing subscribt end-of-line comment (#10492)
## Summary This PR fixes an instability where formatting a subscribt where the `slice` is not an `ExprSlice` and it has a trailing end-of-line comment after its opening `[` required two formatting passes to be stable. The fix is to associate the trailing end-of-line comment as dangling comment on `[` to preserve its position, similar to how Ruff does it for other parenthesized expressions. This also matches how trailing end-of-line subscript comments are handled when the `slice` is an `ExprSlice`. Fixes #10355 ## Versioning Shipping this as part of a patch release is fine because: * It fixes a stability issue * It doesn't impact already formatted code because Ruff would already have moved to the comment to the end of the line (instead of preserving it) ## Test Plan Added tests
1 parent 7caf0d0 commit 954a48b

File tree

3 files changed

+111
-4
lines changed

3 files changed

+111
-4
lines changed

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py

+26
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,29 @@
33
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
44
+ 1
55
)[0]
6+
7+
8+
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
9+
repro(
10+
"some long string that takes up some space"
11+
)[ # some long comment also taking up space
12+
0
13+
]
14+
15+
repro(
16+
"some long string that takes up some space"
17+
)[0 # some long comment also taking up space
18+
]
19+
20+
repro(
21+
"some long string that takes up some space"
22+
)[0] # some long comment also taking up space
23+
24+
repro("some long string that takes up some space")[0] # some long comment also taking up space
25+
26+
27+
repro(
28+
"some long string that takes up some space"
29+
)[ # some long comment also taking up space
30+
0:-1
31+
]

crates/ruff_python_formatter/src/comments/placement.rs

+34-3
Original file line numberDiff line numberDiff line change
@@ -251,10 +251,41 @@ fn handle_enclosed_comment<'a>(
251251
}
252252
AnyNodeRef::ExprSubscript(expr_subscript) => {
253253
if let Expr::Slice(expr_slice) = expr_subscript.slice.as_ref() {
254-
handle_slice_comments(comment, expr_slice, comment_ranges, locator)
255-
} else {
256-
CommentPlacement::Default(comment)
254+
return handle_slice_comments(comment, expr_slice, comment_ranges, locator);
255+
}
256+
257+
// Handle non-slice subscript end-of-line comments coming after the `[`
258+
// ```python
259+
// repro(
260+
// "some long string that takes up some space"
261+
// )[ # some long comment also taking up space
262+
// 0
263+
// ]
264+
// ```
265+
if comment.line_position().is_end_of_line() {
266+
// Ensure that there are no tokens between the open bracket and the comment.
267+
let mut lexer = SimpleTokenizer::new(
268+
locator.contents(),
269+
TextRange::new(expr_subscript.value.end(), comment.start()),
270+
)
271+
.skip_trivia();
272+
273+
// Skip the opening parenthesis.
274+
let Some(paren) = lexer.next() else {
275+
return CommentPlacement::Default(comment);
276+
};
277+
278+
debug_assert_eq!(paren.kind(), SimpleTokenKind::LBracket);
279+
280+
// If there are no additional tokens between the open parenthesis and the comment, then
281+
// it should be attached as a dangling comment on the brackets, rather than a leading
282+
// comment on the first argument.
283+
if lexer.next().is_none() {
284+
return CommentPlacement::dangling(expr_subscript, comment);
285+
}
257286
}
287+
288+
CommentPlacement::Default(comment)
258289
}
259290
AnyNodeRef::ModModule(module) => {
260291
handle_trailing_module_comment(module, comment).or_else(|comment| {

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

+51-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,32 @@ result = (
99
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
1010
+ 1
1111
)[0]
12+
13+
14+
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
15+
repro(
16+
"some long string that takes up some space"
17+
)[ # some long comment also taking up space
18+
0
19+
]
20+
21+
repro(
22+
"some long string that takes up some space"
23+
)[0 # some long comment also taking up space
24+
]
25+
26+
repro(
27+
"some long string that takes up some space"
28+
)[0] # some long comment also taking up space
29+
30+
repro("some long string that takes up some space")[0] # some long comment also taking up space
31+
32+
33+
repro(
34+
"some long string that takes up some space"
35+
)[ # some long comment also taking up space
36+
0:-1
37+
]
1238
```
1339

1440
## Output
@@ -18,7 +44,31 @@ result = (
1844
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
1945
+ 1
2046
)[0]
21-
```
2247
2348
49+
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
50+
repro(
51+
"some long string that takes up some space"
52+
)[ # some long comment also taking up space
53+
0
54+
]
55+
56+
repro("some long string that takes up some space")[
57+
0 # some long comment also taking up space
58+
]
59+
60+
repro("some long string that takes up some space")[
61+
0
62+
] # some long comment also taking up space
2463
64+
repro("some long string that takes up some space")[
65+
0
66+
] # some long comment also taking up space
67+
68+
69+
repro(
70+
"some long string that takes up some space"
71+
)[ # some long comment also taking up space
72+
0:-1
73+
]
74+
```

0 commit comments

Comments
 (0)