From cbc6863b8cb62ebaa6371e0130f441feb196bc66 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Tue, 18 Nov 2025 10:48:14 -0500 Subject: [PATCH] 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:
Input Main PR 
if (
    not
    # comment
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (
    # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass


if (
    not
    # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass


if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass

if (
    # unary comment
    # operand comment
    not (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass


if (
    # unary comment
    not
    # operand comment
    (
        # comment
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    )
):
    pass


if (
    not # comment
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass

if (  # comment
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass


if (
    not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa  # comment
    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
):
    pass
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 Co-authored-by: Takayuki Maeda --- .../test/fixtures/ruff/expression/unary.py | 55 ++++++ .../src/comments/placement.rs | 17 +- .../src/expression/expr_unary_op.rs | 73 ++++++-- .../format@expression__unary.py.snap | 159 +++++++++++++++--- ...s__expression_parentheses_comments.py.snap | 13 +- 5 files changed, 269 insertions(+), 48 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py index 5c8ab8d8b3..a444088b0a 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py @@ -193,3 +193,58 @@ 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 + +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 diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 2bd7402a31..f28f9b18a8 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1890,9 +1890,11 @@ 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 an end-of-line comment between a unary op and its operand after the operand by marking +/// it as dangling. /// /// For example, given: +/// /// ```python /// ( /// 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 -/// it remains on the same line as the operator. +/// the `# comment` will be attached as a dangling comment on the unary op and formatted as: +/// +/// ```python +/// ( +/// not True # comment +/// ) +/// ``` fn handle_unary_op_comment<'a>( comment: DecoratedComment<'a>, unary_op: &'a ast::ExprUnaryOp, @@ -1923,8 +1930,8 @@ fn handle_unary_op_comment<'a>( let up_to = tokenizer .find(|token| token.kind == SimpleTokenKind::LParen) .map_or(unary_op.operand.start(), |lparen| lparen.start()); - if comment.end() < up_to { - CommentPlacement::leading(unary_op, comment) + if comment.end() < up_to && comment.line_position().is_end_of_line() { + CommentPlacement::dangling(unary_op, comment) } else { CommentPlacement::Default(comment) } diff --git a/crates/ruff_python_formatter/src/expression/expr_unary_op.rs b/crates/ruff_python_formatter/src/expression/expr_unary_op.rs index a8454cda1e..5cc741018f 100644 --- a/crates/ruff_python_formatter/src/expression/expr_unary_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_unary_op.rs @@ -1,6 +1,8 @@ use ruff_python_ast::AnyNodeRef; use ruff_python_ast::ExprUnaryOp; use ruff_python_ast::UnaryOp; +use ruff_python_ast::parenthesize::parenthesized_range; +use ruff_text_size::Ranged; use crate::comments::trailing_comments; use crate::expression::parentheses::{ @@ -39,20 +41,25 @@ impl FormatNodeRule for FormatExprUnaryOp { // ``` 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 // if ( // not // # comment - // a) + // a): + // pass + // + // if 1 and ( + // not + // # comment + // ( + // a + // ) + // ): + // pass // ``` - if comments.has_leading(operand.as_ref()) - && !is_expression_parenthesized( - operand.as_ref().into(), - f.context().comments().ranges(), - f.context().source(), - ) - { + if needs_line_break(item, f.context()) { hard_line_break().fmt(f)?; } else if op.is_not() { space().fmt(f)?; @@ -76,17 +83,51 @@ impl NeedsParentheses for ExprUnaryOp { context: &PyFormatContext, ) -> OptionalParentheses { if parent.is_expr_await() { - OptionalParentheses::Always - } else if is_expression_parenthesized( + return OptionalParentheses::Always; + } + + if needs_line_break(self, context) { + return OptionalParentheses::Always; + } + + if is_expression_parenthesized( self.operand.as_ref().into(), context.comments().ranges(), context.source(), ) { - OptionalParentheses::Never - } else if context.comments().has(self.operand.as_ref()) { - OptionalParentheses::Always - } else { - self.operand.needs_parentheses(self.into(), context) + return OptionalParentheses::Never; } + + 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 +} diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap index 67f54f9da9..9dffb4e517 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap @@ -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,61 @@ 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 + +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 @@ -250,31 +304,35 @@ if +( pass if ( + not # comment - not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( + ~ # comment - ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( + - # comment - -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( + + # comment - +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass @@ -283,8 +341,9 @@ if ( if ( # unary comment + not # operand comment - not ( + ( # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb @@ -318,31 +377,28 @@ if ( ## Trailing operator comments -if ( # comment - not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +if ( + not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - # comment - ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - # comment - -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - # comment - +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass @@ -362,14 +418,13 @@ if ( pass if ( + not # comment - not a + a ): pass -if ( # comment - not a -): +if not a: # comment pass # 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 x = ( # a - # b + not # b # c - not ( # d + ( # d # e True ) @@ -415,4 +470,68 @@ 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 + +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 ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap index 5e8e826231..16152c5670 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap @@ -1,7 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py -snapshot_kind: text --- ## Input ```python @@ -179,13 +178,13 @@ nested_parentheses4 = [ x = ( # unary comment + not # in-between comment - not ( + ( # leading inner "a" ), - # in-between comment - not ( + not ( # in-between comment # leading inner "b" ), @@ -194,8 +193,7 @@ x = ( "c" ), # 1 - # 2 - not ( # 3 + not ( # 2 # 3 # 4 "d" ), @@ -203,8 +201,9 @@ x = ( if ( # unary comment + not # in-between comment - not ( + ( # leading inner 1 )