From b05574babd6224af99938a429d0b19ba3297513c Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Fri, 11 Aug 2023 08:21:16 -0300 Subject: [PATCH] Fix formatter instability with half-indented comment (#6460) ## Summary The bug was happening in this [loop](https://github.com/LaBatata101/ruff/blob/75f402eb8262f88dc0445beb6cab4d3cd224716c/crates/ruff_python_formatter/src/comments/placement.rs#L545). Basically, In the first iteration of the loop, the `comment_indentation` is bigger than `child_indentation` (`comment_indentation` is 7 and `child_indentation` is 4) making the `Ordering::Greater` branch execute. Inside the `Ordering::Greater` branch, the `if` block gets executed, resulting in the update of these variables. ```rust parent_body = current_body; current_body = Some(last_child_in_current_body); last_child_in_current_body = nested_child; ``` In the second iteration of the loop, `comment_indentation` is smaller than `child_indentation` (`comment_indentation` is 7 and `child_indentation` is 8) making the `Ordering::Less` branch execute. Inside the `Ordering::Less` branch, the `if` block gets executed, this is where the bug was happening. At this point `parent_body` should be a `StmtFunctionDef` but it was a `StmtClassDef`. Causing the comment to be incorrectly formatted. That happened for the following code: ```python class A: def f(): pass # strangely indented comment print() ``` There is only one problem that I couldn't figure it out a solution, the variable `current_body` in this [line](https://github.com/LaBatata101/ruff/blob/75f402eb8262f88dc0445beb6cab4d3cd224716c/crates/ruff_python_formatter/src/comments/placement.rs#L542C5-L542C49) now gives this warning _"value assigned to `current_body` is never read maybe it is overwritten before being read?"_ Any tips on how to solve that? Closes #5337 ## Test Plan Add new test case. --------- Co-authored-by: konstin --- .../test/fixtures/ruff/statement/if.py | 12 +++++++ .../ruff_python_formatter/src/comments/mod.rs | 9 +++-- .../src/comments/placement.rs | 35 +++++++++---------- ...nts__tests__trailing_function_comment.snap | 25 ++++++++++--- .../snapshots/format@statement__if.py.snap | 24 +++++++++++++ 5 files changed, 79 insertions(+), 26 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py index 266b2da8a7..61e3bc130b 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py @@ -103,3 +103,15 @@ else: if True: print("a") # 1 elif True: print("b") # 2 else: print("c") # 3 + +# Regression test for https://github.com/astral-sh/ruff/issues/5337 +if parent_body: + if current_body: + child_in_body() + last_child_in_current_body() # may or may not have children on its own +# a + # b + # c + # d + # e + #f diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index ae855c82b6..238665f9b7 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -600,8 +600,13 @@ def test(x, y): def other(y, z): if y == z: pass - # Trailing `if` comment - # Trailing `other` function comment + # Trailing `pass` comment + # Trailing `if` statement comment + +class Test: + def func(): + pass + # Trailing `func` function comment test(10, 20) "#; diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index a231a54096..81035bf7dd 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -451,12 +451,11 @@ fn handle_own_line_comment_after_branch<'a>( return CommentPlacement::Default(comment); } - let mut parent_body = None; - let mut current_body = Some(preceding_node); - let mut last_child_in_current_body = last_child; + let mut parent = None; + let mut last_child_in_parent = last_child; loop { - let child_indentation = indentation(locator, &last_child_in_current_body) + let child_indentation = indentation(locator, &last_child_in_parent) .unwrap_or_default() .len(); @@ -465,15 +464,16 @@ fn handle_own_line_comment_after_branch<'a>( // if parent_body: // if current_body: // child_in_body() - // last_child_in_current_body # may or may not have children on its own + // last_child_in_current_body # may or may not have children on its own // # less: Comment belongs to the parent block. - // # less + // # less: Comment belongs to the parent block. // # equal: The comment belongs to this block. - // # greater - // # greater: The comment belongs to the inner block. + // # greater (but less in the next iteration) + // # greater: The comment belongs to the inner block. + // ``` match comment_indentation.cmp(&child_indentation) { Ordering::Less => { - return if let Some(parent_block) = parent_body { + return if let Some(parent_block) = parent { // Comment belongs to the parent block. CommentPlacement::trailing(parent_block, comment) } else { @@ -488,14 +488,13 @@ fn handle_own_line_comment_after_branch<'a>( } Ordering::Equal => { // The comment belongs to this block. - return CommentPlacement::trailing(last_child_in_current_body, comment); + return CommentPlacement::trailing(last_child_in_parent, comment); } Ordering::Greater => { - if let Some(nested_child) = last_child_in_body(last_child_in_current_body) { + if let Some(nested_child) = last_child_in_body(last_child_in_parent) { // The comment belongs to the inner block. - parent_body = current_body; - current_body = Some(last_child_in_current_body); - last_child_in_current_body = nested_child; + parent = Some(last_child_in_parent); + last_child_in_parent = nested_child; } else { // The comment is overindented, we assign it to the most indented child we have. // ```python @@ -503,7 +502,7 @@ fn handle_own_line_comment_after_branch<'a>( // pass // # comment // ``` - return CommentPlacement::trailing(last_child_in_current_body, comment); + return CommentPlacement::trailing(last_child_in_parent, comment); } } } @@ -1346,7 +1345,7 @@ where right.is_some_and(|right| left.ptr_eq(right.into())) } -/// The last child of the last branch, if the node hs multiple branches. +/// The last child of the last branch, if the node has multiple branches. fn last_child_in_body(node: AnyNodeRef) -> Option { let body = match node { AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. }) @@ -1504,9 +1503,7 @@ mod tests { ); assert_eq!( - max_empty_lines( - "# trailing comment\n\n# own line comment\n\n\n# an other own line comment\n# block" - ), + max_empty_lines("# trailing comment\n\n# own line comment\n\n\n# an other own line comment\n# block"), 2 ); diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_function_comment.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_function_comment.snap index 005e4fcf8d..9dd6d289d4 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_function_comment.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_function_comment.snap @@ -34,15 +34,15 @@ expression: comments.debug(test_case.source_code) ], }, Node { - kind: StmtFunctionDef, - range: 193..237, - source: `def other(y, z):⏎`, + kind: StmtIf, + range: 214..237, + source: `if y == z:⏎`, }: { "leading": [], "dangling": [], "trailing": [ SourceComment { - text: "# Trailing `other` function comment", + text: "# Trailing `if` statement comment", position: OwnLine, formatted: false, }, @@ -57,7 +57,22 @@ expression: comments.debug(test_case.source_code) "dangling": [], "trailing": [ SourceComment { - text: "# Trailing `if` comment", + text: "# Trailing `pass` comment", + position: OwnLine, + formatted: false, + }, + ], + }, + Node { + kind: StmtFunctionDef, + range: 333..357, + source: `def func():⏎`, + }: { + "leading": [], + "dangling": [], + "trailing": [ + SourceComment { + text: "# Trailing `func` function comment", position: OwnLine, formatted: false, }, diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap index 10ab45b872..9c459694dd 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap @@ -109,6 +109,18 @@ else: if True: print("a") # 1 elif True: print("b") # 2 else: print("c") # 3 + +# Regression test for https://github.com/astral-sh/ruff/issues/5337 +if parent_body: + if current_body: + child_in_body() + last_child_in_current_body() # may or may not have children on its own +# a + # b + # c + # d + # e + #f ``` ## Output @@ -224,6 +236,18 @@ elif True: print("b") # 2 else: print("c") # 3 + +# Regression test for https://github.com/astral-sh/ruff/issues/5337 +if parent_body: + if current_body: + child_in_body() + last_child_in_current_body() # may or may not have children on its own + # e + # f + # c + # d +# a +# b ```