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 attribute chain own line comments #5340

Merged
merged 2 commits into from
Jun 26, 2023
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
@@ -1,3 +1,7 @@
from argparse import Namespace

a = Namespace()

(
a
# comment
Expand Down Expand Up @@ -26,4 +30,74 @@
)


aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr
a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr


# Test that we add parentheses around the outermost attribute access in an attribute
# chain if and only if we need them, that is if there are own line comments inside
# the chain.
x1 = (
a
.b
# comment 1
. # comment 2
# comment 3
c
.d
)

x20 = (
a
.b
)
x21 = (
# leading name own line
a # trailing name end-of-line
.b
)
x22 = (
a
# outermost leading own line
.b # outermost trailing end-of-line
)

x31 = (
a
# own line between nodes 1
.b
)
x321 = (
a
. # end-of-line dot comment
b
)
x322 = (
a
. # end-of-line dot comment 2
b
.c
)
x331 = (
a.
# own line between nodes 3
b
)
x332 = (
""
# own line between nodes
.find
)

x8 = (
(a + a)
.b
)

x51 = (
a.b.c
)
x52 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
x53 = (
a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
)

14 changes: 13 additions & 1 deletion crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,19 @@ fn handle_attribute_comment<'a>(
.expect("Expected the `.` character after the value");

if TextRange::new(dot.end(), attribute.attr.start()).contains(comment.slice().start()) {
CommentPlacement::dangling(attribute.into(), comment)
if comment.line_position().is_end_of_line() {
// Attach to node with b
// ```python
// x322 = (
// a
// . # end-of-line dot comment 2
// b
// )
// ```
CommentPlacement::trailing(comment.enclosing_node(), comment)
} else {
CommentPlacement::dangling(attribute.into(), comment)
}
} else {
CommentPlacement::Default(comment)
}
Expand Down
47 changes: 38 additions & 9 deletions crates/ruff_python_formatter/src/expression/expr_attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,28 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
})
);

let comments = f.context().comments().clone();
let dangling_comments = comments.dangling_comments(item);
let leading_attribute_comments_start =
dangling_comments.partition_point(|comment| comment.line_position().is_end_of_line());
let (trailing_dot_comments, leading_attribute_comments) =
dangling_comments.split_at(leading_attribute_comments_start);

if needs_parentheses {
value.format().with_options(Parenthesize::Always).fmt(f)?;
} else if let Expr::Attribute(expr_attribute) = value.as_ref() {
// We're in a attribute chain (`a.b.c`). The outermost node adds parentheses if
// required, the inner ones don't need them so we skip the `Expr` formatting that
// normally adds the parentheses.
expr_attribute.format().fmt(f)?;
} else {
Comment on lines +39 to 44
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case where an inner attribute change is parenthesized? I wonder if we need to preserve the parentheses.

value.format().fmt(f)?;
}

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

if comments.has_trailing_own_line_comments(value.as_ref()) {
hard_line_break().fmt(f)?;
}

let dangling_comments = comments.dangling_comments(item);

let leading_attribute_comments_start =
dangling_comments.partition_point(|comment| comment.line_position().is_end_of_line());
let (trailing_dot_comments, leading_attribute_comments) =
dangling_comments.split_at(leading_attribute_comments_start);

write!(
f,
[
Expand All @@ -68,13 +71,39 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
}
}

/// Checks if there are any own line comments in an attribute chain (a.b.c). This method is
/// recursive up to the innermost expression that the attribute chain starts behind.
fn has_breaking_comments_attribute_chain(
expr_attribute: &ExprAttribute,
comments: &Comments,
) -> bool {
if comments
.dangling_comments(expr_attribute)
.iter()
.any(|comment| comment.line_position().is_own_line())
|| comments.has_trailing_own_line_comments(expr_attribute)
{
return true;
}

if let Expr::Attribute(inner) = expr_attribute.value.as_ref() {
return has_breaking_comments_attribute_chain(inner, comments);
}

return comments.has_trailing_own_line_comments(expr_attribute.value.as_ref());
}

impl NeedsParentheses for ExprAttribute {
fn needs_parentheses(
&self,
parenthesize: Parenthesize,
source: &str,
comments: &Comments,
) -> Parentheses {
if has_breaking_comments_attribute_chain(self, comments) {
return Parentheses::Always;
}

match default_expression_needs_parentheses(self.into(), parenthesize, source, comments) {
Parentheses::Optional => Parentheses::Never,
parentheses => parentheses,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression
---
## Input
```py
from argparse import Namespace

a = Namespace()

(
a
# comment
Expand Down Expand Up @@ -32,13 +36,87 @@ input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression
)


aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr
a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr


# Test that we add parentheses around the outermost attribute access in an attribute
# chain if and only if we need them, that is if there are own line comments inside
# the chain.
x1 = (
a
.b
# comment 1
. # comment 2
# comment 3
c
.d
)

x20 = (
a
.b
)
x21 = (
# leading name own line
a # trailing name end-of-line
.b
)
x22 = (
a
# outermost leading own line
.b # outermost trailing end-of-line
)

x31 = (
a
# own line between nodes 1
.b
)
x321 = (
a
. # end-of-line dot comment
b
)
x322 = (
a
. # end-of-line dot comment 2
b
.c
)
x331 = (
a.
# own line between nodes 3
b
)
x332 = (
""
# own line between nodes
.find
)

x8 = (
(a + a)
.b
)

x51 = (
a.b.c
)
x52 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
x53 = (
a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
)

```



## Output
```py
NOT_YET_IMPLEMENTED_StmtImportFrom

a = NOT_IMPLEMENTED_call()

(
a
# comment
Expand All @@ -61,13 +139,60 @@ aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaa
(
a
# comment
. # trailing dot comment
.
# in between
b # trailing identifier comment
b # trailing dot comment # trailing identifier comment
)


a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr


# Test that we add parentheses around the outermost attribute access in an attribute
# chain if and only if we need them, that is if there are own line comments inside
# the chain.
x1 = (
a.b
# comment 1
.
# comment 3
c.d # comment 2
)

x20 = a.b
x21 = (
# leading name own line
a.b # trailing name end-of-line
)
x22 = (
a
# outermost leading own line
.b # outermost trailing end-of-line
)

x31 = (
a
# own line between nodes 1
.b
)
x321 = a.b # end-of-line dot comment
x322 = a.b.c # end-of-line dot comment 2
x331 = (
a.
# own line between nodes 3
b
)
x332 = (
""
# own line between nodes
.find
)

x8 = (a + a).b

aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaaaaaaaaaaaiiiiiiiiiiiiiiiiiiiiiiinnnnnnnn.ooooooooooooooooooooooooofffffffff.aaaaaaaaaattr
x51 = a.b.c
x52 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
x53 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs
```