Fix a number of formatter errors from the cpython repository (#5089)

## Summary

This fixes a number of problems in the formatter that showed up with
various files in the [cpython](https://github.com/python/cpython)
repository. These problems surfaced as unstable formatting and invalid
code. This is not the entirety of problems discovered through cpython,
but a big enough chunk to separate it. Individual fixes are generally
individual commits. They were discovered with #5055, which i update as i
work through the output

## Test Plan

I added regression tests with links to cpython for each entry, except
for the two stubs that also got comment stubs since they'll be
implemented properly later.
This commit is contained in:
konstin 2023-06-15 13:24:14 +02:00 committed by GitHub
parent 097823b56d
commit 66089e1a2e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
25 changed files with 234 additions and 23 deletions

View file

@ -332,7 +332,20 @@ fn handle_in_between_bodies_end_of_line_comment<'a>(
// else:
// b
// ```
CommentPlacement::trailing(preceding, comment)
if preceding.is_node_with_body() {
// We can't set this as a trailing comment of the function declaration because it
// will then move behind the function block instead of sticking with the pass
// ```python
// if True:
// def f():
// pass # a
// else:
// pass
// ```
CommentPlacement::Default(comment)
} else {
CommentPlacement::trailing(preceding, comment)
}
} else if following.is_stmt_if() || following.is_except_handler() {
// The `elif` or except handlers have their own body to which we can attach the trailing comment
// ```python
@ -837,7 +850,12 @@ fn find_pos_only_slash_offset(
return Some(maybe_slash.start());
}
debug_assert_eq!(maybe_slash.kind(), TokenKind::RParen);
debug_assert_eq!(
maybe_slash.kind(),
TokenKind::RParen,
"{:?}",
maybe_slash.kind()
);
}
}

View file

