From a52cd47c7fbfb4cd161c1ead905a1314c3707c92 Mon Sep 17 00:00:00 2001 From: konstin Date: Mon, 26 Jun 2023 11:13:07 +0200 Subject: [PATCH] Fix attribute chain own line comments (#5340) ## Motation Previously, ```python x = ( a1 .a2 # a . # b # c a3 ) ``` got formatted as ```python x = a1.a2 # a . # b # c a3 ``` which is invalid syntax. This fixes that. ## Summary This implements a basic form of attribute chaining () by checking if any inner attribute access contains an own line comment, and if this is the case, adds parentheses around the outermost attribute access while disabling parentheses for all inner attribute expressions. We want to replace this with an implementation that uses recursion or a stack while formatting instead of in `needs_parentheses` and also includes calls rather sooner than later, but i'm fixing this now because i'm uncomfortable with having known invalid syntax generation in the formatter. ## Test Plan I added new fixtures. --- .../fixtures/ruff/expression/attribute.py | 76 +++++++++- .../src/comments/placement.rs | 14 +- .../src/expression/expr_attribute.rs | 53 +++++-- .../format@expression__attribute.py.snap | 133 +++++++++++++++++- 4 files changed, 258 insertions(+), 18 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py index bdc18f4696..24bea5ca42 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py @@ -1,3 +1,7 @@ +from argparse import Namespace + +a = Namespace() + ( a # comment @@ -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 +) + diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index ca0fec261e..0e801c7096 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -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) } diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index fd43e9f20a..16ccaa330c 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -27,25 +27,28 @@ impl FormatNodeRule for FormatExprAttribute { }) ); - if needs_parentheses { - value.format().with_options(Parenthesize::Always).fmt(f)?; - } else { - 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); + 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 { + value.format().fmt(f)?; + } + + if comments.has_trailing_own_line_comments(value.as_ref()) { + hard_line_break().fmt(f)?; + } + write!( f, [ @@ -68,6 +71,28 @@ impl FormatNodeRule 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, @@ -75,6 +100,10 @@ impl NeedsParentheses for ExprAttribute { 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, diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap index 82792c7d3c..a1871573c8 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap @@ -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 @@ -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 @@ -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 ) -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 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 + +x51 = a.b.c +x52 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs +x53 = a.askjdfahdlskjflsajfadhsaf.akjdsf.aksjdlfadhaljsashdfljaf.askjdflhasfdlashdlfaskjfd.asdkjfksahdfkjafs ```