Fix formatter instability with half-indented comment (#6460)

## Summary
The bug was happening in this
[loop](75f402eb82/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](75f402eb82/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 <konstin@mailbox.org>
This commit is contained in:
Victor Hugo Gomes 2023-08-11 08:21:16 -03:00 committed by GitHub
parent 0ef6af807b
commit b05574babd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 79 additions and 26 deletions

View file

@ -103,3 +103,15 @@ else:
if True: print("a") # 1 if True: print("a") # 1
elif True: print("b") # 2 elif True: print("b") # 2
else: print("c") # 3 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

View file

@ -600,8 +600,13 @@ def test(x, y):
def other(y, z): def other(y, z):
if y == z: if y == z:
pass pass
# Trailing `if` comment # Trailing `pass` comment
# Trailing `other` function comment # Trailing `if` statement comment
class Test:
def func():
pass
# Trailing `func` function comment
test(10, 20) test(10, 20)
"#; "#;

View file

@ -451,12 +451,11 @@ fn handle_own_line_comment_after_branch<'a>(
return CommentPlacement::Default(comment); return CommentPlacement::Default(comment);
} }
let mut parent_body = None; let mut parent = None;
let mut current_body = Some(preceding_node); let mut last_child_in_parent = last_child;
let mut last_child_in_current_body = last_child;
loop { loop {
let child_indentation = indentation(locator, &last_child_in_current_body) let child_indentation = indentation(locator, &last_child_in_parent)
.unwrap_or_default() .unwrap_or_default()
.len(); .len();
@ -465,15 +464,16 @@ fn handle_own_line_comment_after_branch<'a>(
// if parent_body: // if parent_body:
// if current_body: // if current_body:
// child_in_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: Comment belongs to the parent block.
// # less // # less: Comment belongs to the parent block.
// # equal: The comment belongs to this block. // # equal: The comment belongs to this block.
// # greater // # greater (but less in the next iteration)
// # greater: The comment belongs to the inner block. // # greater: The comment belongs to the inner block.
// ```
match comment_indentation.cmp(&child_indentation) { match comment_indentation.cmp(&child_indentation) {
Ordering::Less => { Ordering::Less => {
return if let Some(parent_block) = parent_body { return if let Some(parent_block) = parent {
// Comment belongs to the parent block. // Comment belongs to the parent block.
CommentPlacement::trailing(parent_block, comment) CommentPlacement::trailing(parent_block, comment)
} else { } else {
@ -488,14 +488,13 @@ fn handle_own_line_comment_after_branch<'a>(
} }
Ordering::Equal => { Ordering::Equal => {
// The comment belongs to this block. // 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 => { 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. // The comment belongs to the inner block.
parent_body = current_body; parent = Some(last_child_in_parent);
current_body = Some(last_child_in_current_body); last_child_in_parent = nested_child;
last_child_in_current_body = nested_child;
} else { } else {
// The comment is overindented, we assign it to the most indented child we have. // The comment is overindented, we assign it to the most indented child we have.
// ```python // ```python
@ -503,7 +502,7 @@ fn handle_own_line_comment_after_branch<'a>(
// pass // pass
// # comment // # 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())) 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<AnyNodeRef> { fn last_child_in_body(node: AnyNodeRef) -> Option<AnyNodeRef> {
let body = match node { let body = match node {
AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. }) AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. })
@ -1504,9 +1503,7 @@ mod tests {
); );
assert_eq!( assert_eq!(
max_empty_lines( max_empty_lines("# trailing comment\n\n# own line comment\n\n\n# an other own line comment\n# block"),
"# trailing comment\n\n# own line comment\n\n\n# an other own line comment\n# block"
),
2 2
); );

View file

@ -34,15 +34,15 @@ expression: comments.debug(test_case.source_code)
], ],
}, },
Node { Node {
kind: StmtFunctionDef, kind: StmtIf,
range: 193..237, range: 214..237,
source: `def other(y, z):⏎`, source: `if y == z:⏎`,
}: { }: {
"leading": [], "leading": [],
"dangling": [], "dangling": [],
"trailing": [ "trailing": [
SourceComment { SourceComment {
text: "# Trailing `other` function comment", text: "# Trailing `if` statement comment",
position: OwnLine, position: OwnLine,
formatted: false, formatted: false,
}, },
@ -57,7 +57,22 @@ expression: comments.debug(test_case.source_code)
"dangling": [], "dangling": [],
"trailing": [ "trailing": [
SourceComment { 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, position: OwnLine,
formatted: false, formatted: false,
}, },

View file

@ -109,6 +109,18 @@ else:
if True: print("a") # 1 if True: print("a") # 1
elif True: print("b") # 2 elif True: print("b") # 2
else: print("c") # 3 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 ## Output
@ -224,6 +236,18 @@ elif True:
print("b") # 2 print("b") # 2
else: else:
print("c") # 3 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
``` ```