Skip to content

Commit 889d2e7

Browse files
DunqingDunqing
committed
fix(formatter): handle poor layout for grouped call arguments (#16093)
This PR is a follow-up on #16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments
1 parent 891e0b4 commit 889d2e7

File tree

5 files changed

+127
-162
lines changed

5 files changed

+127
-162
lines changed

‎crates/oxc_formatter/src/write/arrow_function_expression.rs‎

Lines changed: 36 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -200,56 +200,27 @@ impl<'a, 'b> FormatJsArrowFunctionExpression<'a, 'b> {
200200
|| (matches!(self.arrow.parent, AstNodes::JSXExpressionContainer(container)
201201
if !f.context().comments().has_comment_in_range(arrow.span.end, container.span.end)));
202202

203-
let body_is_condition_type =
204-
matches!(arrow_expression, Some(Expression::ConditionalExpression(_)));
205-
206-
if body_is_condition_type {
207-
write!(
208-
f,
209-
[
210-
formatted_signature,
211-
group(&format_args!(
212-
soft_line_indent_or_hard_space(&format_with(|f| {
213-
if should_add_parens {
214-
write!(f, if_group_fits_on_line(&"("));
215-
}
216-
217-
write!(f, format_body);
218-
219-
if should_add_parens {
220-
write!(f, if_group_fits_on_line(&")"));
221-
}
222-
})),
223-
is_last_call_arg
224-
.then_some(format_args!(FormatTrailingCommas::All,)),
225-
should_add_soft_line.then_some(format_args!(soft_line_break()))
226-
))
227-
]
228-
);
229-
} else {
230-
write!(
231-
f,
232-
[
233-
formatted_signature,
234-
group(&format_args!(
235-
soft_line_indent_or_space(&format_with(|f| {
236-
if should_add_parens {
237-
write!(f, if_group_fits_on_line(&"("));
238-
}
239-
240-
write!(f, format_body);
241-
242-
if should_add_parens {
243-
write!(f, if_group_fits_on_line(&")"));
244-
}
245-
})),
246-
is_last_call_arg
247-
.then_some(format_args!(FormatTrailingCommas::All,)),
248-
should_add_soft_line.then_some(format_args!(soft_line_break()))
249-
))
250-
]
251-
);
252-
}
203+
write!(
204+
f,
205+
[
206+
formatted_signature,
207+
group(&format_args!(
208+
soft_line_indent_or_hard_space(&format_with(|f| {
209+
if should_add_parens {
210+
write!(f, if_group_fits_on_line(&"("));
211+
}
212+
213+
write!(f, format_body);
214+
215+
if should_add_parens {
216+
write!(f, if_group_fits_on_line(&")"));
217+
}
218+
})),
219+
is_last_call_arg.then_some(&FormatTrailingCommas::All),
220+
should_add_soft_line.then_some(soft_line_break())
221+
))
222+
]
223+
);
253224
}
254225
}
255226
}
@@ -295,17 +266,18 @@ impl<'a, 'b> ArrowFunctionLayout<'a, 'b> {
295266
let mut middle = Vec::new();
296267
let mut current = arrow;
297268
let mut should_break = false;
269+
let is_non_grouped_or_grouped_last_argument = matches!(
270+
options.call_argument_layout,
271+
None | Some(GroupedCallArgumentLayout::GroupedLastArgument)
272+
);
298273

299274
loop {
300-
if current.expression()
275+
if is_non_grouped_or_grouped_last_argument
276+
&& current.expression()
301277
&& let Some(AstNodes::ExpressionStatement(expr_stmt)) =
302278
current.body().statements().first().map(AstNode::<Statement>::as_ast_nodes)
303279
&& let AstNodes::ArrowFunctionExpression(next) =
304280
&expr_stmt.expression().as_ast_nodes()
305-
&& matches!(
306-
options.call_argument_layout,
307-
None | Some(GroupedCallArgumentLayout::GroupedLastArgument)
308-
)
309281
{
310282
should_break = should_break || Self::should_break_chain(current);
311283

@@ -710,11 +682,11 @@ fn has_rest_object_or_array_parameter(params: &FormalParameters) -> bool {
710682

711683
/// Writes the arrow function type parameters, parameters, and return type annotation.
712684
///
713-
/// Formats the parameters and return type annotation without any soft line breaks if `is_first_or_last_call_argument` is `true`
685+
/// Formats the parameters and return type annotation without any soft line breaks if `is_grouped_call_argument` is `true`
714686
/// so that the parameters and return type are kept on the same line.
715687
fn format_signature<'a, 'b>(
716688
arrow: &'b AstNode<'a, ArrowFunctionExpression<'a>>,
717-
is_first_or_last_call_argument: bool,
689+
is_grouped_call_argument: bool,
718690
is_first_in_chain: bool,
719691
cache_mode: FunctionCacheMode,
720692
) -> impl Format<'a> + 'b {
@@ -735,15 +707,13 @@ fn format_signature<'a, 'b>(
735707
});
736708
let format_head = FormatContentWithCacheMode::new(arrow.params.span, content, cache_mode);
737709

738-
if is_first_or_last_call_argument {
739-
let mut buffer = RemoveSoftLinesBuffer::new(f);
740-
let mut recording = buffer.start_recording();
741-
742-
write!(recording, format_head);
743-
744-
if recording.stop().will_break() {
745-
// TODO: figure out
746-
// return Err(FormatError::PoorLayout);
710+
if is_grouped_call_argument {
711+
// The first arrow's soft lines have already been removed in the CallArguments.
712+
if is_first_in_chain {
713+
write!(f, format_head);
714+
} else {
715+
let mut buffer = RemoveSoftLinesBuffer::new(f);
716+
write!(buffer, format_head);
747717
}
748718
} else {
749719
write!(

‎crates/oxc_formatter/src/write/call_arguments.rs‎

Lines changed: 87 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ use crate::{
77
ast_nodes::{AstNode, AstNodes},
88
format_args,
99
formatter::{
10-
Comments, FormatElement, Formatter, SourceText, VecBuffer, format_element,
10+
Comments, FormatElement, Formatter, SourceText, VecBuffer,
11+
buffer::RemoveSoftLinesBuffer,
12+
format_element,
1113
prelude::{
1214
FormatElements, Tag, empty_line, expand_parent, format_once, format_with, group,
1315
soft_block_indent, soft_line_break_or_space, space,
@@ -634,7 +636,13 @@ fn write_grouped_arguments<'a>(
634636
let is_grouped_argument = (group_layout.is_grouped_first() && index == 0)
635637
|| (group_layout.is_grouped_last() && index == last_index);
636638

637-
let format_argument = format_once(|f| {
639+
// We have to get the lines before the argument has been formatted, because it relies on
640+
// the comments before the argument. After formatting, the comments might marked as printed,
641+
// which would lead to a wrong line count.
642+
let lines_before = f.source_text().get_lines_before(argument.span(), f.comments());
643+
let comma = (last_index != index).then_some(",");
644+
645+
let interned = f.intern(&format_once(|f| {
638646
if is_grouped_argument {
639647
match argument.as_ast_nodes() {
640648
AstNodes::Function(function)
@@ -643,40 +651,40 @@ fn write_grouped_arguments<'a>(
643651
|| function_has_only_simple_parameters(&function.params)) =>
644652
{
645653
has_cached = true;
646-
return FormatFunction::new_with_options(
647-
function,
648-
FormatFunctionOptions {
649-
cache_mode: FunctionCacheMode::Cache,
650-
..FormatFunctionOptions::default()
651-
},
652-
)
653-
.fmt(f);
654+
return write!(
655+
f,
656+
[
657+
FormatFunction::new_with_options(
658+
function,
659+
FormatFunctionOptions {
660+
cache_mode: FunctionCacheMode::Cache,
661+
..FormatFunctionOptions::default()
662+
},
663+
),
664+
comma
665+
]
666+
);
654667
}
655668
AstNodes::ArrowFunctionExpression(arrow) => {
656669
has_cached = true;
657-
return FormatJsArrowFunctionExpression::new_with_options(
658-
arrow,
659-
FormatJsArrowFunctionExpressionOptions {
660-
cache_mode: FunctionCacheMode::Cache,
661-
..FormatJsArrowFunctionExpressionOptions::default()
662-
},
663-
)
664-
.fmt(f);
670+
return write!(
671+
f,
672+
[
673+
FormatJsArrowFunctionExpression::new_with_options(
674+
arrow,
675+
FormatJsArrowFunctionExpressionOptions {
676+
cache_mode: FunctionCacheMode::Cache,
677+
..FormatJsArrowFunctionExpressionOptions::default()
678+
},
679+
),
680+
comma
681+
]
682+
);
665683
}
666684
_ => {}
667685
}
668686
}
669-
argument.fmt(f);
670-
});
671-
672-
// We have to get the lines before the argument has been formatted, because it relies on
673-
// the comments before the argument. After formatting, the comments might marked as printed,
674-
// which would lead to a wrong line count.
675-
let lines_before = f.source_text().get_lines_before(argument.span(), f.comments());
676-
677-
let interned = f.intern(&format_with(|f| {
678-
format_argument.fmt(f);
679-
write!(f, (last_index != index).then_some(","));
687+
write!(f, [argument, comma]);
680688
}));
681689

682690
let break_type =
@@ -715,46 +723,60 @@ fn write_grouped_arguments<'a>(
715723
// as first or last argument.
716724
let mut grouped = elements;
717725
if has_cached {
718-
match group_layout {
726+
let (argument, grouped_element) = match group_layout {
719727
GroupedCallArgumentLayout::GroupedFirstArgument => {
720-
let argument = node.first().unwrap();
721-
let interned = f.intern(&format_with(|f| {
722-
FormatGroupedFirstArgument { argument }.fmt(f);
723-
write!(f, (last_index != 0).then_some(","));
724-
}));
725-
726-
// Turns out, using the grouped layout isn't a good fit because some parameters of the
727-
// grouped function or arrow expression break. In that case, fall back to the all args expanded
728-
// formatting.
729-
// This back tracking is required because testing if the grouped argument breaks would also return `true`
730-
// if any content of the function body breaks. But, as far as this is concerned, it's only interested if
731-
// any content in the signature breaks.
732-
// TODO: should figure out
733-
// if matches!(interned, Err(FormatError::PoorLayout)) {
734-
// return format_all_elements_broken_out(node, grouped.into_iter(), true, f);
735-
// }
736-
737-
grouped.first_mut().unwrap().0 = interned;
728+
(node.first().unwrap(), &mut grouped.first_mut().unwrap().0)
738729
}
739730
GroupedCallArgumentLayout::GroupedLastArgument => {
740-
let argument = node.last().unwrap();
741-
let interned = f.intern(&format_once(|f| {
742-
FormatGroupedLastArgument { argument, is_only: only_one_argument }.fmt(f);
743-
}));
744-
745-
// Turns out, using the grouped layout isn't a good fit because some parameters of the
746-
// grouped function or arrow expression break. In that case, fall back to the all args expanded
747-
// formatting.
748-
// This back tracking is required because testing if the grouped argument breaks would also return `true`
749-
// if any content of the function body breaks. But, as far as this is concerned, it's only interested if
750-
// any content in the signature breaks.
751-
// TODO: should figure out
752-
// if matches!(interned, Err(FormatError::PoorLayout)) {
753-
// return format_all_elements_broken_out(node, grouped.into_iter(), true, f);
754-
// }
755-
756-
grouped.last_mut().unwrap().0 = interned;
731+
(node.last().unwrap(), &mut grouped.last_mut().unwrap().0)
757732
}
733+
};
734+
735+
let function_params = match argument.as_ast_nodes() {
736+
AstNodes::ArrowFunctionExpression(arrow) => Some(&arrow.params),
737+
AstNodes::Function(function) => Some(&function.params),
738+
_ => None,
739+
};
740+
741+
// Turns out, using the grouped layout isn't a good fit because some parameters of the
742+
// grouped function or arrow expression break. In that case, fall back to the all args expanded
743+
// formatting.
744+
// This back tracking is required because testing if the grouped argument breaks would also return `true`
745+
// if any content of the function body breaks. But, as far as this is concerned, it's only interested if
746+
// any content in the signature breaks.
747+
//
748+
// <https://github.com/biomejs/biome/blob/98ca2ae9f3b9b25a14d63b243223583aba6e4907/crates/biome_js_formatter/src/js/expressions/call_arguments.rs#L466-L482>
749+
if let Some(params) = function_params {
750+
let Some(cached_element) = f.context().get_cached_element(params.as_ref()) else {
751+
unreachable!(
752+
"The parameters should have already been formatted and cached in the `FormatFunction` or `FormatJsArrowFunctionExpression`"
753+
);
754+
};
755+
756+
// Remove soft lines from the cached parameters and check if they would break.
757+
// If they break even without soft lines, we need to use the expanded layout.
758+
let interned = f.intern(&format_once(|f| {
759+
RemoveSoftLinesBuffer::new(f).write_element(cached_element);
760+
}));
761+
762+
if let Some(interned) = interned {
763+
if interned.will_break() {
764+
return format_all_elements_broken_out(node, grouped.into_iter(), true, f);
765+
}
766+
767+
// No break; it should print the element without soft lines.
768+
// It would be used in the `FormatFunction` or `FormatJsArrowFunctionExpression`.
769+
f.context_mut().cache_element(params.as_ref(), interned);
770+
}
771+
}
772+
773+
*grouped_element = if group_layout.is_grouped_first() {
774+
f.intern(&format_with(|f| {
775+
FormatGroupedFirstArgument { argument }.fmt(f);
776+
write!(f, (last_index != 0).then_some(","));
777+
}))
778+
} else {
779+
f.intern(&FormatGroupedLastArgument { argument, is_only: only_one_argument })
758780
}
759781
}
760782

‎crates/oxc_formatter/src/write/function.rs‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,7 @@ use super::{
1010
use crate::{
1111
ast_nodes::AstNode,
1212
format_args,
13-
formatter::{
14-
Buffer, Formatter, buffer::RemoveSoftLinesBuffer, prelude::*, trivia::FormatLeadingComments,
15-
},
13+
formatter::{Buffer, Formatter, prelude::*, trivia::FormatLeadingComments},
1614
write,
1715
write::{
1816
arrow_function_expression::FormatMaybeCachedFunctionBody, semicolon::OptionalSemicolon,
@@ -86,24 +84,6 @@ impl<'a, 'b> FormatFunction<'a, 'b> {
8684
)
8785
.memoized();
8886

89-
let format_parameters = format_with(|f: &mut Formatter<'_, 'a>| {
90-
if self.options.call_argument_layout.is_some() {
91-
let mut buffer = RemoveSoftLinesBuffer::new(f);
92-
93-
let mut recording = buffer.start_recording();
94-
write!(recording, format_parameters);
95-
// let recorded = recording.stop();
96-
97-
// TODO: figure out
98-
// if recorded.will_break() {
99-
// return Err(FormatError::PoorLayout);
100-
// }
101-
} else {
102-
format_parameters.fmt(f);
103-
}
104-
})
105-
.memoized();
106-
10787
let format_return_type = self
10888
.return_type()
10989
.map(|return_type| {

‎tasks/prettier_conformance/snapshots/prettier.js.snap.md‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1-
js compatibility: 723/759 (95.26%)
1+
js compatibility: 729/759 (96.05%)
22

33
# Failed
44

55
| Spec path | Failed or Passed | Match ratio |
66
| :-------- | :--------------: | :---------: |
7-
| js/arrows/call.js | 💥💥 | 61.89% |
87
| js/arrows/comment.js | 💥💥 | 88.89% |
98
| js/comments/15661.js | 💥💥 | 55.17% |
109
| js/comments/dangling_for.js | 💥💥 | 22.22% |
@@ -24,13 +23,8 @@ js compatibility: 723/759 (95.26%)
2423
| js/if/expr_and_same_line_comments.js | 💥 | 97.73% |
2524
| js/if/if_comments.js | 💥 | 76.00% |
2625
| js/if/trailing_comment.js | 💥 | 91.43% |
27-
| js/last-argument-expansion/assignment-pattern.js | 💥 | 0.00% |
28-
| js/last-argument-expansion/dangling-comment-in-arrow-function.js | 💥 | 0.00% |
29-
| js/last-argument-expansion/empty-lines.js | 💥 | 14.29% |
30-
| js/last-argument-expansion/issue-10708.js | 💥 | 0.00% |
31-
| js/last-argument-expansion/issue-7518.js | 💥 | 0.00% |
26+
| js/last-argument-expansion/dangling-comment-in-arrow-function.js | 💥 | 22.22% |
3227
| js/object-multiline/multiline.js | 💥✨ | 22.22% |
33-
| js/preserve-line/parameter-list.js | 💥 | 91.08% |
3428
| js/quotes/objects.js | 💥💥 | 80.00% |
3529
| js/sequence-expression/ignored.js | 💥 | 25.00% |
3630
| js/strings/template-literals.js | 💥💥 | 98.01% |

0 commit comments

Comments
 (0)