Compare commits

...

10 Commits

Author SHA1 Message Date
Brent Westbrook
fc513684d2 update handle_unary_op_comment docs 2025-11-14 17:42:47 -05:00
Brent Westbrook
ca5f4cc96f factor out has_parenthesis_between, add some docs 2025-11-14 15:50:33 -05:00
Brent Westbrook
e0b0b54b25 try a different approach 2025-11-13 10:09:37 -05:00
Brent Westbrook
9bee558c3a add more variations on issue test case 2025-11-13 09:37:15 -05:00
Brent Westbrook
f49fda317e Revert "try only dangling"
This reverts commit 4c8540ede798f544a895225b541f46552649fd7b.
2025-11-12 15:33:48 -05:00
Brent Westbrook
1cf5eff9f4 try only dangling 2025-11-12 15:33:48 -05:00
Brent Westbrook
ecccfd2a56 dangling operator comment
fix leading comments
2025-11-12 15:33:48 -05:00
Brent Westbrook
a303d1aa49 leading operand comment 2025-11-12 15:33:48 -05:00
Brent Westbrook
6eebc35eec add failing test 2025-11-12 15:33:48 -05:00
Brent Westbrook
923b4dd01e Fix panic when formatting comments in unary expressions
Summary
--

This is a second attempt at fixing #19226 based on the feedback in https://github.com/astral-sh/ruff/pull/20494#issuecomment-3467920065.

We currently mark the comment in an expression like this:

```py
if '' and (not #
0):
    pass
```

as a leading comment on `not`, eventually causing a panic in
`Operand::has_unparenthesized_leading_comments` because the end of the comment
is greater than the start of the expression.

a1d9cb5830/crates/ruff_python_formatter/src/expression/binary_like.rs (L843)

This PR fixes the issue by instead making such a comment a dangling comment on
the unary expression.

In the third commit, I instead tried making the comment a leading comment on the
operand, which also looks pretty reasonable to me. Making it a dangling comment
seems more in line with the docs on `handle_unary_op_comments`, though.

I also tried deleting the leading comment logic in favor of the new dangling
logic in the fifth commit before reverting in the sixth. This looks okay to me
too, but the current state of the PR seems like the least invasive fix.

Test Plan
--

A new, minimized test case based on the issue. I also checked that the original
snippet from the report works now.

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>
2025-11-12 15:31:49 -05:00
4 changed files with 110 additions and 32 deletions

View File

@@ -193,3 +193,19 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
):
pass
# Regression tests for https://github.com/astral-sh/ruff/issues/19226
if '' and (not #
0):
pass
if '' and (not #
(0)
):
pass
if '' and (not
( #
0
)):
pass

View File

@@ -1831,7 +1831,7 @@ fn handle_lambda_comment<'a>(
CommentPlacement::Default(comment)
}
/// Move comment between a unary op and its operand before the unary op by marking them as trailing.
/// Move comment between a unary op and its operand before the unary op by marking them as leading.
///
/// For example, given:
/// ```python
@@ -1841,8 +1841,13 @@ fn handle_lambda_comment<'a>(
/// )
/// ```
///
/// The `# comment` will be attached as a dangling comment on the enclosing node, to ensure that
/// it remains on the same line as the operator.
/// The `# comment` will be attached as a leading comment on the unary op, to ensure that
/// it doesn't fall between the operator and its operand:
/// ```python
/// ( # comment
/// not True
/// )
/// ```
fn handle_unary_op_comment<'a>(
comment: DecoratedComment<'a>,
unary_op: &'a ast::ExprUnaryOp,

View File

@@ -835,21 +835,9 @@ impl<'a> Operand<'a> {
Operand::Middle { expression } | Operand::Right { expression, .. } => {
let leading = comments.leading(*expression);
if is_expression_parenthesized((*expression).into(), comments.ranges(), source) {
leading.iter().any(|comment| {
!comment.is_formatted()
&& matches!(
SimpleTokenizer::new(
source,
TextRange::new(comment.end(), expression.start()),
)
.skip_trivia()
.next(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen,
..
})
)
})
leading
.iter()
.any(|comment| has_parenthesis_between(comment, expression, source))
} else {
!leading.is_empty()
}
@@ -877,6 +865,53 @@ impl<'a> Operand<'a> {
}
}
/// Returns `true` if a left parenthesis is the first non-trivia token found between `comment` and
/// `expression`.
///
/// This can be used to detect unparenthesized leading comments, for example:
///
/// ```py
/// # leading comment
/// (
/// expression
/// )
/// ```
fn has_parenthesis_between(comment: &SourceComment, expression: &Expr, source: &str) -> bool {
// We adjust the comment placement for unary operators to avoid splitting the operator and its
// operand as in:
//
// ```py
// (
// not
// # comment
// True
// )
// ```
//
// but making these leading comments means that the comment range falls after the start of the
// expression.
//
// This case can only occur when the comment is after the operator, so it's safe to return
// `false` here. The comment will definitely be parenthesized if the the operator is.
//
// Unary operators are the only known case of this, so `debug_assert!` that it stays that way.
debug_assert!(
expression.is_unary_op_expr() || comment.end() <= expression.start(),
"Expected leading comment to come before its expression",
);
comment.end() <= expression.start()
&& !comment.is_formatted()
&& matches!(
SimpleTokenizer::new(source, TextRange::new(comment.end(), expression.start()),)
.skip_trivia()
.next(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen,
..
})
)
}
impl Format<PyFormatContext<'_>> for Operand<'_> {
fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> {
let expression = self.expression();
@@ -922,19 +957,7 @@ impl Format<PyFormatContext<'_>> for Operand<'_> {
let leading_before_parentheses_end = leading
.iter()
.rposition(|comment| {
comment.is_unformatted()
&& matches!(
SimpleTokenizer::new(
f.context().source(),
TextRange::new(comment.end(), expression.start()),
)
.skip_trivia()
.next(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen,
..
})
)
has_parenthesis_between(comment, expression, f.context().source())
})
.map_or(0, |position| position + 1);

View File

@@ -1,7 +1,6 @@
---
source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py
snapshot_kind: text
---
## Input
```python
@@ -200,6 +199,22 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
):
pass
# Regression tests for https://github.com/astral-sh/ruff/issues/19226
if '' and (not #
0):
pass
if '' and (not #
(0)
):
pass
if '' and (not
( #
0
)):
pass
```
## Output
@@ -415,4 +430,23 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
):
pass
# Regression tests for https://github.com/astral-sh/ruff/issues/19226
if "" and ( #
not 0
):
pass
if "" and ( #
not (0)
):
pass
if "" and (
not ( #
0
)
):
pass
```