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`).
This commit is contained in:
Micha Reiser 2023-06-03 15:17:33 +02:00 committed by GitHub
parent e82160a83a
commit cb6788ab5f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 132 additions and 9 deletions

View file

@ -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));
}
}

View file

@ -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.
///

View file

@ -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,
},
],
},
}

View file

@ -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):

View file

@ -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

View file

@ -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"],

View file

@ -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
```

View file

@ -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))