Skip to content

Commit

Permalink
Fix instable formatting for trailing subscribt end-of-line comment (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
MichaReiser committed Mar 20, 2024
1 parent 7caf0d0 commit 954a48b
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,29 @@
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
)[0]


# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0
]

repro(
"some long string that takes up some space"
)[0 # some long comment also taking up space
]

repro(
"some long string that takes up some space"
)[0] # some long comment also taking up space

repro("some long string that takes up some space")[0] # some long comment also taking up space


repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0:-1
]
37 changes: 34 additions & 3 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,41 @@ fn handle_enclosed_comment<'a>(
}
AnyNodeRef::ExprSubscript(expr_subscript) => {
if let Expr::Slice(expr_slice) = expr_subscript.slice.as_ref() {
handle_slice_comments(comment, expr_slice, comment_ranges, locator)
} else {
CommentPlacement::Default(comment)
return handle_slice_comments(comment, expr_slice, comment_ranges, locator);
}

// Handle non-slice subscript end-of-line comments coming after the `[`
// ```python
// repro(
// "some long string that takes up some space"
// )[ # some long comment also taking up space
// 0
// ]
// ```
if comment.line_position().is_end_of_line() {
// Ensure that there are no tokens between the open bracket and the comment.
let mut lexer = SimpleTokenizer::new(
locator.contents(),
TextRange::new(expr_subscript.value.end(), comment.start()),
)
.skip_trivia();

// Skip the opening parenthesis.
let Some(paren) = lexer.next() else {
return CommentPlacement::Default(comment);
};

debug_assert_eq!(paren.kind(), SimpleTokenKind::LBracket);

// If there are no additional tokens between the open parenthesis and the comment, then
// it should be attached as a dangling comment on the brackets, rather than a leading
// comment on the first argument.
if lexer.next().is_none() {
return CommentPlacement::dangling(expr_subscript, comment);
}
}

CommentPlacement::Default(comment)
}
AnyNodeRef::ModModule(module) => {
handle_trailing_module_comment(module, comment).or_else(|comment| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,32 @@ result = (
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
)[0]
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0
]
repro(
"some long string that takes up some space"
)[0 # some long comment also taking up space
]
repro(
"some long string that takes up some space"
)[0] # some long comment also taking up space
repro("some long string that takes up some space")[0] # some long comment also taking up space
repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0:-1
]
```

## Output
Expand All @@ -18,7 +44,31 @@ result = (
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
)[0]
```
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355
repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0
]
repro("some long string that takes up some space")[
0 # some long comment also taking up space
]
repro("some long string that takes up some space")[
0
] # some long comment also taking up space
repro("some long string that takes up some space")[
0
] # some long comment also taking up space
repro(
"some long string that takes up some space"
)[ # some long comment also taking up space
0:-1
]
```

0 comments on commit 954a48b

Please sign in to comment.