Allow return type annotations to use their own parentheses#6436
Allow return type annotations to use their own parentheses#6436charliermarsh merged 7 commits intomainfrom
Conversation
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
232dea5 to
b06831e
Compare
|
This seems to do the right thing, but I'm already anticipating that I'm misunderstanding something around why / how Black makes it decisions, and the relationship to our grouping and parenthesization logic. |
|
(Looks like I have one failing stability test.) |
crates/ruff_python_formatter/src/statement/stmt_function_def.rs
Outdated
Show resolved
Hide resolved
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
a1a6764 to
a44080a
Compare
PR Check ResultsBenchmarkLinuxWindows |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This is interesting and I wonder how intentional the behavior is. I tried to deduce the behavior from Black's source code. I can confirm that the behavior is different if the function has no arguments, but I haven't yet found the place where the branching happens:
- Black uses
left_hand_splitfor function definitions (rather than the usualrhs) [source] - For empty: It forces the optional parentheses of the return type and applies the default transforms for parenthesized expressions (
delimiter_split,standalone_comment_split,rhs). This matchesoptional_parentheses - Non empty: It splits after the
(of the arguments and then applies the default transform for unparenthesized expressions (rhs,hug_power_op). This matchesmabye_parenthesizewithParenthesize::IfBreaks
crates/ruff_python_formatter/src/statement/stmt_function_def.rs
Outdated
Show resolved
Hide resolved
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
a44080a to
10aef8e
Compare
|
(I need to figure out the instability here, then will re-request a review.) |
|
Having trouble with this, looking for help. Given: def double(a: int) -> (
int # Hello
):
return 2*aWe first format as: def double(
a: int
) -> int: # Hello
return 2 * aThis is because we format When we reformat, we get: def double(a: int) -> int: # Hello
return 2 * aSince the Is there any way for me to avoid the breaking in the first format? (This does get fixed by #6464 since we don't incorrectly break, but perhaps we won't end up merging that.) |
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
10aef8e to
6e263eb
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Let's remove the comment formatting changes.
|
We can certainly remove the comment formatting changes, but it then requires coming up with a strategy for resolving the instability that I documented above, which either requires further deviating from Black or changing the groups in a way that I don’t fully understand. |
|
One option would be to group the let group_function_parameters = item
.returns
.as_deref()
.is_some_and(|return_type| comments.has_trailing_comments(return_type));
if group_function_parameters {
write!(f, [group(&item.parameters.format())])?;
} else {
write!(f, [item.parameters.format()])?;
} |
|
Never, this doesn't work (for the opposite reason): def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(
x, b, c, d
) -> (Set[
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
] # test
):
...
# Pass 1
def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x, b, c, d) -> Set[
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
]: # test
...
# Pass 2
def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(
x, b, c, d
) -> Set[
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
]: # test
... |
|
The easiest might be to group parameters if the return type doesn't have its own breakpoint and has a trailing comment. The list should be rather short because we know that we're in a return-type position. Rome has a similar function |
|
(Trying this.) |
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
6e263eb to
2d8650c
Compare
|
Another deviation is that I could decide to preserve the parentheses if a trailing comment is present there, e.g., make this stable formatting: def double(
a: int
) -> (
int # Hello
):
pass(I haven't looked into how difficult this would be, but I assume it would be easier than what I'm trying to do here.) |
crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py
Outdated
Show resolved
Hide resolved
|
Can you push your latest change so that I can play with them myself? |
|
I just pushed. If simpler, you can also remove the diff --git a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs
index d47825cc2..39025b11c 100644
--- a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs
+++ b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs
@@ -60,15 +60,7 @@ impl FormatNodeRule<StmtFunctionDef> for FormatStmtFunctionDef {
}
let format_inner = format_with(|f: &mut PyFormatter| {
- if should_group_function_parameters(
- &item.parameters,
- item.returns.as_deref(),
- f.context(),
- ) {
- write!(f, [group(&item.parameters.format())])?;
- } else {
- write!(f, [item.parameters.format()])?;
- }
+ write!(f, [item.parameters.format()])?;
if let Some(return_annotation) = item.returns.as_ref() {
write!(f, [space(), text("->"), space()])?; |
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
3 times, most recently
from
3c0b569 to
b6eba55
Compare
|
The current version deviates from Black, but it's at least stable... Specifically, we now preserve formatting like: def double(
a: int
) -> (
int # Hello
):
passI think this is reasonable but perhaps not ideal. |
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
2 times, most recently
from
3d96db4 to
a17be4f
Compare
| | Parenthesize::IfBreaksOrIfRequired => needs_parentheses, | ||
| }; | ||
|
|
||
| match needs_parentheses { |
There was a problem hiding this comment.
This is a bit less DRY but I find it much easier to reason about the match when every OptionalParentheses has a single branch.
|
I think this formatting is arguably more consistent for us, since we format this: def double(
a # comment
):
return 2 * aAs: def double(
a, # comment
):
return 2 * aThat is, we don't push those out to after the function definition (unlike Black). |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I like the outcome. It even preserves the authors comment choice:
e.g. I understand that we would keep
def test() -> (
[a, b ] # noqa: Ruf01
): pass
crates/ruff_python_formatter/src/statement/stmt_function_def.rs
Outdated
Show resolved
Hide resolved
charliermarsh
force-pushed
the
charlie/function-returns-wrap
branch
from
a17be4f to
f0b5447
Compare
…#6436) ## Summary This PR modifies our logic for wrapping return type annotations. Previously, we _always_ wrapped the annotation in parentheses if it expanded; however, Black only exhibits this behavior when the function parameters is empty (i.e., it doesn't and can't break). In other cases, it uses the normal parenthesization rules, allowing nodes to bring their own parentheses. For example, given: ```python def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ]: ... def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ]: ... ``` Black will format as: ```python def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> ( Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ] ): ... def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( x, ) -> Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ]: ... ``` Whereas, prior to this PR, Ruff would format as: ```python def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> ( Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ] ): ... def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( x, ) -> ( Set[ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ] ): ... ``` Closes astral-sh#6431. ## Test Plan Before: - `zulip`: 0.99702 - `django`: 0.99784 - `warehouse`: 0.99585 - `build`: 0.75623 - `transformers`: 0.99470 - `cpython`: 0.75988 - `typeshed`: 0.74853 After: - `zulip`: 0.99724 - `django`: 0.99791 - `warehouse`: 0.99586 - `build`: 0.75623 - `transformers`: 0.99474 - `cpython`: 0.75956 - `typeshed`: 0.74857
3 participants
Summary
This PR modifies our logic for wrapping return type annotations. Previously, we always wrapped the annotation in parentheses if it expanded; however, Black only exhibits this behavior when the function parameters is empty (i.e., it doesn't and can't break). In other cases, it uses the normal parenthesization rules, allowing nodes to bring their own parentheses.
For example, given:
Black will format as:
Whereas, prior to this PR, Ruff would format as:
Closes #6431.
Test Plan
Before:
zulip: 0.99702django: 0.99784warehouse: 0.99585build: 0.75623transformers: 0.99470cpython: 0.75988typeshed: 0.74853After:
zulip: 0.99724django: 0.99791warehouse: 0.99586build: 0.75623transformers: 0.99474cpython: 0.75956typeshed: 0.74857