Skip to content
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

Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values #16187

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,20 @@
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
).bit_length()


# Regression test for https://github.com/astral-sh/ruff/issues/16151
result = (
(await query_the_thing(mypy_doesnt_understand)) # type: ignore[x]
.foo()
.bar()
)

(
(
a # trailing end-of-line
# trailing own-line
) # trailing closing parentheses
# dangling before dot
.b # trailing end-of-line
)
Comment on lines +164 to +171
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the significance of this test case? It seems to be working fine on main

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly wanted to make sure that I don't break this case.

6 changes: 2 additions & 4 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1391,10 +1391,8 @@ fn handle_attribute_comment<'a>(
.take_while(|token| token.kind == SimpleTokenKind::RParen)
.last()
{
return if comment.start() < right_paren.start() {
CommentPlacement::trailing(attribute.value.as_ref(), comment)
} else {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to fall through to the next case if the comment is after the closing parentheses

CommentPlacement::dangling(comment.enclosing_node(), comment)
if comment.start() < right_paren.start() {
return CommentPlacement::trailing(attribute.value.as_ref(), comment);
};
}

Expand Down
48 changes: 40 additions & 8 deletions crates/ruff_python_formatter/src/expression/expr_attribute.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use ruff_formatter::{write, FormatRuleWithOptions};
use ruff_python_ast::AnyNodeRef;
use ruff_python_ast::{Expr, ExprAttribute, ExprNumberLiteral, Number};
use ruff_python_trivia::{find_only_token_in_range, SimpleTokenKind};
use ruff_python_trivia::{find_only_token_in_range, SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{Ranged, TextRange};

use crate::comments::dangling_comments;
Expand Down Expand Up @@ -50,21 +50,16 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
if parenthesize_value {
// Don't propagate the call chain layout.
value.format().with_options(Parentheses::Always).fmt(f)?;

// Format the dot on its own line.
soft_line_break().fmt(f)?;
} else {
match value.as_ref() {
Expr::Attribute(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?;
}
Expr::Call(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?;
soft_line_break().fmt(f)?;
}
Expr::Subscript(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?;
soft_line_break().fmt(f)?;
}
_ => {
value.format().with_options(Parentheses::Never).fmt(f)?;
Expand All @@ -77,19 +72,56 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
value.format().with_options(Parentheses::Never).fmt(f)?;
}

let comments = f.context().comments().clone();

// Always add a line break if the value is parenthesized and there's an
// end of line comment on the same line as the closing parenthesis.
// ```python
// (
// (
// a
// ) # `end_of_line_comment`
// .
// b
// )
// ```
let has_trailing_end_of_line_comment =
SimpleTokenizer::starts_at(value.end(), f.context().source())
.skip_trivia()
.take_while(|token| token.kind == SimpleTokenKind::RParen)
.last()
.is_some_and(|right_paren| {
let trailing_value_comments = comments.trailing(&**value);
trailing_value_comments.iter().any(|comment| {
comment.line_position().is_end_of_line()
&& comment.start() > right_paren.end()
})
});

if has_trailing_end_of_line_comment {
hard_line_break().fmt(f)?;
}
// Allow the `.` on its own line if this is a fluent call chain
// and the value either requires parenthesizing or is a call or subscript expression
// (it's a fluent chain but not the first element).
else if call_chain_layout == CallChainLayout::Fluent {
if parenthesize_value || value.is_call_expr() || value.is_subscript_expr() {
soft_line_break().fmt(f)?;
}
}

// Identify dangling comments before and after the dot:
// ```python
// (
// (
// a
// ) # `before_dot`
// )
// # `before_dot`
// . # `after_dot`
// # `after_dot`
// b
// )
// ```
let comments = f.context().comments().clone();
let dangling = comments.dangling(item);
let (before_dot, after_dot) = if dangling.is_empty() {
(dangling, dangling)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
---
source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py
snapshot_kind: text
---
## Input
```python
Expand Down Expand Up @@ -159,6 +158,23 @@ result = (
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
).bit_length()


# Regression test for https://github.com/astral-sh/ruff/issues/16151
result = (
(await query_the_thing(mypy_doesnt_understand)) # type: ignore[x]
.foo()
.bar()
)

(
(
a # trailing end-of-line
# trailing own-line
) # trailing closing parentheses
# dangling before dot
.b # trailing end-of-line
)
```

## Output
Expand Down Expand Up @@ -295,4 +311,21 @@ result = (
f(111111111111111111111111111111111111111111111111111111111111111111111111111111111)
+ 1
).bit_length()


# Regression test for https://github.com/astral-sh/ruff/issues/16151
result = (
(await query_the_thing(mypy_doesnt_understand)) # type: ignore[x]
.foo()
.bar()
)

(
(
a # trailing end-of-line
# trailing own-line
) # trailing closing parentheses
# dangling before dot
.b # trailing end-of-line
)
```
Loading