From 1b2a7449e76a92c3f804c0dc0c2e3a606bbc4af2 Mon Sep 17 00:00:00 2001 From: Jon Date: Fri, 1 Dec 2023 16:24:21 -0800 Subject: [PATCH] fix(js_formatter): Allow function expressions to group and break as call arguments (#1003) --- .../src/js/bindings/parameters.rs | 43 +++++++ .../expressions/arrow_function_expression.rs | 26 ++-- .../src/js/expressions/call_arguments.rs | 16 ++- crates/biome_js_formatter/tests/quick_test.rs | 15 ++- .../tests/specs/js/module/call_expression.js | 18 ++- .../specs/js/module/call_expression.js.snap | 32 +++++ .../js/arrows/issue-1389-curry.js.snap | 82 ------------- .../forward-ref.tsx.snap | 111 ------------------ 8 files changed, 127 insertions(+), 216 deletions(-) delete mode 100644 crates/biome_js_formatter/tests/specs/prettier/js/arrows/issue-1389-curry.js.snap delete mode 100644 crates/biome_js_formatter/tests/specs/prettier/typescript/last-argument-expansion/forward-ref.tsx.snap diff --git a/crates/biome_js_formatter/src/js/bindings/parameters.rs b/crates/biome_js_formatter/src/js/bindings/parameters.rs index 536f99ff6b98..b60a0c357252 100644 --- a/crates/biome_js_formatter/src/js/bindings/parameters.rs +++ b/crates/biome_js_formatter/src/js/bindings/parameters.rs @@ -375,3 +375,46 @@ pub(crate) fn should_hug_function_parameters( Ok(result) } + +/// Tests if all of the parameters of `expression` are simple enough to allow +/// a function to group. +pub(crate) fn has_only_simple_parameters( + parameters: &JsParameters, + allow_type_annotations: bool, +) -> bool { + parameters + .items() + .into_iter() + .flatten() + .all(|parameter| is_simple_parameter(¶meter, allow_type_annotations)) +} + +/// Tests if the single parameter is "simple", as in a plain identifier with no +/// explicit type annotation and no initializer: +/// +/// Examples: +/// foo => true +/// foo? => true +/// foo = 'bar' => false +/// foo: string => false +/// {a, b} => false +/// {a, b} = {} => false +/// [a, b] => false +/// +pub(crate) fn is_simple_parameter( + parameter: &AnyJsParameter, + allow_type_annotations: bool, +) -> bool { + match parameter { + AnyJsParameter::AnyJsFormalParameter(AnyJsFormalParameter::JsFormalParameter(param)) => { + matches!( + param.binding(), + Ok(AnyJsBindingPattern::AnyJsBinding( + AnyJsBinding::JsIdentifierBinding(_) + )) + ) && (allow_type_annotations || param.type_annotation().is_none()) + && param.initializer().is_none() + } + _ => false, + } +} diff --git a/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs b/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs index a478c95fab84..099cbe955838 100644 --- a/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs +++ b/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs @@ -1,4 +1,4 @@ -use crate::js::bindings::parameters::FormatJsParametersOptions; +use crate::js::bindings::parameters::{has_only_simple_parameters, FormatJsParametersOptions}; use crate::prelude::*; use biome_formatter::{ format_args, write, CstFormatContext, FormatRuleWithOptions, RemoveSoftLinesBuffer, @@ -317,8 +317,9 @@ fn format_signature( /// Returns a `true` result if the arrow function contains any elements which /// should force the chain to break onto multiple lines. This includes any kind /// of return type annotation if the function also takes parameters (e.g., -/// `(a, b): bool => ...`), or any kind of rest/object/array binding parameter -/// (e.g., `({a, b: foo}) => ...`). +/// `(a, b): bool => ...`), any kind of rest/object/array binding parameter +/// (e.g., `({a, b: foo}) => ...`), and any kind of initializer for a parameter +/// (e.g., `(a = 2) => ...`). /// /// The complexity of these expressions limits their legibility when printed /// inline, so they force the chain to break to preserve clarity. Any other @@ -332,17 +333,20 @@ fn should_break_chain(arrow: &JsArrowFunctionExpression) -> SyntaxResult { let has_parameters = match ¶meters { AnyJsArrowFunctionParameters::AnyJsBinding(_) => true, - AnyJsArrowFunctionParameters::JsParameters(parameters) => !parameters.items().is_empty(), + AnyJsArrowFunctionParameters::JsParameters(parameters) => { + // This matches Prettier, which allows type annotations when + // grouping arrow expressions, but disallows them when grouping + // normal function expressions. + if !has_only_simple_parameters(parameters, true) { + return Ok(true); + } + !parameters.items().is_empty() + } }; - if arrow.return_type_annotation().is_some() && has_parameters { - return Ok(true); - } - - // Break if the function has any rest, object, or array parameter - let result = has_rest_object_or_array_parameter(¶meters); + let has_type_and_parameters = arrow.return_type_annotation().is_some() && has_parameters; - Ok(result) + Ok(has_type_and_parameters || has_rest_object_or_array_parameter(¶meters)) } fn should_add_parens(body: &AnyJsFunctionBody) -> bool { diff --git a/crates/biome_js_formatter/src/js/expressions/call_arguments.rs b/crates/biome_js_formatter/src/js/expressions/call_arguments.rs index 1e96f0ddfd19..9123d77fb175 100644 --- a/crates/biome_js_formatter/src/js/expressions/call_arguments.rs +++ b/crates/biome_js_formatter/src/js/expressions/call_arguments.rs @@ -1,4 +1,5 @@ use crate::context::trailing_comma::FormatTrailingComma; +use crate::js::bindings::parameters::has_only_simple_parameters; use crate::js::declarations::function_declaration::FormatFunctionOptions; use crate::js::expressions::arrow_function_expression::{ is_multiline_template_starting_on_same_line, FormatJsArrowFunctionExpressionOptions, @@ -359,7 +360,7 @@ fn write_grouped_arguments( grouped_arg.cache_function_body(f); } AnyJsCallArgument::AnyJsExpression(AnyJsExpression::JsFunctionExpression(function)) - if !other_args.is_empty() && !has_no_parameters(function) => + if !other_args.is_empty() || function_has_only_simple_parameters(function) => { grouped_arg.cache_function_body(f); } @@ -604,7 +605,7 @@ impl Format for FormatGroupedLastArgument<'_> { // to remove any soft line breaks. match element.node()? { AnyJsCallArgument::AnyJsExpression(JsFunctionExpression(function)) - if !self.is_only && !has_no_parameters(function) => + if !self.is_only || function_has_only_simple_parameters(function) => { with_token_tracking_disabled(f, |f| { write!( @@ -667,13 +668,10 @@ fn with_token_tracking_disabled R, R>( result } -/// Tests if `expression` has an empty parameters list. -fn has_no_parameters(expression: &JsFunctionExpression) -> bool { - match expression.parameters() { - // Use default formatting for expressions without parameters, will return `Err` anyway - Err(_) => true, - Ok(parameters) => parameters.items().is_empty(), - } +fn function_has_only_simple_parameters(expression: &JsFunctionExpression) -> bool { + expression.parameters().map_or(true, |parameters| { + has_only_simple_parameters(¶meters, false) + }) } /// Helper for formatting a grouped call argument (see [should_group_first_argument] and [should_group_last_argument]). diff --git a/crates/biome_js_formatter/tests/quick_test.rs b/crates/biome_js_formatter/tests/quick_test.rs index 6b0cc455df39..e71fdc0ab672 100644 --- a/crates/biome_js_formatter/tests/quick_test.rs +++ b/crates/biome_js_formatter/tests/quick_test.rs @@ -9,12 +9,23 @@ mod language { include!("language.rs"); } -#[ignore] #[test] // use this test check if your snippet prints as you wish, without using a snapshot fn quick_test() { let src = r#" - foo( /*hi*/(a) => (b) => {}) + +const makeSomeFunction = +(services = {logger:null}) => + (a, b, c) => + services.logger(a,b,c) + +const makeSomeFunction2 = +(services = { + logger: null +}) => + (a, b, c) => + services.logger(a, b, c) + "#; let syntax = JsFileSource::tsx(); let tree = parse( diff --git a/crates/biome_js_formatter/tests/specs/js/module/call_expression.js b/crates/biome_js_formatter/tests/specs/js/module/call_expression.js index 73f17674f9ad..e601d6b751f8 100644 --- a/crates/biome_js_formatter/tests/specs/js/module/call_expression.js +++ b/crates/biome_js_formatter/tests/specs/js/module/call_expression.js @@ -45,4 +45,20 @@ test.expect(t => { t.true(a) }, false, // comment - ) \ No newline at end of file + ) + +// should group start of function expression onto the same line +const Button1 = forwardRef(function Button(props, ref) { + return