-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix panic when formatting comments in unary expressions #21501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
9fda4d1
e7a838a
7b1da06
d9486aa
e4dc4b2
2d9b3fe
c7fc121
60317c8
7e193df
31d43c5
a50e006
bd612c4
91cb799
c0d4d91
58cd40f
d89e1c2
9d86a5c
9e16151
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| use ruff_python_ast::AnyNodeRef; | ||
| use ruff_python_ast::ExprUnaryOp; | ||
| use ruff_python_ast::UnaryOp; | ||
| use ruff_python_ast::parenthesize::parenthesized_range; | ||
| use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; | ||
| use ruff_text_size::{Ranged, TextRange, TextSize}; | ||
|
|
||
| use crate::comments::trailing_comments; | ||
| use crate::expression::parentheses::{ | ||
|
|
@@ -39,19 +42,43 @@ impl FormatNodeRule<ExprUnaryOp> 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 | ||
| // ``` | ||
| let parenthesized_operand_range = parenthesized_range( | ||
| operand.into(), | ||
| item.into(), | ||
| comments.ranges(), | ||
| f.context().source(), | ||
| ); | ||
| let has_leading_comments_before_parens = parenthesized_operand_range.is_some_and(|range| { | ||
| comments | ||
| .leading(operand.as_ref()) | ||
| .iter() | ||
| .any(|comment| comment.start() < range.start()) | ||
| }); | ||
| if comments.has_leading(operand.as_ref()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's assign the leading commnts to a variable to avoid retrieving them twice |
||
| && !is_expression_parenthesized( | ||
| operand.as_ref().into(), | ||
| f.context().comments().ranges(), | ||
| f.context().source(), | ||
| ) | ||
| || has_leading_comments_before_parens | ||
| { | ||
| hard_line_break().fmt(f)?; | ||
| } else if op.is_not() { | ||
|
|
@@ -76,17 +103,51 @@ impl NeedsParentheses for ExprUnaryOp { | |
| context: &PyFormatContext, | ||
| ) -> OptionalParentheses { | ||
| if parent.is_expr_await() { | ||
| OptionalParentheses::Always | ||
| } else if is_expression_parenthesized( | ||
| 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()) { | ||
| return OptionalParentheses::Never; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to change the logic here too to match the logic for when we insert a hard line break in the unary formatting?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean something like this? if !context.comments().has_leading(self.operand.as_ref())
|| is_expression_parenthesized(
self.operand.as_ref().into(),
context.comments().ranges(),
context.source(),
)
{
return OptionalParentheses::Never;
}I played with a few variations on this and kept running into instabilities. It seems to be working okay without matching the check exactly, like on main.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, more like this: It's important that it exactly mirrors the case when we insert a hard line break in the formatting code because any line break will lead to invalid syntax if the Here's an example where your PR produces invalid syntax: if (
not
# comment
(a)):
passWe should add more tests that exercise the new leading comment placement (may even be true for the trailing comment placement, are there more combinations that you could test?) |
||
| } | ||
|
|
||
| let operand_start = operand_start(self, context.source()); | ||
| if context | ||
| .comments() | ||
| .dangling(self) | ||
| .iter() | ||
| .any(|comment| comment.end() < operand_start) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can simplify this to returning Does this need to take precedence over the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to work in both orders, at least with our current tests. |
||
| { | ||
| return OptionalParentheses::Multiline; | ||
| } | ||
|
|
||
| if context.comments().has(self.operand.as_ref()) { | ||
| OptionalParentheses::Always | ||
| } else { | ||
| self.operand.needs_parentheses(self.into(), context) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Returns the start of `unary_op`'s operand, or its leading parenthesis, if it has one. | ||
| pub(crate) fn operand_start(unary_op: &ExprUnaryOp, source: &str) -> TextSize { | ||
| let mut tokenizer = SimpleTokenizer::new( | ||
| source, | ||
| TextRange::new(unary_op.start(), unary_op.operand.start()), | ||
| ) | ||
| .skip_trivia(); | ||
| let op_token = tokenizer.next(); | ||
| debug_assert!(op_token.is_some_and(|token| matches!( | ||
| token.kind, | ||
| SimpleTokenKind::Tilde | ||
| | SimpleTokenKind::Not | ||
| | SimpleTokenKind::Plus | ||
| | SimpleTokenKind::Minus | ||
| ))); | ||
| tokenizer | ||
| .find(|token| token.kind == SimpleTokenKind::LParen) | ||
| .map_or(unary_op.operand.start(), |lparen| lparen.start()) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the method description to match our new behavior