From cb29117db75ffc76ba12c7075649de9c9bb64e82 Mon Sep 17 00:00:00 2001 From: Dunqing <29533304+Dunqing@users.noreply.github.com> Date: Mon, 29 Sep 2025 13:17:19 +0000 Subject: [PATCH] feat(formatter): correct printing parameters with `return_type` for function-like node (#14084) --- crates/oxc_formatter/src/generated/format.rs | 10 ++ crates/oxc_formatter/src/write/class.rs | 74 +++++++-- crates/oxc_formatter/src/write/function.rs | 6 +- crates/oxc_formatter/src/write/mod.rs | 142 ++++++------------ crates/oxc_formatter/src/write/object_like.rs | 8 +- .../src/write/object_pattern_like.rs | 8 +- crates/oxc_formatter/src/write/parameters.rs | 16 +- .../src/write/type_parameters.rs | 26 ++-- .../src/generators/formatter/format.rs | 1 + .../snapshots/formatter_typescript.snap | 4 +- .../snapshots/prettier.ts.snap.md | 6 +- 11 files changed, 154 insertions(+), 147 deletions(-) diff --git a/crates/oxc_formatter/src/generated/format.rs b/crates/oxc_formatter/src/generated/format.rs index be896e49014a2..085783229385e 100644 --- a/crates/oxc_formatter/src/generated/format.rs +++ b/crates/oxc_formatter/src/generated/format.rs @@ -4610,9 +4610,19 @@ impl<'a> Format<'a> for AstNode<'a, TSImportTypeQualifiedName<'a>> { impl<'a> Format<'a> for AstNode<'a, TSFunctionType<'a>> { fn fmt(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { let is_suppressed = f.comments().is_suppressed(self.span().start); + if !is_suppressed && format_type_cast_comment_node(self, false, f)? { + return Ok(()); + } self.format_leading_comments(f)?; + let needs_parentheses = self.needs_parentheses(f); + if needs_parentheses { + "(".fmt(f)?; + } let result = if is_suppressed { FormatSuppressedNode(self.span()).fmt(f) } else { self.write(f) }; + if needs_parentheses { + ")".fmt(f)?; + } self.format_trailing_comments(f)?; result } diff --git a/crates/oxc_formatter/src/write/class.rs b/crates/oxc_formatter/src/write/class.rs index f997ec19d5516..f443b15ef7b69 100644 --- a/crates/oxc_formatter/src/write/class.rs +++ b/crates/oxc_formatter/src/write/class.rs @@ -20,7 +20,9 @@ use crate::{ object::format_property_key, }, write, - write::{semicolon::OptionalSemicolon, type_parameters}, + write::{ + function::should_group_function_parameters, semicolon::OptionalSemicolon, type_parameters, + }, }; use super::{ @@ -83,6 +85,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, MethodDefinition<'a>> { } } let value = self.value(); + if value.r#async { write!(f, ["async", space()])?; } @@ -95,22 +98,27 @@ impl<'a> FormatWrite<'a> for AstNode<'a, MethodDefinition<'a>> { format_property_key(self.key(), f)?; } - if self.optional() { + if self.optional { write!(f, "?")?; } - if let Some(type_parameters) = &value.type_parameters() { - write!(f, type_parameters)?; - } - // Handle comments between method name and parameters - // Example: method /* comment */ (param) {} - let comments = f.context().comments().comments_before(value.params().span.start); - if !comments.is_empty() { - write!(f, [space(), FormatTrailingComments::Comments(comments)])?; - } - write!(f, group(&value.params()))?; - if let Some(return_type) = &value.return_type() { - write!(f, return_type)?; + + if value.type_parameters.is_none() { + // // Handle comments between method name and parameters + // // Example: method /* comment */ (param) {} + // let comments = f.context().comments().comments_before(value.params().span.start); + // if !comments.is_empty() { + // write!(f, [space(), FormatTrailingComments::Comments(comments)])?; + // } } + + format_grouped_parameters_with_return_type( + value.type_parameters(), + value.this_param.as_deref(), + value.params(), + value.return_type(), + f, + )?; + if let Some(body) = &value.body() { // Handle block comments between method signature and body // Example: method() /* comment */ {} @@ -623,3 +631,41 @@ impl<'a> Format<'a> for ClassPropertySemicolon<'a, '_> { } } } + +pub fn format_grouped_parameters_with_return_type<'a>( + type_parameters: Option<&AstNode<'a, TSTypeParameterDeclaration<'a>>>, + this_param: Option<&TSThisParameter<'a>>, + params: &AstNode<'a, FormalParameters<'a>>, + return_type: Option<&AstNode<'a, TSTypeAnnotation<'a>>>, + f: &mut Formatter<'_, 'a>, +) -> FormatResult<()> { + write!(f, [type_parameters])?; + + group(&format_once(|f| { + let mut format_parameters = params.memoized(); + let mut format_return_type = return_type.memoized(); + + // Inspect early, in case the `return_type` is formatted before `parameters` + // in `should_group_function_parameters`. + format_parameters.inspect(f)?; + + let group_parameters = should_group_function_parameters( + type_parameters.map(AsRef::as_ref), + params.items.len() + + usize::from(params.rest.is_some()) + + usize::from(this_param.is_some()), + return_type.map(AsRef::as_ref), + &mut format_return_type, + f, + )?; + + if group_parameters { + write!(f, [group(&format_parameters)]) + } else { + write!(f, [format_parameters]) + }?; + + write!(f, [format_return_type]) + })) + .fmt(f) +} diff --git a/crates/oxc_formatter/src/write/function.rs b/crates/oxc_formatter/src/write/function.rs index 7e40f827f5c9d..ceafa4d85531c 100644 --- a/crates/oxc_formatter/src/write/function.rs +++ b/crates/oxc_formatter/src/write/function.rs @@ -117,10 +117,10 @@ impl<'a> FormatWrite<'a> for FormatFunction<'a, '_> { )?; if group_parameters { - write!(f, [group(&format_parameters)])?; + write!(f, [group(&format_parameters)]) } else { - write!(f, [format_parameters])?; - } + write!(f, [format_parameters]) + }?; write!(f, [format_return_type]) }))] diff --git a/crates/oxc_formatter/src/write/mod.rs b/crates/oxc_formatter/src/write/mod.rs index ac1c93ff49196..0ff21c6e2fad6 100644 --- a/crates/oxc_formatter/src/write/mod.rs +++ b/crates/oxc_formatter/src/write/mod.rs @@ -78,6 +78,8 @@ use crate::{ use self::{ array_expression::FormatArrayExpression, + class::format_grouped_parameters_with_return_type, + function::should_group_function_parameters, object_like::ObjectLike, object_pattern_like::ObjectPatternLike, parameters::{ParameterLayout, ParameterList}, @@ -1072,10 +1074,16 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSEnumMember<'a>> { impl<'a> FormatWrite<'a> for AstNode<'a, TSTypeAnnotation<'a>> { fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { - if matches!(self.parent, AstNodes::TSTypePredicate(_)) { - write!(f, [self.type_annotation()]) - } else { - write!(f, [":", space(), self.type_annotation()]) + match self.parent { + AstNodes::TSFunctionType(_) | AstNodes::TSConstructorType(_) => { + write!(f, ["=>", space(), self.type_annotation()]) + } + AstNodes::TSTypePredicate(_) => { + write!(f, [self.type_annotation()]) + } + _ => { + write!(f, [":", space(), self.type_annotation()]) + } } } } @@ -1466,18 +1474,7 @@ impl<'a> Format<'a> for AstNode<'a, Vec<'a, TSSignature<'a>>> { impl<'a> FormatWrite<'a> for AstNode<'a, TSCallSignatureDeclaration<'a>> { fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { - if let Some(type_parameters) = &self.type_parameters() { - write!(f, group(type_parameters))?; - } - - if let Some(this_param) = &self.this_param() { - write!(f, [this_param, ",", soft_line_break_or_space()])?; - } - write!(f, group(&self.params()))?; - if let Some(return_type) = &self.return_type() { - write!(f, return_type)?; - } - Ok(()) + write!(f, group(&format_args!(self.type_parameters(), self.params(), self.return_type()))) } } @@ -1502,42 +1499,21 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSMethodSignature<'a>> { if self.optional() { write!(f, "?")?; } - // TODO: - // if should_group_function_parameters( - // type_parameters.as_ref(), - // parameters.len(), - // return_type_annotation - // .as_ref() - // .map(|annotation| annotation.ty()), - // &mut format_return_type_annotation, - // f, - // )? { - // write!(f, [group(¶meters)])?; - // } else { - // write!(f, [parameters])?; - // } - if let Some(type_parameters) = &self.type_parameters() { - write!(f, [group(&type_parameters)])?; - } - write!(f, group(&self.params()))?; - if let Some(return_type) = &self.return_type() { - write!(f, return_type)?; - } - Ok(()) + + format_grouped_parameters_with_return_type( + self.type_parameters(), + self.this_param.as_deref(), + self.params(), + self.return_type(), + f, + ) } } impl<'a> FormatWrite<'a> for AstNode<'a, TSConstructSignatureDeclaration<'a>> { fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { write!(f, ["new", space()])?; - if let Some(type_parameters) = &self.type_parameters() { - write!(f, group(&type_parameters))?; - } - write!(f, group(&self.params()))?; - if let Some(return_type) = self.return_type() { - write!(f, return_type)?; - } - Ok(()) + write!(f, group(&format_args!(self.type_parameters(), self.params(), self.return_type()))) } } @@ -1677,45 +1653,39 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSImportTypeQualifiedName<'a>> { impl<'a> FormatWrite<'a> for AstNode<'a, TSFunctionType<'a>> { fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { - let type_parameters = self.type_parameters(); - let params = self.params(); - let return_type = self.return_type(); - let format_inner = format_with(|f| { + let type_parameters = self.type_parameters(); write!(f, type_parameters)?; - // TODO - // let mut format_return_type = return_type.format().memoized(); - let should_group_parameters = false; - // TODO - //should_group_function_parameters( - // type_parameters.as_ref(), - // parameters.as_ref()?.items().len(), - // Some(return_type.clone()), - // &mut format_return_type, - // f, - // )?; - - if should_group_parameters { - write!(f, group(¶ms))?; - } else { - write!(f, params)?; - } + let params = self.params(); + let return_type = self.return_type(); + let mut format_parameters = params.memoized(); + let mut format_return_type = return_type.memoized(); + + // Inspect early, in case the `return_type` is formatted before `parameters` + // in `should_group_function_parameters`. + format_parameters.inspect(f)?; + + let group_parameters = should_group_function_parameters( + type_parameters.map(AsRef::as_ref), + params.items.len() + + usize::from(params.rest.is_some()) + + usize::from(self.this_param.is_some()), + Some(&self.return_type), + &mut format_return_type, + f, + )?; - let comments = f.context().comments().comments_before_character(params.span.end, b'='); - FormatTrailingComments::Comments(comments).fmt(f)?; + if group_parameters { + write!(f, [group(&format_parameters)]) + } else { + write!(f, [format_parameters]) + }?; - write!(f, [space(), "=>", space(), return_type.type_annotation()]) + write!(f, [space(), format_return_type]) }); - if self.needs_parentheses(f) { - "(".fmt(f)?; - } - write!(f, group(&format_inner))?; - if self.needs_parentheses(f) { - ")".fmt(f)?; - } - Ok(()) + write!(f, group(&format_inner)) } } @@ -1731,21 +1701,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSConstructorType<'a>> { } write!( f, - [group(&format_args!( - "new", - space(), - type_parameters, - params, - format_once(|f| { - let comments = - f.context().comments().comments_before_character(params.span.end, b'='); - FormatTrailingComments::Comments(comments).fmt(f) - }), - space(), - "=>", - space(), - return_type.type_annotation() - ))] + [group(&format_args!("new", space(), type_parameters, params, space(), return_type))] ); Ok(()) } diff --git a/crates/oxc_formatter/src/write/object_like.rs b/crates/oxc_formatter/src/write/object_like.rs index 4efdee383cdc8..42ea1833520ee 100644 --- a/crates/oxc_formatter/src/write/object_like.rs +++ b/crates/oxc_formatter/src/write/object_like.rs @@ -10,7 +10,7 @@ use crate::{ generated::ast_nodes::{AstNode, AstNodes}, options::Expand, write, - write::parameters::should_hug_function_parameters, + write::parameters::{get_this_param, should_hug_function_parameters}, }; #[derive(Clone, Copy)] @@ -39,11 +39,7 @@ impl<'a> ObjectLike<'a, '_> { let AstNodes::FormalParameters(parameters) = ¶m.parent else { unreachable!() }; - let this_param = if let AstNodes::Function(function) = parameters.parent { - function.this_param() - } else { - None - }; + let this_param = get_this_param(parameters.parent); should_hug_function_parameters(parameters, this_param, false, f) } diff --git a/crates/oxc_formatter/src/write/object_pattern_like.rs b/crates/oxc_formatter/src/write/object_pattern_like.rs index 6391c43252eb8..9396af4efe034 100644 --- a/crates/oxc_formatter/src/write/object_pattern_like.rs +++ b/crates/oxc_formatter/src/write/object_pattern_like.rs @@ -9,7 +9,7 @@ use crate::{ }, generated::ast_nodes::{AstNode, AstNodes}, write, - write::parameters::should_hug_function_parameters, + write::parameters::{get_this_param, should_hug_function_parameters}, }; use super::{ @@ -117,11 +117,7 @@ impl<'a> ObjectPatternLike<'a, '_> { matches!(self, Self::ObjectPattern(node) if { matches!(node.parent, AstNodes::FormalParameter(param) if { matches!(param.parent, AstNodes::FormalParameters(params) if { - let this_param = if let AstNodes::Function(function) = params.parent { - function.this_param() - } else { - None - }; + let this_param = get_this_param(param.parent); should_hug_function_parameters(params, this_param, false, f) }) }) diff --git a/crates/oxc_formatter/src/write/parameters.rs b/crates/oxc_formatter/src/write/parameters.rs index 60680560719fd..5a274db55a563 100644 --- a/crates/oxc_formatter/src/write/parameters.rs +++ b/crates/oxc_formatter/src/write/parameters.rs @@ -15,6 +15,16 @@ use crate::{ use super::FormatWrite; +pub fn get_this_param<'a>(parent: &AstNodes<'a>) -> Option<&'a AstNode<'a, TSThisParameter<'a>>> { + match parent { + AstNodes::Function(func) => func.this_param(), + AstNodes::TSFunctionType(func) => func.this_param(), + AstNodes::TSMethodSignature(func) => func.this_param(), + AstNodes::TSCallSignatureDeclaration(func) => func.this_param(), + _ => None, + } +} + impl<'a> FormatWrite<'a> for AstNode<'a, FormalParameters<'a>> { fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> { let comments = f.context().comments().comments_before(self.span.start); @@ -28,11 +38,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, FormalParameters<'a>> { false }; - let this_param = if let AstNodes::Function(function) = self.parent { - function.this_param() - } else { - None - }; + let this_param = get_this_param(self.parent); let has_any_decorated_parameter = self.items.iter().any(|param| !param.decorators.is_empty()); diff --git a/crates/oxc_formatter/src/write/type_parameters.rs b/crates/oxc_formatter/src/write/type_parameters.rs index a4d82aea5f62a..c950b619aa5a1 100644 --- a/crates/oxc_formatter/src/write/type_parameters.rs +++ b/crates/oxc_formatter/src/write/type_parameters.rs @@ -6,8 +6,10 @@ use oxc_ast::{AstKind, ast::*}; use crate::{ format_args, formatter::{ - Buffer, Format, FormatError, FormatResult, Formatter, GroupId, prelude::*, + Buffer, Format, FormatError, FormatResult, Formatter, GroupId, + prelude::*, separated::FormatSeparatedIter, + trivia::{DanglingIndentMode, FormatDanglingComments}, }, generated::ast_nodes::{AstNode, AstNodes}, options::{FormatTrailingCommas, TrailingSeparator}, @@ -116,7 +118,9 @@ impl<'a> Format<'a> for FormatTSTypeParameters<'a, '_> { f.join_nodes_with_space().entries_with_trailing_separator(params, ",", TrailingSeparator::Omit).finish() } else { soft_block_indent(¶ms).fmt(f) - } + }?; + + format_dangling_comments(self.decl.span).with_soft_block_indent().fmt(f) }), ">")) .with_group_id(self.options.group_id)] ) @@ -130,14 +134,13 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSTypeParameterInstantiation<'a>> { if params.is_empty() { // This shouldn't happen in valid TypeScript code, but handle it gracefully - return write!( - f, - [&group(&format_args!( - "<", - format_dangling_comments(self.span).with_soft_block_indent(), - ">" - ))] - ); + let comments = f.context().comments().comments_before(self.span.end); + let indent = if comments.iter().any(|c| c.is_line()) { + DanglingIndentMode::Soft + } else { + DanglingIndentMode::None + }; + return write!(f, ["<", FormatDanglingComments::Comments { comments, indent }, ">"]); } // Check if this is in the context of an arrow function variable @@ -161,8 +164,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, TSTypeParameterInstantiation<'a>> { .finish() }); - let should_inline = - !is_arrow_function_vars && (params.is_empty() || first_arg_can_be_hugged); + let should_inline = !is_arrow_function_vars && first_arg_can_be_hugged; if should_inline { write!(f, ["<", format_params, ">"]) diff --git a/tasks/ast_tools/src/generators/formatter/format.rs b/tasks/ast_tools/src/generators/formatter/format.rs index c0c15e2d1e2c2..83518b488c711 100644 --- a/tasks/ast_tools/src/generators/formatter/format.rs +++ b/tasks/ast_tools/src/generators/formatter/format.rs @@ -45,6 +45,7 @@ const AST_NODE_NEEDS_PARENTHESES: &[&str] = &[ "TSIntersectionType", "TSConstructorType", "TSTypeQuery", + "TSFunctionType", ]; const NEEDS_IMPLEMENTING_FMT_WITH_OPTIONS: phf::Map<&'static str, &'static str> = phf::phf_map! { diff --git a/tasks/coverage/snapshots/formatter_typescript.snap b/tasks/coverage/snapshots/formatter_typescript.snap index 5b3f20bcc4609..cb3e9c57d91b6 100644 --- a/tasks/coverage/snapshots/formatter_typescript.snap +++ b/tasks/coverage/snapshots/formatter_typescript.snap @@ -2,7 +2,7 @@ commit: 261630d6 formatter_typescript Summary: AST Parsed : 8816/8816 (100.00%) -Positive Passed: 8796/8816 (99.77%) +Positive Passed: 8797/8816 (99.78%) Mismatch: tasks/coverage/typescript/tests/cases/compiler/amdLikeInputDeclarationEmit.ts Expect to Parse: tasks/coverage/typescript/tests/cases/compiler/arrayFromAsync.ts @@ -29,8 +29,6 @@ Mismatch: tasks/coverage/typescript/tests/cases/compiler/styledComponentsInstant Mismatch: tasks/coverage/typescript/tests/cases/compiler/tryStatementInternalComments.ts -Mismatch: tasks/coverage/typescript/tests/cases/compiler/unionSignaturesWithThisParameter.ts - Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts Classes may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototype Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/expressions/elementAccess/letIdentifierInElementAccess01.ts diff --git a/tasks/prettier_conformance/snapshots/prettier.ts.snap.md b/tasks/prettier_conformance/snapshots/prettier.ts.snap.md index 43776a7338c30..23f2c2c2eaa4b 100644 --- a/tasks/prettier_conformance/snapshots/prettier.ts.snap.md +++ b/tasks/prettier_conformance/snapshots/prettier.ts.snap.md @@ -1,4 +1,4 @@ -ts compatibility: 529/573 (92.32%) +ts compatibility: 533/573 (93.02%) # Failed @@ -20,7 +20,6 @@ ts compatibility: 529/573 (92.32%) | typescript/class/empty-method-body.ts | 💥 | 80.00% | | typescript/class/quoted-property.ts | 💥 | 66.67% | | typescript/comments/method_types.ts | 💥 | 84.62% | -| typescript/comments/type-parameters.ts | 💥 | 65.52% | | typescript/conditional-types/comments.ts | 💥✨ | 31.51% | | typescript/conditional-types/conditonal-types.ts | 💥✨ | 34.48% | | typescript/conditional-types/infer-type.ts | 💥✨ | 4.76% | @@ -31,12 +30,10 @@ ts compatibility: 529/573 (92.32%) | typescript/decorators-ts/angular.ts | 💥 | 87.50% | | typescript/definite/without-annotation.ts | 💥 | 83.33% | | typescript/enum/computed-members.ts | 💥 | 0.00% | -| typescript/function-type/consistent.ts | 💥 | 70.83% | | typescript/interface/ignore.ts | 💥✨ | 40.09% | | typescript/intersection/intersection-parens.ts | 💥💥 | 80.85% | | typescript/intersection/consistent-with-flow/intersection-parens.ts | 💥 | 69.77% | | typescript/last-argument-expansion/decorated-function.tsx | 💥 | 29.06% | -| typescript/method/issue-10352-consistency.ts | 💥 | 63.64% | | typescript/multiparser-css/issue-6259.ts | 💥 | 57.14% | | typescript/non-null/optional-chain.ts | 💥 | 72.22% | | typescript/object-multiline/multiline.ts | 💥✨ | 23.21% | @@ -44,7 +41,6 @@ ts compatibility: 529/573 (92.32%) | typescript/prettier-ignore/prettier-ignore-nested-unions.ts | 💥 | 44.00% | | typescript/type-arguments-bit-shift-left-like/3.ts | 💥 | 0.00% | | typescript/type-arguments-bit-shift-left-like/5.tsx | 💥 | 0.00% | -| typescript/typeparams/empty-parameters-with-arrow-function/issue-13817.ts | 💥 | 73.68% | | typescript/union/union-parens.ts | 💥 | 92.59% | | typescript/union/consistent-with-flow/prettier-ignore.ts | 💥 | 60.00% | | typescript/union/single-type/single-type.ts | 💥 | 0.00% |