@ -39,6 +39,22 @@ impl FormatNodeRule<ExprConstant> for FormatExprConstant {
}
}
}
fn fmt_dangling_comments(
&self,
_node: &ExprConstant,
_f: &mut PyFormatter,
) -> FormatResult<()> {
// TODO(konstin): Reactivate when string formatting works, currently a source of unstable
// formatting, e.g.:
// magic_methods = (
// "enter exit "
// # we added divmod and rdivmod here instead of numerics
// # because there is no idivmod
// "divmod rdivmod neg pos abs invert "
// )
Ok(())
}
}
impl NeedsParentheses for ExprConstant {

View file

@ -18,6 +18,17 @@ impl FormatNodeRule<ExprDict> for FormatExprDict {
)]
)
}
fn fmt_dangling_comments(&self, _node: &ExprDict, _f: &mut PyFormatter) -> FormatResult<()> {
// TODO(konstin): Reactivate when string formatting works, currently a source of unstable
// formatting, e.g.
// ```python
// coverage_ignore_c_items = {
// # 'cfunction': [...]
// }
// ```
Ok(())
}
}
impl NeedsParentheses for ExprDict {

View file

@ -1,4 +1,4 @@
use crate::comments::{dangling_comments, Comments};
use crate::comments::{dangling_comments, CommentTextPosition, Comments};
use crate::expression::parentheses::{
default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize,
};
@ -18,6 +18,39 @@ impl FormatNodeRule<ExprList> for FormatExprList {
ctx: _,
} = item;
let comments = f.context().comments().clone();
let dangling = comments.dangling_comments(item.into());
// The empty list is special because there can be dangling comments, and they can be in two
// positions:
// ```python
// a3 = [ # end-of-line
// # own line
// ]
// ```
// In all other cases comments get assigned to a list element
if elts.is_empty() {
let end_of_line_split = dangling
.partition_point(|comment| comment.position() == CommentTextPosition::EndOfLine);
debug_assert!(dangling[end_of_line_split..]
.iter()
.all(|comment| comment.position() == CommentTextPosition::OwnLine));
return write!(
f,
[group(&format_args![
text("["),
dangling_comments(&dangling[..end_of_line_split]),
soft_block_indent(&dangling_comments(&dangling[end_of_line_split..])),
text("]")
])]
);
}
debug_assert!(
dangling.is_empty(),
"A non-empty expression list has dangling comments"
);
let items = format_with(|f| {
let mut iter = elts.iter();
@ -36,14 +69,10 @@ impl FormatNodeRule<ExprList> for FormatExprList {
Ok(())
});
let comments = f.context().comments().clone();
let dangling = comments.dangling_comments(item.into());
write!(
f,
[group(&format_args![
text("["),
dangling_comments(dangling),
soft_block_indent(&items),
text("]")
])]

View file

@ -58,6 +58,9 @@ impl FormatNodeRule<Arguments> for FormatArguments {
last_node = Some(default.map_or_else(|| argument.into(), AnyNodeRef::from));
}
// kw only args need either a `*args` ahead of them capturing all var args or a `*`
// pseudo-argument capturing all fields. We can also have `*args` without any kwargs
// afterwards.
if let Some(vararg) = vararg {
joiner.entry(&format_args![
leading_node_comments(vararg.as_ref()),
@ -65,6 +68,8 @@ impl FormatNodeRule<Arguments> for FormatArguments {
vararg.format()
]);
last_node = Some(vararg.as_any_node_ref());
} else if !kwonlyargs.is_empty() {
joiner.entry(&text("*"));
}
debug_assert!(defaults.next().is_none());

View file

@ -222,7 +222,7 @@ d={'a':1,
# Comment 1
# Comment 2
@@ -18,109 +16,111 @@
@@ -18,109 +16,112 @@
# fmt: off
def func_no_args():
@ -269,6 +269,7 @@ d={'a':1,
+ number: int,
+ no_annotation=None,
+ text: str = "NOT_YET_IMPLEMENTED_STRING",
+ *,
+ debug: bool = False,
+ **kwargs,
+) -> str:
@ -393,7 +394,7 @@ d={'a':1,
# fmt: off
# hey, that won't work
@@ -130,13 +130,15 @@
@@ -130,13 +131,15 @@
def on_and_off_broken():
@ -414,7 +415,7 @@ d={'a':1,
# fmt: on
# fmt: off
# ...but comments still get reformatted even though they should not be
@@ -145,80 +147,21 @@
@@ -145,80 +148,21 @@
def long_lines():
if True:
@ -426,13 +427,11 @@ d={'a':1,
- implicit_default=True,
- )
- )
+ NOT_IMPLEMENTED_call()
# fmt: off
- # fmt: off
- a = (
- unnecessary_bracket()
- )
+ a = NOT_IMPLEMENTED_call()
# fmt: on
- # fmt: on
- _type_comment_re = re.compile(
- r"""
- ^
@ -453,9 +452,11 @@ d={'a':1,
- )
- $
- """,
- # fmt: off
+ NOT_IMPLEMENTED_call()
# fmt: off
- re.MULTILINE|re.VERBOSE
- # fmt: on
+ a = NOT_IMPLEMENTED_call()
# fmt: on
- )
+ _type_comment_re = NOT_IMPLEMENTED_call()
@ -550,6 +551,7 @@ def function_signature_stress_test(
number: int,
no_annotation=None,
text: str = "NOT_YET_IMPLEMENTED_STRING",
*,
debug: bool = False,
**kwargs,
) -> str:

View file

@ -126,7 +126,7 @@ def __await__(): return (yield)
def func_no_args():
@@ -14,135 +13,86 @@
@@ -14,135 +13,87 @@
b
c
if True:
@ -161,8 +161,8 @@ def __await__(): return (yield)
number: int,
no_annotation=None,
- text: str = "default",
- *,
+ text: str = "NOT_YET_IMPLEMENTED_STRING",
*,
debug: bool = False,
**kwargs,
) -> str:
@ -339,6 +339,7 @@ def function_signature_stress_test(
number: int,
no_annotation=None,
text: str = "NOT_YET_IMPLEMENTED_STRING",
*,
debug: bool = False,
**kwargs,
) -> str:

View file

@ -0,0 +1,35 @@
---
source: crates/ruff_python_formatter/src/lib.rs
expression: snapshot
---
## Input
```py
# Dangling comment placement in empty lists
# Regression test for https://github.com/python/cpython/blob/03160630319ca26dcbbad65225da4248e54c45ec/Tools/c-analyzer/c_analyzer/datafiles.py#L14-L16
a1 = [ # a
]
a2 = [ # a
# b
]
a3 = [
# b
]
```
## Output
```py
# Dangling comment placement in empty lists
# Regression test for https://github.com/python/cpython/blob/03160630319ca26dcbbad65225da4248e54c45ec/Tools/c-analyzer/c_analyzer/datafiles.py#L14-L16
a1 = [ # a
]
a2 = [ # a
# b
]
a3 = [
# b
]
```

View file

@ -74,6 +74,28 @@ def test(): ...
# Comment
def with_leading_comment(): ...
# Comment that could be mistaken for a trailing comment of the function declaration when
# looking from the position of the if
# Regression test for https://github.com/python/cpython/blob/ad56340b665c5d8ac1f318964f71697bba41acb7/Lib/logging/__init__.py#L253-L260
if True:
def f1():
pass # a
else:
pass
# Here it's actually a trailing comment
if True:
def f2():
pass
# a
else:
pass
# Make sure the star is printed
# Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513
def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2):
pass
```
@ -177,6 +199,30 @@ def test():
# Comment
def with_leading_comment():
...
# Comment that could be mistaken for a trailing comment of the function declaration when
# looking from the position of the if
# Regression test for https://github.com/python/cpython/blob/ad56340b665c5d8ac1f318964f71697bba41acb7/Lib/logging/__init__.py#L253-L260
if True:
def f1():
pass # a
else:
pass
# Here it's actually a trailing comment
if True:
def f2():
pass
# a
else:
pass
# Make sure the star is printed
# Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513
def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2):
pass
```

View file

@ -1,6 +1,6 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: tokens
expression: test_case.tokens()
---
[
Token {

View file

@ -1,6 +1,6 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: tokens
expression: test_case.tokens()
---
[
Token {

View file

@ -1,6 +1,6 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: tokens
expression: test_case.tokens()
---
[
Token {

View file

@ -1,6 +1,6 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: tokens
expression: test_case.tokens()
---
[
Token {

View file

@ -1,6 +1,6 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: tokens
expression: test_case.tokens()
---
[
Token {