Fix subscript comment placement with parenthesized value (#10496)

## Summary

This is a follow up on https://github.com/astral-sh/ruff/pull/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.
This commit is contained in:
Micha Reiser 2024-03-20 21:30:22 +01:00 committed by GitHub
parent fd3d272026
commit 9d705a4414
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 42 additions and 13 deletions

View file

@ -4,7 +4,6 @@ result = (
+ 1 + 1
)[0] )[0]
# Regression tests for: https://github.com/astral-sh/ruff/issues/10355 # Regression tests for: https://github.com/astral-sh/ruff/issues/10355
repro( repro(
"some long string that takes up some space" "some long string that takes up some space"
@ -14,7 +13,7 @@ repro(
repro( repro(
"some long string that takes up some space" "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( repro(
@ -23,9 +22,20 @@ 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")[0] # some long comment also taking up space
repro( repro(
"some long string that takes up some space" "some long string that takes up some space"
)[ # some long comment also taking up space )[ # some long comment also taking up space
0:-1 0:-1
] ]
(
repro
)[ # some long comment also taking up space
0
]
(
repro # some long comment also taking up space
)[
0
]

View file

@ -262,7 +262,9 @@ fn handle_enclosed_comment<'a>(
// 0 // 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. // Ensure that there are no tokens between the open bracket and the comment.
let mut lexer = SimpleTokenizer::new( let mut lexer = SimpleTokenizer::new(
locator.contents(), locator.contents(),
@ -270,13 +272,14 @@ fn handle_enclosed_comment<'a>(
) )
.skip_trivia(); .skip_trivia();
// Skip the opening parenthesis. // Skip to after the opening parenthesis (may skip some closing parentheses of value)
let Some(paren) = lexer.next() else { if !lexer
.by_ref()
.any(|token| token.kind() == SimpleTokenKind::LBracket)
{
return CommentPlacement::Default(comment); 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 // 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 // it should be attached as a dangling comment on the brackets, rather than a leading
// comment on the first argument. // comment on the first argument.

View file

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