From cb6788ab5fd1ccc1cfe8a81df013cd61eb755242 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 3 Jun 2023 15:17:33 +0200 Subject: [PATCH] Handle trailing body end-of-line comments (#4811) ### Summary This PR adds custom logic to handle end-of-line comments of the last statement in a body. For example: ```python while True: if something.changed: do.stuff() # trailing comment b ``` The `# trailing comment` is a trailing comment of the `do.stuff()` expression statement. We incorrectly attached the comment as a trailing comment of the enclosing `while` statement because the comment is between the end of the while statement (the `while` statement ends right after `do.stuff()`) and before the `b` statement. This PR fixes the placement to correctly attach these comments to the last statement in a body (recursively). ## Test Plan I reviewed the snapshots and they now look correct. This may appear odd because a lot comments have now disappeared. This is the expected result because we use `verbatim` formatting for the block statements (like `while`) and that means that it only formats the inner content of the block, but not any trailing comments. The comments were visible before, because they were associated with the block statement (e.g. `while`). --- .../ruff_python_formatter/src/comments/mod.rs | 14 ++++++ .../src/comments/placement.rs | 36 ++++++++++++++ ...s__while_trailing_end_of_line_comment.snap | 21 ++++++++ ...tter__tests__black_test__comments5_py.snap | 5 +- ...tter__tests__black_test__comments6_py.snap | 8 +-- ...atter__tests__black_test__fmtonoff_py.snap | 4 +- ...atter__tests__black_test__fmtskip6_py.snap | 49 +++++++++++++++++++ ...s__black_test__remove_await_parens_py.snap | 4 +- 8 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_trailing_end_of_line_comment.snap create mode 100644 crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtskip6_py.snap diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index fdc5ea2190..6792adfcd2 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -858,4 +858,18 @@ a = ( assert_debug_snapshot!(comments.debug(test_case.source_code)); } + + #[test] + fn while_trailing_end_of_line_comment() { + let source = r#"while True: + if something.changed: + do.stuff() # trailing comment +"#; + + let test_case = CommentsTestCase::from_code(source); + + let comments = test_case.to_comments(); + + assert_debug_snapshot!(comments.debug(test_case.source_code)); + } } diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index f689d7c127..23bc961c5f 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -18,6 +18,7 @@ pub(super) fn place_comment<'a>( .or_else(|comment| handle_match_comment(comment, locator)) .or_else(|comment| handle_in_between_bodies_comment(comment, locator)) .or_else(|comment| handle_trailing_body_comment(comment, locator)) + .or_else(handle_trailing_end_of_line_body_comment) .or_else(|comment| handle_positional_only_arguments_separator_comment(comment, locator)) .or_else(|comment| { handle_trailing_binary_expression_left_or_operator_comment(comment, locator) @@ -401,6 +402,41 @@ fn handle_trailing_body_comment<'a>( } } +/// Handles end of line comments of the last statement in an indented body: +/// +/// ```python +/// while True: +/// if something.changed: +/// do.stuff() # trailing comment +/// ``` +fn handle_trailing_end_of_line_body_comment(comment: DecoratedComment<'_>) -> CommentPlacement<'_> { + // Must be an end of line comment + if comment.text_position().is_own_line() { + return CommentPlacement::Default(comment); + } + + // Must be *after* a statement + let Some(preceding) = comment.preceding_node() else { + return CommentPlacement::Default(comment); + }; + + // Recursively get the last child of statements with a body. + let last_children = std::iter::successors(last_child_in_body(preceding), |parent| { + last_child_in_body(*parent) + }); + + if let Some(last_child) = last_children.last() { + CommentPlacement::trailing(last_child, comment) + } else { + // End of line comment of a statement that has no body. This is not what we're looking for. + // ```python + // a # trailing comment + // b + // ``` + CommentPlacement::Default(comment) + } +} + /// Attaches comments for the positional-only arguments separator `/` as trailing comments to the /// enclosing [`Arguments`] node. /// diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_trailing_end_of_line_comment.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_trailing_end_of_line_comment.snap new file mode 100644 index 0000000000..41e845c4b4 --- /dev/null +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__while_trailing_end_of_line_comment.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff_python_formatter/src/comments/mod.rs +expression: comments.debug(test_case.source_code) +--- +{ + Node { + kind: StmtExpr, + range: 46..56, + source: `do.stuff()`, + }: { + "leading": [], + "dangling": [], + "trailing": [ + SourceComment { + text: "# trailing comment", + position: EndOfLine, + formatted: false, + }, + ], + }, +} diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments5_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments5_py.snap index df1348d88f..7087a8c711 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments5_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments5_py.snap @@ -89,12 +89,13 @@ if __name__ == "__main__": @@ -1,11 +1,6 @@ while True: if something.changed: - do.stuff() # trailing comment +- do.stuff() # trailing comment - # Comment belongs to the `if` block. - # This one belongs to the `while` block. - - # Should this one, too? I guess so. - ++ do.stuff() # This one is properly standalone now. for i in range(100): @@ -161,7 +162,7 @@ if __name__ == "__main__": ```py while True: if something.changed: - do.stuff() # trailing comment + do.stuff() # This one is properly standalone now. for i in range(100): diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments6_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments6_py.snap index 24247fc646..6181d6489b 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments6_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__comments6_py.snap @@ -168,12 +168,14 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite def f( a, # type: int b, # type: int -@@ -67,22 +57,16 @@ +@@ -66,23 +56,17 @@ + + element + another_element + another_element_with_long_name - ) # type: int +- ) # type: int - - ++ ) def f( x, # not a type comment y, # type: int @@ -272,7 +274,7 @@ def f( + element + another_element + another_element_with_long_name - ) # type: int + ) def f( x, # not a type comment y, # type: int diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtonoff_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtonoff_py.snap index 362982ef50..1766ef2a6b 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtonoff_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtonoff_py.snap @@ -369,7 +369,7 @@ d={'a':1, + (1, 2, 3, 4), + (5, 6, 7, 8), + (9, 10, 11, 12) -+ } # yapf: disable ++ } cfg.rule( - "Default", - "address", @@ -544,7 +544,7 @@ def single_literal_yapf_disable(): (1, 2, 3, 4), (5, 6, 7, 8), (9, 10, 11, 12) - } # yapf: disable + } cfg.rule( "Default", "address", xxxx_xxxx=["xxx-xxxxxx-xxxxxxxxxx"], diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtskip6_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtskip6_py.snap new file mode 100644 index 0000000000..46b8920531 --- /dev/null +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__fmtskip6_py.snap @@ -0,0 +1,49 @@ +--- +source: crates/ruff_python_formatter/src/lib.rs +expression: snapshot +input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/fmtskip6.py +--- +## Input + +```py +class A: + def f(self): + for line in range(10): + if True: + pass # fmt: skip +``` + +## Black Differences + +```diff +--- Black ++++ Ruff +@@ -2,4 +2,4 @@ + def f(self): + for line in range(10): + if True: +- pass # fmt: skip ++ pass +``` + +## Ruff Output + +```py +class A: + def f(self): + for line in range(10): + if True: + pass +``` + +## Black Output + +```py +class A: + def f(self): + for line in range(10): + if True: + pass # fmt: skip +``` + + diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__remove_await_parens_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__remove_await_parens_py.snap index 62096f8534..603cab0375 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__remove_await_parens_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__remove_await_parens_py.snap @@ -143,7 +143,7 @@ async def main(): - + await ( + asyncio.sleep(1) -+ ) # Hello ++ ) # Long lines async def main(): - await asyncio.gather( @@ -251,7 +251,7 @@ async def main(): async def main(): await ( asyncio.sleep(1) - ) # Hello + ) # Long lines async def main(): await asyncio.gather(asyncio.sleep(1), asyncio.sleep(1), asyncio.sleep(1), asyncio.sleep(1), asyncio.sleep(1), asyncio.sleep(1), asyncio.sleep(1))