From 3bf1c66cdae3915c4945099bd03ef9da7fb06b2d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 9 Aug 2023 08:13:58 -0400 Subject: [PATCH] Group function definition parameters with return type annotations (#6410) ## Summary This PR removes the group around function definition parameters, instead grouping the parameters with the type parameters and return type annotation. This increases Zulip's similarity score from 0.99385 to 0.99699, so it's a meaningful improvement. However, there's at least one stability error that I'm working on, and I'm really just looking for high-level feedback at this point, because I'm not happy with the solution. Closes https://github.com/astral-sh/ruff/issues/6352. ## Test Plan Before: - `zulip`: 0.99396 - `django`: 0.99784 - `warehouse`: 0.99578 - `build`: 0.75436 - `transformers`: 0.99407 - `cpython`: 0.75987 - `typeshed`: 0.74432 After: - `zulip`: 0.99702 - `django`: 0.99784 - `warehouse`: 0.99585 - `build`: 0.75623 - `transformers`: 0.99470 - `cpython`: 0.75988 - `typeshed`: 0.74853 --- .../test/fixtures/ruff/statement/function.py | 26 ++++++ .../src/other/parameters.rs | 18 +++-- .../src/statement/stmt_function_def.rs | 38 +++++---- ..._cases__return_annotation_brackets.py.snap | 12 ++- .../format@statement__function.py.snap | 80 ++++++++++++++++++- 5 files changed, 147 insertions(+), 27 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py index e59547d6ce..d242751143 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py @@ -409,3 +409,29 @@ def double(a: int) -> ( # Hello def double(a: int) -> ( # Hello ): return 2*a + +# Breaking over parameters and return types. (Black adds a trailing comma when the +# function arguments break here with a single argument; we do not.) +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a: + ... + +def f(a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> a: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a: + ... diff --git a/crates/ruff_python_formatter/src/other/parameters.rs b/crates/ruff_python_formatter/src/other/parameters.rs index cbd9b594b7..34696fbe52 100644 --- a/crates/ruff_python_formatter/src/other/parameters.rs +++ b/crates/ruff_python_formatter/src/other/parameters.rs @@ -7,14 +7,15 @@ use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{TextRange, TextSize}; use crate::comments::{ - leading_comments, leading_node_comments, trailing_comments, CommentLinePosition, SourceComment, + dangling_open_parenthesis_comments, leading_comments, leading_node_comments, trailing_comments, + CommentLinePosition, SourceComment, }; use crate::context::{NodeLevel, WithNodeLevel}; -use crate::expression::parentheses::{empty_parenthesized, parenthesized}; +use crate::expression::parentheses::empty_parenthesized; use crate::prelude::*; use crate::FormatNodeRule; -#[derive(Eq, PartialEq, Debug, Default)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Default)] pub enum ParametersParentheses { /// By default, parameters will always preserve their surrounding parentheses. #[default] @@ -246,10 +247,17 @@ impl FormatNodeRule for FormatParameters { // No parameters, format any dangling comments between `()` write!(f, [empty_parenthesized("(", dangling, ")")]) } else { + // Intentionally avoid `parenthesized`, which groups the entire formatted contents. + // We want parameters to be grouped alongside return types, one level up, so we + // format them "inline" here. write!( f, - [parenthesized("(", &group(&format_inner), ")") - .with_dangling_comments(parenthesis_dangling)] + [ + text("("), + dangling_open_parenthesis_comments(parenthesis_dangling), + soft_block_indent(&group(&format_inner)), + text(")") + ] ) } } 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 4533a6012c..746b69de42 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs @@ -58,24 +58,28 @@ impl FormatNodeRule for FormatStmtFunctionDef { write!(f, [type_params.format()])?; } - write!(f, [item.parameters.format()])?; - - if let Some(return_annotation) = item.returns.as_ref() { - write!(f, [space(), text("->"), space()])?; - if return_annotation.is_tuple_expr() { - write!( - f, - [return_annotation.format().with_options(Parentheses::Never)] - )?; - } else { - write!( - f, - [optional_parentheses( - &return_annotation.format().with_options(Parentheses::Never), - )] - )?; + let format_inner = format_with(|f: &mut PyFormatter| { + write!(f, [item.parameters.format()])?; + if let Some(return_annotation) = item.returns.as_ref() { + write!(f, [space(), text("->"), space()])?; + if return_annotation.is_tuple_expr() { + write!( + f, + [return_annotation.format().with_options(Parentheses::Never)] + )?; + } else { + write!( + f, + [optional_parentheses( + &return_annotation.format().with_options(Parentheses::Never), + )] + )?; + } } - } + Ok(()) + }); + + write!(f, [group(&format_inner)])?; write!( f, diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__return_annotation_brackets.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__return_annotation_brackets.py.snap index c35111a8b1..36e4656eb0 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__return_annotation_brackets.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__return_annotation_brackets.py.snap @@ -100,18 +100,20 @@ def foo() -> tuple[int, int, int,]: ```diff --- Black +++ Ruff -@@ -26,7 +26,9 @@ +@@ -26,7 +26,11 @@ return 2 * a -def double(a: int) -> int: # Hello -+def double(a: int) -> ( ++def double( ++ a: int ++) -> ( + int # Hello +): return 2 * a -@@ -54,7 +56,9 @@ +@@ -54,7 +58,9 @@ a: int, b: int, c: int, @@ -155,7 +157,9 @@ def double(a: int) -> int: # Hello return 2 * a -def double(a: int) -> ( +def double( + a: int +) -> ( int # Hello ): return 2 * a diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap index 7868120be5..d9f8a55ccf 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap @@ -415,6 +415,32 @@ def double(a: int) -> ( # Hello def double(a: int) -> ( # Hello ): return 2*a + +# Breaking over parameters and return types. (Black adds a trailing comma when the +# function arguments break here with a single argument; we do not.) +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a: + ... + +def f(a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> a: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a: + ... ``` ## Output @@ -1023,9 +1049,61 @@ def double(a: int) -> int: # Hello return 2 * a -def double(a: int) -> ( # Hello +def double( + a: int +) -> ( # Hello ): return 2 * a + + +# Breaking over parameters and return types. (Black adds a trailing comma when the +# function arguments break here with a single argument; we do not.) +def f( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + + +def f( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a +) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + + +def f( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +) -> a: + ... + + +def f( + a +) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +): + ... + + +def f[ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +]() -> a: + ... + + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + ... + + +def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +) -> a: + ... ```