Skip to content

Commit b40e562

Browse files
DunqingDunqing
committed
refactor(formatter): add speculate_will_break and fix is_complex_type_arguments (#19661)
Closes: #19436 ## Summary - Add `Formatter::speculate_will_break` to encapsulate the snapshot/skip/intern/restore pattern for safely checking if formatted content would break across lines - Replace the approximation heuristics in `is_complex_type_arguments` with Prettier's actual `willBreak(print(typeArgs))` logic - Add `Comments::snapshot`, `restore`, and `skip_comments_before` methods to support speculative formatting without losing comments ## Details The old `is_complex_type_arguments` couldn't call `f.intern()` because it would permanently advance the comment cursor, causing comments to be lost. It used heuristic checks (complex type references, mapped types, etc.) as an approximation instead. With the new comment snapshot/restore infrastructure, we can now match Prettier's behavior exactly: ```rust // Before: ~30 lines of heuristic checks // After: f.speculate_will_break(type_arguments) ``` Prettier ref: https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L432-L459 ## Test plan - [x] Existing formatter tests pass (193/193) - [x] Clippy clean - [x] Added snapshot tests for comment preservation with call expressions and type arguments 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 parent ae79a57 commit b40e562

File tree

7 files changed

+153
-33
lines changed

7 files changed

+153
-33
lines changed

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,19 @@ use oxc_span::{GetSpan, Span};
102102

103103
use crate::formatter::SourceText;
104104

105+
/// Snapshot of the comment processing state for speculative formatting.
106+
///
107+
/// Created by [`Comments::snapshot`] and restored by [`Comments::restore`].
108+
/// This allows speculative formatting (e.g., checking `will_break`) without
109+
/// permanently advancing the comment cursor.
110+
#[derive(Clone, Copy)]
111+
pub struct CommentSnapshot {
112+
printed_count: usize,
113+
last_handled_type_cast_comment: usize,
114+
type_cast_node_span: Span,
115+
view_limit: Option<usize>,
116+
}
117+
105118
#[derive(Debug, Clone)]
106119
pub struct Comments<'a> {
107120
source_text: SourceText<'a>,
@@ -469,4 +482,39 @@ impl<'a> Comments<'a> {
469482
pub fn restore_view_limit(&mut self, limit: Option<usize>) {
470483
self.view_limit = limit;
471484
}
485+
486+
/// Saves the current comment processing state for later restoration.
487+
///
488+
/// Use with [`Comments::restore`] to safely perform speculative formatting
489+
/// without permanently advancing the comment cursor. This prevents comment
490+
/// deletion bugs when using [`Formatter::intern`] for `will_break` checks.
491+
pub fn snapshot(&self) -> CommentSnapshot {
492+
CommentSnapshot {
493+
printed_count: self.printed_count,
494+
last_handled_type_cast_comment: self.last_handled_type_cast_comment,
495+
type_cast_node_span: self.type_cast_node_span,
496+
view_limit: self.view_limit,
497+
}
498+
}
499+
500+
/// Restores comment processing state from a previous snapshot.
501+
///
502+
/// This rolls back the comment cursor so that any comments consumed
503+
/// during speculative formatting are available again for real formatting.
504+
pub fn restore(&mut self, snapshot: CommentSnapshot) {
505+
self.printed_count = snapshot.printed_count;
506+
self.last_handled_type_cast_comment = snapshot.last_handled_type_cast_comment;
507+
self.type_cast_node_span = snapshot.type_cast_node_span;
508+
self.view_limit = snapshot.view_limit;
509+
}
510+
511+
/// Advances the cursor past all comments ending before `pos`.
512+
///
513+
/// Used before speculative formatting to skip comments that are outside
514+
/// the span of interest, preventing them from being incorrectly included
515+
/// as leading comments of the speculatively formatted node.
516+
pub fn skip_comments_before(&mut self, pos: u32) {
517+
let count = self.comments_before(pos).len();
518+
self.printed_count += count;
519+
}
472520
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#![allow(clippy::module_inception)]
22

33
use oxc_allocator::{Allocator, Vec as ArenaVec};
4+
use oxc_span::GetSpan;
45

56
use crate::options::FormatOptions;
67

@@ -227,6 +228,19 @@ impl<'buf, 'ast> Formatter<'buf, 'ast> {
227228
FillBuilder::new(self)
228229
}
229230

231+
/// Speculatively formats `content` and returns whether the result would break across lines.
232+
///
233+
/// This snapshots and restores the comment state so that the speculative formatting
234+
/// doesn't permanently advance the comment cursor. Comments before the content's span
235+
/// are skipped so they don't get incorrectly included as leading comments.
236+
pub fn speculate_will_break(&mut self, content: &(impl Format<'ast> + GetSpan)) -> bool {
237+
let snapshot = self.context().comments().snapshot();
238+
self.context_mut().comments_mut().skip_comments_before(content.span().start);
239+
let will_break = self.intern(content).is_some_and(|e| e.will_break());
240+
self.context_mut().comments_mut().restore(snapshot);
241+
will_break
242+
}
243+
230244
/// Formats `content` into an interned element without writing it to the formatter's buffer.
231245
pub fn intern(&mut self, content: &dyn Format<'ast>) -> Option<FormatElement<'ast>> {
232246
let mut buffer = VecBuffer::new(self.state_mut());

‎crates/oxc_formatter/src/utils/assignment_like.rs‎

Lines changed: 13 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ impl<'a> AssignmentLike<'a, '_> {
446446
&self,
447447
is_left_short: bool,
448448
left_may_break: bool,
449-
f: &Formatter<'_, 'a>,
449+
f: &mut Formatter<'_, 'a>,
450450
) -> AssignmentLikeLayout {
451451
let right_expression = self.get_right_expression();
452452
if let Some(expr) = right_expression {
@@ -610,7 +610,7 @@ impl<'a> AssignmentLike<'a, '_> {
610610
&self,
611611
right_expression: Option<&AstNode<'a, Expression<'a>>>,
612612
is_left_short: bool,
613-
f: &Formatter<'_, 'a>,
613+
f: &mut Formatter<'_, 'a>,
614614
) -> bool {
615615
let comments = f.context().comments();
616616
if let Some(right_expression) = right_expression {
@@ -707,7 +707,7 @@ impl<'a> AssignmentLike<'a, '_> {
707707
fn should_break_after_operator<'a>(
708708
right: &AstNode<'a, Expression<'a>>,
709709
is_left_short: bool,
710-
f: &Formatter<'_, 'a>,
710+
f: &mut Formatter<'_, 'a>,
711711
) -> bool {
712712
if right.is_jsx() {
713713
return false;
@@ -919,7 +919,7 @@ impl<'a> Format<'a> for WithAssignmentLayout<'a, '_> {
919919
/// [Prettier applies]: <https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L329>
920920
fn is_poorly_breakable_member_or_call_chain<'a>(
921921
expression: &AstNode<'a, Expression<'a>>,
922-
f: &Formatter<'_, 'a>,
922+
f: &mut Formatter<'_, 'a>,
923923
) -> bool {
924924
let threshold = f.options().line_width.value() / 4;
925925

@@ -1060,47 +1060,27 @@ fn is_short_argument(argument: &Expression, threshold: u16, f: &Formatter) -> bo
10601060
/// We need it to decide if `CallExpression` with the type arguments is breakable or not.
10611061
/// If the type arguments is complex the function call is breakable.
10621062
///
1063-
/// NOTE: This function does not follow Prettier exactly.
10641063
/// <https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L432-L459>
10651064
fn is_complex_type_arguments<'a>(
1066-
type_arguments: &TSTypeParameterInstantiation<'a>,
1067-
f: &Formatter<'_, 'a>,
1065+
type_arguments: &AstNode<'a, TSTypeParameterInstantiation<'a>>,
1066+
f: &mut Formatter<'_, 'a>,
10681067
) -> bool {
1069-
let is_complex_ts_type = |ts_type: &TSType| match ts_type {
1070-
TSType::TSUnionType(_) | TSType::TSIntersectionType(_) | TSType::TSTypeLiteral(_) => true,
1071-
// Check for newlines after `{` in mapped types, as it will expand to multiple lines if so
1072-
TSType::TSMappedType(mapped) => f.source_text().has_newline_after(mapped.span.start + 1),
1073-
_ => false,
1074-
};
1075-
1076-
let is_complex_type_reference = |reference: &TSTypeReference| {
1077-
reference.type_arguments.as_ref().is_some_and(|type_arguments| {
1078-
type_arguments.params.iter().any(|param| match param {
1079-
TSType::TSTypeLiteral(literal) => literal.members.len() > 1,
1080-
_ => false,
1081-
})
1082-
})
1083-
};
1084-
10851068
let params = &type_arguments.params;
10861069
if params.len() > 1 {
10871070
return true;
10881071
}
10891072

1090-
// NOTE: Prettier checks `willBreak(print(typeArgs))` here.
1091-
// Our equivalent is `type_arguments.memoized().inspect(f).will_break()`,
1092-
// but we avoid using it because:
1093-
// - `inspect(f)` (= `f.intern()`) will update the comment counting state in `f`
1094-
// - And resulted IRs are discarded after this check
1095-
// So we approximate it by checking if the type arguments contain complex types.
1096-
if params.first().is_some_and(|param| match param {
1097-
TSType::TSTypeReference(reference) => is_complex_type_reference(reference),
1098-
ts_type => is_complex_ts_type(ts_type),
1073+
if params.first().is_some_and(|param| {
1074+
matches!(
1075+
param,
1076+
TSType::TSUnionType(_) | TSType::TSIntersectionType(_) | TSType::TSTypeLiteral(_)
1077+
)
10991078
}) {
11001079
return true;
11011080
}
11021081

1103-
f.comments().has_comment_in_span(type_arguments.span)
1082+
// Prettier: `willBreak(print(typeArgsKeyName))`
1083+
f.speculate_will_break(type_arguments)
11041084
}
11051085

11061086
/// [Prettier applies]: <https://github.com/prettier/prettier/blob/fde0b49d7866e203ca748c306808a87b7c15548f/src/language-js/print/assignment.js#L278>

‎crates/oxc_formatter/tests/fixtures/js/comments/assignment-call-expression.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// Minimum repro from issue #19436:
2+
// Comments before call expressions must be preserved
3+
const r = /* THIS */ f()
4+
5+
// @__PURE__ comments must not be deleted
6+
export const globalRegistry = /*@__PURE__*/ registry();

‎crates/oxc_formatter/tests/fixtures/js/comments/assignment-call-expression.js.snap‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
source: crates/oxc_formatter/tests/fixtures/mod.rs
3+
---
4+
==================== Input ====================
5+
// Minimum repro from issue #19436:
6+
// Comments before call expressions must be preserved
7+
const r = /* THIS */ f()
8+
9+
// @__PURE__ comments must not be deleted
10+
export const globalRegistry = /*@__PURE__*/ registry();
11+
12+
==================== Output ====================
13+
------------------
14+
{ printWidth: 80 }
15+
------------------
16+
// Minimum repro from issue #19436:
17+
// Comments before call expressions must be preserved
18+
const r = /* THIS */ f();
19+
20+
// @__PURE__ comments must not be deleted
21+
export const globalRegistry = /*@__PURE__*/ registry();
22+
23+
-------------------
24+
{ printWidth: 100 }
25+
-------------------
26+
// Minimum repro from issue #19436:
27+
// Comments before call expressions must be preserved
28+
const r = /* THIS */ f();
29+
30+
// @__PURE__ comments must not be deleted
31+
export const globalRegistry = /*@__PURE__*/ registry();
32+
33+
===================== End =====================

‎crates/oxc_formatter/tests/fixtures/ts/comments/assignment-call-with-type-args.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// Type arguments with speculative formatting should not lose comments
2+
export const globalRegistry: $ZodRegistry = /*@__PURE__*/ registry();
3+
4+
// Comments before call expressions with type arguments should be preserved
5+
const r = /* THIS */ f<Type>()
6+
const s = /* comment */ foo<A | B | C>()

‎crates/oxc_formatter/tests/fixtures/ts/comments/assignment-call-with-type-args.ts.snap‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
source: crates/oxc_formatter/tests/fixtures/mod.rs
3+
---
4+
==================== Input ====================
5+
// Type arguments with speculative formatting should not lose comments
6+
export const globalRegistry: $ZodRegistry = /*@__PURE__*/ registry();
7+
8+
// Comments before call expressions with type arguments should be preserved
9+
const r = /* THIS */ f<Type>()
10+
const s = /* comment */ foo<A | B | C>()
11+
12+
==================== Output ====================
13+
------------------
14+
{ printWidth: 80 }
15+
------------------
16+
// Type arguments with speculative formatting should not lose comments
17+
export const globalRegistry: $ZodRegistry = /*@__PURE__*/ registry();
18+
19+
// Comments before call expressions with type arguments should be preserved
20+
const r = /* THIS */ f<Type>();
21+
const s = /* comment */ foo<A | B | C>();
22+
23+
-------------------
24+
{ printWidth: 100 }
25+
-------------------
26+
// Type arguments with speculative formatting should not lose comments
27+
export const globalRegistry: $ZodRegistry = /*@__PURE__*/ registry();
28+
29+
// Comments before call expressions with type arguments should be preserved
30+
const r = /* THIS */ f<Type>();
31+
const s = /* comment */ foo<A | B | C>();
32+
33+
===================== End =====================

0 commit comments

Comments
 (0)