Fix subscript comment placement with parenthesized value (#10496)

## Summary This is a follow up on #10492 I incorrectly assumed that `subscript.value.end()` always points past the value. However, this isn't the case for parenthesized values where the end "ends" before the parentheses. ## Test Plan I added new tests for the parenthesized case.
astral-sh · Mar 20, 2024 · 9d705a4 · 9d705a4
1 parent fd3d272
commit 9d705a4
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 13 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
@@ -4,7 +4,6 @@
     + 1
 )[0]
 
-
 # Regression tests for: https://github.com/astral-sh/ruff/issues/10355
 repro(
     "some long string that takes up some space"
@@ -14,7 +13,7 @@
 
 repro(
     "some long string that takes up some space"
-)[0 # some long comment also taking up space
+)[0  # some long comment also taking up space
 ]
 
 repro(
@@ -23,9 +22,20 @@
 
 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
 ]
+
+(
+    repro
+)[  # some long comment also taking up space
+    0
+]
+
+(
+    repro  # some long comment also taking up space
+)[
+    0
+]
diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs
@@ -262,21 +262,24 @@ fn handle_enclosed_comment<'a>(
             //     0
             // ]
             // ```
-            if comment.line_position().is_end_of_line() {
+            if comment.line_position().is_end_of_line()
+                && expr_subscript.value.end() < comment.start()
+            {
                 // 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 {
+                // Skip to after the opening parenthesis (may skip some closing parentheses of value)
+                if !lexer
+                    .by_ref()
+                    .any(|token| token.kind() == SimpleTokenKind::LBracket)
+                {
                     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.

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
@@ -10,7 +10,6 @@ result = (
     + 1
 )[0]
 
-
 # Regression tests for: https://github.com/astral-sh/ruff/issues/10355
 repro(
     "some long string that takes up some space"
@@ -20,7 +19,7 @@ repro(
 
 repro(
     "some long string that takes up some space"
-)[0 # some long comment also taking up space
+)[0  # some long comment also taking up space
 ]
 
 repro(
@@ -29,12 +28,23 @@ repro(
 
 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
 ]
+
+(
+    repro
+)[  # some long comment also taking up space
+    0
+]
+
+(
+    repro  # some long comment also taking up space
+)[
+    0
+]
 ```
 
 ## Output
@@ -45,7 +55,6 @@ result = (
     + 1
 )[0]
 
-
 # Regression tests for: https://github.com/astral-sh/ruff/issues/10355
 repro(
     "some long string that takes up some space"
@@ -65,10 +74,17 @@ 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
 ]
+
+(repro)[  # some long comment also taking up space
+    0
+]
+
+(
+    repro  # some long comment also taking up space
+)[0]
 ```