From 954a48b129fb553ee4be6e478b8c9b4e96931b25 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 20 Mar 2024 18:12:10 +0100 Subject: [PATCH] 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 https://github.com/astral-sh/ruff/issues/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 --- .../fixtures/ruff/expression/subscript.py | 26 +++++++++ .../src/comments/placement.rs | 37 +++++++++++- .../format@expression__subscript.py.snap | 56 ++++++++++++++++++- 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py index 679f43acb5..ca1cef47c7 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py @@ -3,3 +3,29 @@ 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 +] diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index eeef8e32f7..8b26ea7cf6 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -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| { diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap index 35f5f5e929..6b318daddf 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap @@ -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 @@ -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 +] ``` - - -