Fix panic when formatting comments in unary expressions (#21501)

## Summary

This is another attempt at https://github.com/astral-sh/ruff/pull/21410
that fixes https://github.com/astral-sh/ruff/issues/19226.

@MichaReiser helped me get something working in a very helpful pairing
session. I pushed one additional commit moving the comments back from
leading comments to trailing comments, which I think retains more of the
input formatting.

I was inspired by Dylan's PR (#21185) to make one of these tables:

<table>
                <thead>
                    <tr>
                    <th scope="col">Input</th>
                    <th scope="col">Main</th>
                    <th scope="col">PR</th>
                    </tr>
                </thead>
                <tbody>
<tr>
<td><pre lang="python">
if (
    not
    # comment
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass
</pre></td>
<td><pre lang="python">
if (
    # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

</pre></td>
<td><pre lang="python">
if (
    not
    # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

</pre></td>
</tr>
<tr>
<td><pre lang="python">
if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass
</pre></td>
<td><pre lang="python">
if (
    # unary comment
    # operand comment
    not (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

</pre></td>
<td><pre lang="python">
if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

</pre></td>
</tr>
<tr>
<td><pre lang="python">
if (
    not # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass
</pre></td>
<td><pre lang="python">
if (  # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

</pre></td>
<td><pre lang="python">
if (
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa  # comment
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

</pre></td>
</tr>
</tbody>
            </table>

hopefully it helps even though the snippets are much wider here.

The two main differences are (1) that we now retain own-line comments
between the unary operator and its operand instead of moving these to
leading comments on the operator itself, and (2) that we move
end-of-line comments between the operator and operand to dangling
end-of-line comments on the operand (the last example in the table).

## Test Plan

Existing tests, plus new ones based on the issue. As I noted below, I
also ran the output from main on the unary.py file back through this
branch to check that we don't reformat code from main. This made me feel
a bit better about not preview-gating the changes in this PR.

```shell
> git show main:crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py | ruff format - | ./target/debug/ruff format --diff -
> echo $?
0
```

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>
This commit is contained in:
Brent Westbrook 2025-11-18 10:48:14 -05:00 committed by GitHub
parent 7043d51df0
commit cbc6863b8c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 269 additions and 48 deletions

View File

@ -193,3 +193,58 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
): ):
pass 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
if (
not
# comment
(a)):
pass
if not ( # comment
a):
pass
if not (
# comment
(a)):
pass
if not (
# comment
a):
pass
not (# comment
(a))
(-#comment
(a))
if ( # a
# b
not # c
# d
( # e
# f
a # g
# h
) # i
# j
):
pass

View File

@ -1890,9 +1890,11 @@ fn handle_lambda_comment<'a>(
CommentPlacement::Default(comment) CommentPlacement::Default(comment)
} }
/// Move comment between a unary op and its operand before the unary op by marking them as trailing. /// Move an end-of-line comment between a unary op and its operand after the operand by marking
/// it as dangling.
/// ///
/// For example, given: /// For example, given:
///
/// ```python /// ```python
/// ( /// (
/// not # comment /// not # comment
@ -1900,8 +1902,13 @@ fn handle_lambda_comment<'a>(
/// ) /// )
/// ``` /// ```
/// ///
/// The `# comment` will be attached as a dangling comment on the enclosing node, to ensure that /// the `# comment` will be attached as a dangling comment on the unary op and formatted as:
/// it remains on the same line as the operator. ///
/// ```python
/// (
/// not True # comment
/// )
/// ```
fn handle_unary_op_comment<'a>( fn handle_unary_op_comment<'a>(
comment: DecoratedComment<'a>, comment: DecoratedComment<'a>,
unary_op: &'a ast::ExprUnaryOp, unary_op: &'a ast::ExprUnaryOp,
@ -1923,8 +1930,8 @@ fn handle_unary_op_comment<'a>(
let up_to = tokenizer let up_to = tokenizer
.find(|token| token.kind == SimpleTokenKind::LParen) .find(|token| token.kind == SimpleTokenKind::LParen)
.map_or(unary_op.operand.start(), |lparen| lparen.start()); .map_or(unary_op.operand.start(), |lparen| lparen.start());
if comment.end() < up_to { if comment.end() < up_to && comment.line_position().is_end_of_line() {
CommentPlacement::leading(unary_op, comment) CommentPlacement::dangling(unary_op, comment)
} else { } else {
CommentPlacement::Default(comment) CommentPlacement::Default(comment)
} }

View File

@ -1,6 +1,8 @@
use ruff_python_ast::AnyNodeRef; use ruff_python_ast::AnyNodeRef;
use ruff_python_ast::ExprUnaryOp; use ruff_python_ast::ExprUnaryOp;
use ruff_python_ast::UnaryOp; use ruff_python_ast::UnaryOp;
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_text_size::Ranged;
use crate::comments::trailing_comments; use crate::comments::trailing_comments;
use crate::expression::parentheses::{ use crate::expression::parentheses::{
@ -39,20 +41,25 @@ impl FormatNodeRule<ExprUnaryOp> for FormatExprUnaryOp {
// ``` // ```
trailing_comments(dangling).fmt(f)?; trailing_comments(dangling).fmt(f)?;
// Insert a line break if the operand has comments but itself is not parenthesized. // Insert a line break if the operand has comments but itself is not parenthesized or if the
// operand is parenthesized but has a leading comment before the parentheses.
// ```python // ```python
// if ( // if (
// not // not
// # comment // # comment
// a) // a):
// pass
//
// if 1 and (
// not
// # comment
// (
// a
// )
// ):
// pass
// ``` // ```
if comments.has_leading(operand.as_ref()) if needs_line_break(item, f.context()) {
&& !is_expression_parenthesized(
operand.as_ref().into(),
f.context().comments().ranges(),
f.context().source(),
)
{
hard_line_break().fmt(f)?; hard_line_break().fmt(f)?;
} else if op.is_not() { } else if op.is_not() {
space().fmt(f)?; space().fmt(f)?;
@ -76,17 +83,51 @@ impl NeedsParentheses for ExprUnaryOp {
context: &PyFormatContext, context: &PyFormatContext,
) -> OptionalParentheses { ) -> OptionalParentheses {
if parent.is_expr_await() { if parent.is_expr_await() {
OptionalParentheses::Always return OptionalParentheses::Always;
} else if is_expression_parenthesized( }
if needs_line_break(self, context) {
return OptionalParentheses::Always;
}
if is_expression_parenthesized(
self.operand.as_ref().into(), self.operand.as_ref().into(),
context.comments().ranges(), context.comments().ranges(),
context.source(), context.source(),
) { ) {
OptionalParentheses::Never return OptionalParentheses::Never;
} else if context.comments().has(self.operand.as_ref()) {
OptionalParentheses::Always
} else {
self.operand.needs_parentheses(self.into(), context)
} }
if context.comments().has(self.operand.as_ref()) {
return OptionalParentheses::Always;
}
self.operand.needs_parentheses(self.into(), context)
} }
} }
/// Returns `true` if the unary operator will have a hard line break between the operator and its
/// operand and thus requires parentheses.
fn needs_line_break(item: &ExprUnaryOp, context: &PyFormatContext) -> bool {
let comments = context.comments();
let parenthesized_operand_range = parenthesized_range(
item.operand.as_ref().into(),
item.into(),
comments.ranges(),
context.source(),
);
let leading_operand_comments = comments.leading(item.operand.as_ref());
let has_leading_comments_before_parens = parenthesized_operand_range.is_some_and(|range| {
leading_operand_comments
.iter()
.any(|comment| comment.start() < range.start())
});
!leading_operand_comments.is_empty()
&& !is_expression_parenthesized(
item.operand.as_ref().into(),
context.comments().ranges(),
context.source(),
)
|| has_leading_comments_before_parens
}

View File

@ -1,7 +1,6 @@
--- ---
source: crates/ruff_python_formatter/tests/fixtures.rs source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py
snapshot_kind: text
--- ---
## Input ## Input
```python ```python
@ -200,6 +199,61 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
): ):
pass 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
if (
not
# comment
(a)):
pass
if not ( # comment
a):
pass
if not (
# comment
(a)):
pass
if not (
# comment
a):
pass
not (# comment
(a))
(-#comment
(a))
if ( # a
# b
not # c
# d
( # e
# f
a # g
# h
) # i
# j
):
pass
``` ```
## Output ## Output
@ -250,31 +304,35 @@ if +(
pass pass
if ( if (
not
# comment # comment
not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
~
# comment # comment
~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
-
# comment # comment
-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
+
# comment # comment
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
@ -283,8 +341,9 @@ if (
if ( if (
# unary comment # unary comment
not
# operand comment # operand comment
not ( (
# comment # comment
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
@ -318,31 +377,28 @@ if (
## Trailing operator comments ## Trailing operator comments
if ( # comment if (
not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
# comment ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment
~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
# comment -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment
-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
if ( if (
# comment +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
): ):
pass pass
@ -362,14 +418,13 @@ if (
pass pass
if ( if (
not
# comment # comment
not a a
): ):
pass pass
if ( # comment if not a: # comment
not a
):
pass pass
# Regression test for: https://github.com/astral-sh/ruff/issues/7423 # Regression test for: https://github.com/astral-sh/ruff/issues/7423
@ -385,9 +440,9 @@ if True:
# Regression test for: https://github.com/astral-sh/ruff/issues/7448 # Regression test for: https://github.com/astral-sh/ruff/issues/7448
x = ( x = (
# a # a
# b not # b
# c # c
not ( # d ( # d
# e # e
True True
) )
@ -415,4 +470,68 @@ def foo():
not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee
): ):
pass 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
if (
not
# comment
(a)
):
pass
if not ( # comment
a
):
pass
if not (
# comment
a
):
pass
if not (
# comment
a
):
pass
not ( # comment
a
)
(
-(a) # comment
)
if ( # a
# b
not # c
# d
( # e
# f
a # g
# h
) # i
# j
):
pass
``` ```

View File

@ -1,7 +1,6 @@
--- ---
source: crates/ruff_python_formatter/tests/fixtures.rs source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py
snapshot_kind: text
--- ---
## Input ## Input
```python ```python
@ -179,13 +178,13 @@ nested_parentheses4 = [
x = ( x = (
# unary comment # unary comment
not
# in-between comment # in-between comment
not ( (
# leading inner # leading inner
"a" "a"
), ),
# in-between comment not ( # in-between comment
not (
# leading inner # leading inner
"b" "b"
), ),
@ -194,8 +193,7 @@ x = (
"c" "c"
), ),
# 1 # 1
# 2 not ( # 2 # 3
not ( # 3
# 4 # 4
"d" "d"
), ),
@ -203,8 +201,9 @@ x = (
if ( if (
# unary comment # unary comment
not
# in-between comment # in-between comment
not ( (
# leading inner # leading inner
1 1
) )