Skip to content

Commit 01f20f3

Browse files
DunqingDunqing
committed
fix(formatter): incorrect comment checking logic for grouping argument (#15354)
1 parent feb2b08 commit 01f20f3

File tree

6 files changed

+237
-39
lines changed

6 files changed

+237
-39
lines changed

‎crates/oxc_formatter/src/formatter/comments.rs‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -259,15 +259,6 @@ impl<'a> Comments<'a> {
259259
.any(|comment| self.source_text.lines_after(comment.span.end) > 0)
260260
}
261261

262-
/// Checks if there are leading or trailing comments around `current_span`.
263-
/// Leading comments are between `previous_end` and `current_span.start`.
264-
/// Trailing comments are between `current_span.end` and `following_start`.
265-
#[inline]
266-
pub fn has_comment(&self, previous_end: u32, current_span: Span, following_start: u32) -> bool {
267-
self.has_comment_in_range(previous_end, current_span.start)
268-
|| self.has_comment_in_range(current_span.end, following_start)
269-
}
270-
271262
/// **Critical method**: Advances the printed cursor by one.
272263
///
273264
/// This MUST be called after formatting each comment to maintain system integrity.

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

Lines changed: 50 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ impl<'a> Format<'a> for AstNode<'a, ArenaVec<'a, Argument<'a>>> {
4343
fn fmt(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> {
4444
let l_paren_token = "(";
4545
let r_paren_token = ")";
46-
let call_like_span = self.parent.span();
4746

4847
let arguments = self.as_ref();
4948

@@ -53,7 +52,7 @@ impl<'a> Format<'a> for AstNode<'a, ArenaVec<'a, Argument<'a>>> {
5352
[
5453
l_paren_token,
5554
// `call/* comment1 */(/* comment2 */)` Both comments are dangling comments.
56-
format_dangling_comments(call_like_span).with_soft_block_indent(),
55+
format_dangling_comments(self.parent.span()).with_soft_block_indent(),
5756
r_paren_token
5857
]
5958
);
@@ -115,7 +114,7 @@ impl<'a> Format<'a> for AstNode<'a, ArenaVec<'a, Argument<'a>>> {
115114
return format_all_args_broken_out(self, true, f);
116115
}
117116

118-
if let Some(group_layout) = arguments_grouped_layout(call_like_span, self, f) {
117+
if let Some(group_layout) = arguments_grouped_layout(self, f) {
119118
write_grouped_arguments(self, group_layout, f)
120119
} else if call_expression.is_some_and(|call| is_long_curried_call(call)) {
121120
let trailing_operator = FormatTrailingCommas::All.trailing_separator(f.options());
@@ -254,7 +253,6 @@ fn format_all_args_broken_out<'a, 'b>(
254253
}
255254

256255
pub fn arguments_grouped_layout(
257-
call_like_span: Span,
258256
args: &[Argument],
259257
f: &Formatter<'_, '_>,
260258
) -> Option<GroupedCallArgumentLayout> {
@@ -270,30 +268,23 @@ pub fn arguments_grouped_layout(
270268
let second_can_group_fn = || second_can_group;
271269

272270
// Check if we should group the last argument (second)
273-
if should_group_last_argument_impl(
274-
call_like_span,
275-
Some(first),
276-
second,
277-
second_can_group_fn,
278-
f,
279-
) {
271+
if should_group_last_argument_impl(Some(first), second, second_can_group_fn, f) {
280272
return Some(GroupedCallArgumentLayout::GroupedLastArgument);
281273
}
282274

283275
// Check if we should group the first argument instead
284276
// Reuse the already-computed `second_can_group` value
285-
should_group_first_argument(call_like_span, first, second, second_can_group, f)
277+
should_group_first_argument(first, second, second_can_group, f)
286278
.then_some(GroupedCallArgumentLayout::GroupedFirstArgument)
287279
} else {
288280
// For other cases (not exactly 2 arguments), only check last argument grouping
289-
should_group_last_argument(call_like_span, args, f)
281+
should_group_last_argument(args, f)
290282
.then_some(GroupedCallArgumentLayout::GroupedLastArgument)
291283
}
292284
}
293285

294286
/// Checks if the first argument requires grouping
295287
fn should_group_first_argument(
296-
call_like_span: Span,
297288
first: &Expression,
298289
second: &Expression,
299290
second_can_group: bool,
@@ -322,16 +313,29 @@ fn should_group_first_argument(
322313
return false;
323314
}
324315

325-
!f.comments().has_comment(call_like_span.start, first.span(), second.span().start)
326-
&& !second_can_group
327-
&& is_relatively_short_argument(second)
316+
let first_span = first.span();
317+
318+
// Don't group if there are comments around the first argument:
319+
// - Before the first argument
320+
// - After the first argument (next non-whitespace is not a comma)
321+
// - End-of-line comments between the first and second argument
322+
if f.comments().has_comment_before(first_span.start)
323+
|| !f.source_text().next_non_whitespace_byte_is(first_span.end, b',')
324+
|| f.comments()
325+
.comments_in_range(first_span.end, second.span().start)
326+
.iter()
327+
.any(|c| f.comments().is_end_of_line_comment(c))
328+
{
329+
return false;
330+
}
331+
332+
!second_can_group && is_relatively_short_argument(second)
328333
}
329334

330335
/// Core logic for checking if the last argument should be grouped.
331336
/// Takes the penultimate argument as an Expression for the 2-argument case,
332337
/// or extracts it from the arguments array for other cases.
333338
fn should_group_last_argument_impl(
334-
call_like_span: Span,
335339
penultimate: Option<&Expression>,
336340
last: &Expression,
337341
last_can_group_fn: impl FnOnce() -> bool,
@@ -343,13 +347,37 @@ fn should_group_last_argument_impl(
343347
(penultimate, last),
344348
(Expression::ObjectExpression(_), Expression::ObjectExpression(_))
345349
| (Expression::ArrayExpression(_), Expression::ArrayExpression(_))
350+
| (Expression::TSAsExpression(_), Expression::TSAsExpression(_))
351+
| (Expression::TSSatisfiesExpression(_), Expression::TSSatisfiesExpression(_))
352+
| (Expression::ArrowFunctionExpression(_), Expression::ArrowFunctionExpression(_))
353+
| (Expression::FunctionExpression(_), Expression::FunctionExpression(_))
346354
)
347355
{
348356
return false;
349357
}
350358

351-
let previous_span = penultimate.map_or(call_like_span.start, |e| e.span().end);
352-
if f.comments().has_comment(previous_span, last.span(), call_like_span.end) {
359+
let last_span = last.span();
360+
361+
// Don't group if there are comments around the last argument
362+
let has_comment_before_last = if let Some(penultimate) = penultimate {
363+
// Check for comments between the penultimate and last argument
364+
f.comments().comments_in_range(penultimate.span().end, last_span.start).last().is_some_and(
365+
|c| {
366+
// Exclude end-of-line comments (treated as previous node's comment)
367+
// and comments followed by a comma
368+
!f.comments().is_end_of_line_comment(c)
369+
&& !f.source_text().next_non_whitespace_byte_is(c.span.end, b',')
370+
},
371+
)
372+
} else {
373+
f.comments().has_comment_before(last_span.start)
374+
};
375+
376+
if has_comment_before_last
377+
// Check for comments after the last argument
378+
|| f.comments().comments_after(last_span.end).first().is_some_and(|c|
379+
!f.source_text().bytes_contain(last_span.end, c.span.start, b')'))
380+
{
353381
return false;
354382
}
355383

@@ -371,19 +399,15 @@ fn should_group_last_argument_impl(
371399
}
372400

373401
/// Checks if the last argument should be grouped.
374-
fn should_group_last_argument(
375-
call_like_span: Span,
376-
args: &[Argument],
377-
f: &Formatter<'_, '_>,
378-
) -> bool {
402+
fn should_group_last_argument(args: &[Argument], f: &Formatter<'_, '_>) -> bool {
379403
let mut iter = args.iter();
380404
let Some(last) = iter.next_back().unwrap().as_expression() else {
381405
return false;
382406
};
383407

384408
let penultimate = iter.next_back().and_then(|arg| arg.as_expression());
385409
let last_can_group_fn = || can_group_expression_argument(last, f);
386-
should_group_last_argument_impl(call_like_span, penultimate, last, last_can_group_fn, f)
410+
should_group_last_argument_impl(penultimate, last, last_can_group_fn, f)
387411
}
388412

389413
/// Check if `ty` is a relatively simple type annotation, allowing a few
@@ -500,23 +524,19 @@ fn can_group_expression_argument(argument: &Expression<'_>, f: &Formatter<'_, '_
500524
!object_expression.properties.is_empty()
501525
|| f.comments().has_comment_in_span(object_expression.span)
502526
}
503-
504527
Expression::ArrayExpression(array_expression) => {
505528
!array_expression.elements.is_empty()
506529
|| f.comments().has_comment_in_span(array_expression.span)
507530
}
508531
Expression::TSTypeAssertion(assertion_expression) => {
509532
can_group_expression_argument(&assertion_expression.expression, f)
510533
}
511-
512534
Expression::TSAsExpression(as_expression) => {
513535
can_group_expression_argument(&as_expression.expression, f)
514536
}
515-
516537
Expression::TSSatisfiesExpression(satisfies_expression) => {
517538
can_group_expression_argument(&satisfies_expression.expression, f)
518539
}
519-
520540
Expression::ArrowFunctionExpression(arrow_function) => {
521541
can_group_arrow_function_expression_argument(arrow_function, false, f)
522542
}

‎crates/oxc_formatter/tests/fixtures/js/comments/calls/last-argument-group.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
call(editor /* comment */, () => {
2+
//
3+
});
4+
call(editor, /* comment */
5+
() => {
6+
//
7+
}
8+
);
9+
call(/* */ editor /* comment */, () => {
10+
//
11+
});
12+
call(/* comment */
13+
() => {
14+
//
15+
}
16+
);
17+
call(
18+
function () {
19+
var a = 1;
20+
// one
21+
},
22+
// two
23+
);

‎crates/oxc_formatter/tests/fixtures/js/comments/calls/last-argument-group.js.snap‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
source: crates/oxc_formatter/tests/fixtures/mod.rs
3+
---
4+
==================== Input ====================
5+
call(editor /* comment */, () => {
6+
//
7+
});
8+
call(editor, /* comment */
9+
() => {
10+
//
11+
}
12+
);
13+
call(/* */ editor /* comment */, () => {
14+
//
15+
});
16+
call(/* comment */
17+
() => {
18+
//
19+
}
20+
);
21+
call(
22+
function () {
23+
var a = 1;
24+
// one
25+
},
26+
// two
27+
);
28+
==================== Output ====================
29+
call(editor /* comment */, () => {
30+
//
31+
});
32+
call(editor /* comment */, () => {
33+
//
34+
});
35+
call(/* */ editor /* comment */, () => {
36+
//
37+
});
38+
call(
39+
/* comment */
40+
() => {
41+
//
42+
},
43+
);
44+
call(
45+
function () {
46+
var a = 1;
47+
// one
48+
},
49+
// two
50+
);
51+
52+
===================== End =====================

‎crates/oxc_formatter/tests/fixtures/ts/arguments/no-group-same-types.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Don't group when both arguments are objects
2+
call({ a: 1 }, { b: 2 });
3+
4+
// Don't group when both arguments are arrays
5+
call([1, 2, 3], [4, 5, 6]);
6+
7+
// Don't group when both arguments are TSAsExpression
8+
call(x as string, y as number);
9+
10+
// Don't group when both arguments are TSSatisfiesExpression
11+
call(x satisfies Foo, y satisfies Bar);
12+
13+
// Don't group when both arguments are arrow functions
14+
call(() => foo, () => bar);
15+
16+
// Don't group when both arguments are function expressions
17+
call(function() { return foo; }, function() { return bar; });
18+
19+
// DO group when arguments are different types - object and array
20+
call({ a: 1, b: 2, c: 3 }, [1, 2, 3, 4, 5, 6]);
21+
22+
// DO group when arguments are different types - array and object
23+
call([1, 2, 3, 4, 5, 6], { a: 1, b: 2, c: 3 });
24+
25+
// DO group when first is arrow and second is object
26+
call(() => { return foo; }, { a: 1, b: 2, c: 3 });
27+
28+
// DO group when first is object and second is arrow
29+
call({ a: 1, b: 2, c: 3 }, () => { return foo; });

‎crates/oxc_formatter/tests/fixtures/ts/arguments/no-group-same-types.ts.snap‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
---
2+
source: crates/oxc_formatter/tests/fixtures/mod.rs
3+
---
4+
==================== Input ====================
5+
// Don't group when both arguments are objects
6+
call({ a: 1 }, { b: 2 });
7+
8+
// Don't group when both arguments are arrays
9+
call([1, 2, 3], [4, 5, 6]);
10+
11+
// Don't group when both arguments are TSAsExpression
12+
call(x as string, y as number);
13+
14+
// Don't group when both arguments are TSSatisfiesExpression
15+
call(x satisfies Foo, y satisfies Bar);
16+
17+
// Don't group when both arguments are arrow functions
18+
call(() => foo, () => bar);
19+
20+
// Don't group when both arguments are function expressions
21+
call(function() { return foo; }, function() { return bar; });
22+
23+
// DO group when arguments are different types - object and array
24+
call({ a: 1, b: 2, c: 3 }, [1, 2, 3, 4, 5, 6]);
25+
26+
// DO group when arguments are different types - array and object
27+
call([1, 2, 3, 4, 5, 6], { a: 1, b: 2, c: 3 });
28+
29+
// DO group when first is arrow and second is object
30+
call(() => { return foo; }, { a: 1, b: 2, c: 3 });
31+
32+
// DO group when first is object and second is arrow
33+
call({ a: 1, b: 2, c: 3 }, () => { return foo; });
34+
35+
==================== Output ====================
36+
// Don't group when both arguments are objects
37+
call({ a: 1 }, { b: 2 });
38+
39+
// Don't group when both arguments are arrays
40+
call([1, 2, 3], [4, 5, 6]);
41+
42+
// Don't group when both arguments are TSAsExpression
43+
call(x as string, y as number);
44+
45+
// Don't group when both arguments are TSSatisfiesExpression
46+
call(x satisfies Foo, y satisfies Bar);
47+
48+
// Don't group when both arguments are arrow functions
49+
call(
50+
() => foo,
51+
() => bar,
52+
);
53+
54+
// Don't group when both arguments are function expressions
55+
call(
56+
function () {
57+
return foo;
58+
},
59+
function () {
60+
return bar;
61+
},
62+
);
63+
64+
// DO group when arguments are different types - object and array
65+
call({ a: 1, b: 2, c: 3 }, [1, 2, 3, 4, 5, 6]);
66+
67+
// DO group when arguments are different types - array and object
68+
call([1, 2, 3, 4, 5, 6], { a: 1, b: 2, c: 3 });
69+
70+
// DO group when first is arrow and second is object
71+
call(
72+
() => {
73+
return foo;
74+
},
75+
{ a: 1, b: 2, c: 3 },
76+
);
77+
78+
// DO group when first is object and second is arrow
79+
call({ a: 1, b: 2, c: 3 }, () => {
80+
return foo;
81+
});
82+
83+
===================== End =====================

0 commit comments

Comments
 (0)