Skip to content

Commit d936024

Browse files
DunqingDunqing
committed
feat(formatter): complete printing array-like ast nodes (#12424)
1 parent ae8a13b commit d936024

File tree

5 files changed

+107
-124
lines changed

5 files changed

+107
-124
lines changed

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,11 @@ impl<'a> Format<'a> for ArrayElementList<'a, '_> {
7070

7171
filler.finish()
7272
}
73-
ArrayLayout::OnePerLine => write_array_node(self.elements, f),
73+
ArrayLayout::OnePerLine => write_array_node(
74+
self.elements.len(),
75+
self.elements.iter().map(|e| if e.is_elision() { None } else { Some(e) }),
76+
f,
77+
),
7478
}
7579
}
7680
}

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

Lines changed: 38 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ use self::{
5757
semicolon::{ClassPropertySemicolon, OptionalSemicolon},
5858
type_parameters::{FormatTsTypeParameters, FormatTsTypeParametersOptions},
5959
utils::{
60-
array::TrailingSeparatorMode,
60+
array::{TrailingSeparatorMode, write_array_node},
6161
statement_body::FormatStatementBody,
6262
string_utils::{FormatLiteralStringToken, StringLiteralParentKind},
6363
},
@@ -454,65 +454,30 @@ impl<'a> FormatWrite<'a> for AstNode<'a, AssignmentExpression<'a>> {
454454

455455
impl<'a> FormatWrite<'a> for AstNode<'a, ArrayAssignmentTarget<'a>> {
456456
fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> {
457-
// Copy of `utils::array::write_array_node`
458-
let trailing_separator = FormatTrailingCommas::ES5.trailing_separator(f.options());
459-
460-
// Specifically do not use format_separated as arrays need separators
461-
// inserted after holes regardless of the formatting since this makes a
462-
// semantic difference
463-
464-
let source_text = f.source_text();
465-
let mut join = f.join_nodes_with_soft_line();
466-
let last_index = self.elements().len().saturating_sub(1);
467-
468-
let test = self.elements().iter();
469-
for (index, element) in self.elements().iter().enumerate() {
470-
let separator_mode = match element.as_ref() {
471-
None => TrailingSeparatorMode::Force,
472-
_ => TrailingSeparatorMode::Auto,
473-
};
474-
475-
let is_disallow = matches!(separator_mode, TrailingSeparatorMode::Disallow);
476-
let is_force = matches!(separator_mode, TrailingSeparatorMode::Force);
457+
write!(f, "[")?;
477458

478-
join.entry(
479-
SPAN,
480-
&format_with(|f| {
481-
if let Some(element) = element.as_ref() {
482-
write!(f, group(element))?;
459+
if self.elements.is_empty() && self.rest.is_none() {
460+
write!(f, [format_dangling_comments(self.span()).with_block_indent()])?;
461+
} else {
462+
write!(
463+
f,
464+
group(&soft_block_indent(&format_once(|f| {
465+
if !self.elements.is_empty() {
466+
write_array_node(
467+
self.elements.len(),
468+
self.elements().iter().map(AstNode::as_ref),
469+
f,
470+
)?;
483471
}
484-
485-
if is_disallow {
486-
// Trailing separators are disallowed, replace it with an empty element
487-
// if let Some(separator) = element.trailing_separator()? {
488-
// write!(f, [format_removed(separator)])?;
489-
// }
490-
} else if is_force || index != last_index {
491-
// In forced separator mode or if this element is not the last in the list, print the separator
492-
// match element.trailing_separator()? {
493-
// Some(trailing) => write!(f, [trailing.format()])?,
494-
// None => text(",").fmt(f)?,
495-
// };
496-
",".fmt(f)?;
497-
// } else if let Some(separator) = element.trailing_separator()? {
498-
// match trailing_separator {
499-
// TrailingSeparator::Omit => {
500-
// // write!(f, [format_removed(separator)])?;
501-
// }
502-
// _ => {
503-
// write!(f, format_only_if_breaks(SPAN, separator))?;
504-
// }
505-
// }
506-
} else {
507-
write!(f, FormatTrailingCommas::ES5)?;
472+
if let Some(rest) = self.rest() {
473+
write!(f, [soft_line_break_or_space(), rest]);
508474
}
509-
510475
Ok(())
511-
}),
512-
);
476+
})))
477+
)?;
513478
}
514479

515-
join.finish()
480+
write!(f, "]")
516481
}
517482
}
518483

@@ -1109,11 +1074,28 @@ impl<'a> FormatWrite<'a> for AstNode<'a, BindingProperty<'a>> {
11091074
impl<'a> FormatWrite<'a> for AstNode<'a, ArrayPattern<'a>> {
11101075
fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> {
11111076
write!(f, "[")?;
1112-
if self.is_empty() {
1077+
1078+
if self.elements.is_empty() && self.rest.is_none() {
11131079
write!(f, [format_dangling_comments(self.span()).with_block_indent()])?;
11141080
} else {
1115-
// write!(f, [group(&soft_block_indent(&self.elements))])?;
1081+
write!(
1082+
f,
1083+
group(&soft_block_indent(&format_once(|f| {
1084+
if !self.elements.is_empty() {
1085+
write_array_node(
1086+
self.elements.len(),
1087+
self.elements().iter().map(AstNode::as_ref),
1088+
f,
1089+
)?;
1090+
}
1091+
if let Some(rest) = self.rest() {
1092+
write!(f, [soft_line_break_or_space(), rest]);
1093+
}
1094+
Ok(())
1095+
})))
1096+
)?;
11161097
}
1098+
11171099
write!(f, "]")
11181100
}
11191101
}

‎crates/oxc_formatter/src/write/utils/array.rs

Lines changed: 55 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use oxc_allocator::Vec;
22
use oxc_ast::ast::*;
3-
use oxc_span::{GetSpan, SPAN};
3+
use oxc_span::{GetSpan, SPAN, Span};
44

55
use crate::{
66
formatter::{FormatResult, Formatter, prelude::*},
@@ -10,72 +10,84 @@ use crate::{
1010
};
1111

1212
/// Utility function to print array-like nodes (array expressions, array bindings and assignment patterns)
13-
pub fn write_array_node<'a>(
14-
node: &AstNode<'a, Vec<'a, ArrayExpressionElement<'a>>>,
13+
pub fn write_array_node<'a, 'b, N>(
14+
len: usize,
15+
array: impl IntoIterator<Item = Option<&'a N>> + 'b,
1516
f: &mut Formatter<'_, 'a>,
16-
) -> FormatResult<()> {
17+
) -> FormatResult<()>
18+
where
19+
N: Format<'a> + GetSpan + std::fmt::Debug + 'a,
20+
{
1721
let trailing_separator = FormatTrailingCommas::ES5.trailing_separator(f.options());
1822

1923
// Specifically do not use format_separated as arrays need separators
2024
// inserted after holes regardless of the formatting since this makes a
2125
// semantic difference
2226

23-
let source_text = f.source_text();
27+
let last_index = len - 1;
28+
let source_text = f.context().source_text();
2429
let mut join = f.join_nodes_with_soft_line();
25-
let last_index = node.len().saturating_sub(1);
26-
2730
let mut has_seen_elision = false;
28-
for (index, element) in node.iter().enumerate() {
29-
let separator_mode = match element.as_ref() {
30-
ArrayExpressionElement::Elision(_) => TrailingSeparatorMode::Force,
31-
_ => TrailingSeparatorMode::Auto,
31+
32+
let mut array_iter = array.into_iter().enumerate().peekable();
33+
34+
while let Some((index, element)) = array_iter.next() {
35+
let separator_mode = if element.is_none() {
36+
TrailingSeparatorMode::Force
37+
} else {
38+
TrailingSeparatorMode::Auto
3239
};
3340

3441
let is_disallow = matches!(separator_mode, TrailingSeparatorMode::Disallow);
3542
let is_force = matches!(separator_mode, TrailingSeparatorMode::Force);
43+
3644
join.entry(
3745
// Note(different-with-Biome): this implementation isn't the same as Biome, because its output doesn't exactly match Prettier.
3846
if has_seen_elision {
3947
// Use fake span to avoid add any empty line between elision and expression element.
4048
SPAN
4149
} else {
4250
has_seen_elision = false;
43-
element.span()
51+
element.map_or_else(
52+
|| {
53+
// TODO: improve the `ArrayPattern` AST to simplify this logic.
54+
// Since `ArrayPattern` doesn't have a elision node, so that we have to find the comma position
55+
// by looking through the source text.
56+
let next_span =
57+
array_iter.peek().map_or(SPAN, |e| e.1.map_or(SPAN, GetSpan::span));
58+
59+
let comma_position = source_text.as_bytes()[..next_span.start as usize]
60+
.iter()
61+
.rev()
62+
.position(|c| *c == b',');
63+
64+
// comma span
65+
#[expect(clippy::cast_possible_truncation)]
66+
comma_position.map_or(SPAN, |pos| {
67+
Span::new(
68+
next_span.start - pos as u32,
69+
next_span.start - pos as u32 + 1,
70+
)
71+
})
72+
},
73+
GetSpan::span,
74+
)
4475
},
4576
&format_once(|f| {
46-
if element.is_elision() {
47-
has_seen_elision = true;
48-
return write!(f, ",");
49-
}
50-
51-
write!(f, group(&element))?;
77+
if let Some(element) = element {
78+
write!(f, group(&element))?;
5279

53-
if is_disallow {
54-
// Trailing separators are disallowed, replace it with an empty element
55-
// if let Some(separator) = element.trailing_separator()? {
56-
// write!(f, [format_removed(separator)])?;
57-
// }
58-
} else if is_force || index != last_index {
59-
// In forced separator mode or if this element is not the last in the list, print the separator
60-
// match element.trailing_separator()? {
61-
// Some(trailing) => write!(f, [trailing.format()])?,
62-
// None => text(",").fmt(f)?,
63-
// };
64-
",".fmt(f)?;
65-
// } else if let Some(separator) = element.trailing_separator()? {
66-
// match trailing_separator {
67-
// TrailingSeparator::Omit => {
68-
// // write!(f, [format_removed(separator)])?;
69-
// }
70-
// _ => {
71-
// write!(f, format_only_if_breaks(SPAN, separator))?;
72-
// }
73-
// }
80+
if is_disallow {
81+
Ok(())
82+
} else if is_force || index != last_index {
83+
",".fmt(f)
84+
} else {
85+
write!(f, FormatTrailingCommas::ES5)
86+
}
7487
} else {
75-
write!(f, FormatTrailingCommas::ES5)?;
88+
has_seen_elision = true;
89+
write!(f, ",")
7690
}
77-
78-
Ok(())
7991
}),
8092
);
8193
}
@@ -84,6 +96,7 @@ pub fn write_array_node<'a>(
8496
}
8597

8698
/// Determines if a trailing separator should be inserted after an array element
99+
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
87100
pub enum TrailingSeparatorMode {
88101
/// Trailing separators are not allowed after this element (eg. rest elements)
89102
Disallow,

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

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
1-
js compatibility: 413/699 (59.08%)
1+
js compatibility: 421/699 (60.23%)
22

33
# Failed
44

55
| Spec path | Failed or Passed | Match ratio |
66
| :-------- | :--------------: | :---------: |
77
| js/arrow-call/arrow_call.js | 💥💥💥 | 94.85% |
8-
| js/arrows/call.js | 💥💥 | 75.68% |
8+
| js/arrows/call.js | 💥💥 | 92.04% |
99
| js/arrows/comment.js | 💥💥 | 94.55% |
1010
| js/arrows/curried.js | 💥💥 | 92.55% |
1111
| js/arrows/currying-4.js | 💥💥 | 94.34% |
1212
| js/arrows/semi/semi.js | 💥✨ | 0.00% |
13-
| js/assignment/destructuring-array.js | 💥 | 0.00% |
1413
| js/assignment/destructuring-heuristic.js | 💥 | 39.02% |
1514
| js/assignment/issue-10218.js | 💥 | 52.63% |
1615
| js/assignment/issue-1419.js | 💥 | 28.57% |
@@ -24,15 +23,13 @@ js compatibility: 413/699 (59.08%)
2423
| js/binary-expressions/jsx_parent.js | 💥 | 33.85% |
2524
| js/binary-expressions/return.js | 💥 | 77.78% |
2625
| js/binary-expressions/test.js | 💥 | 95.65% |
27-
| js/break-calls/reduce.js | 💥 | 77.78% |
2826
| js/call/first-argument-expansion/jsx.js | 💥 | 0.00% |
2927
| js/call/first-argument-expansion/test.js | 💥 | 96.57% |
3028
| js/chain-expression/call-expression.js | 💥 | 42.86% |
31-
| js/chain-expression/issue-15785-1.js | 💥 | 60.87% |
32-
| js/chain-expression/issue-15785-2.js | 💥 | 50.00% |
29+
| js/chain-expression/issue-15785-1.js | 💥 | 78.26% |
30+
| js/chain-expression/issue-15785-2.js | 💥 | 66.67% |
3331
| js/chain-expression/issue-15785-3.js | 💥 | 50.00% |
3432
| js/chain-expression/member-expression.js | 💥 | 45.83% |
35-
| js/chain-expression/test-2.js | 💥 | 84.21% |
3633
| js/chain-expression/test-3.js | 💥 | 75.00% |
3734
| js/chain-expression/test.js | 💥 | 25.00% |
3835
| js/class-comment/class-property.js | 💥 | 30.77% |
@@ -109,7 +106,7 @@ js compatibility: 413/699 (59.08%)
109106
| js/decorators/class-expression/super-class.js | 💥💥 | 14.29% |
110107
| js/decorators-export/after_export.js | 💥 | 50.00% |
111108
| js/decorators-export/before_export.js | 💥 | 88.89% |
112-
| js/destructuring/destructuring.js | 💥 | 78.90% |
109+
| js/destructuring/destructuring.js | 💥 | 82.57% |
113110
| js/destructuring/issue-5988.js | 💥 | 0.00% |
114111
| js/destructuring-ignore/ignore.js | 💥💥💥 | 77.11% |
115112
| js/explicit-resource-management/for-await-using-of-comments.js | 💥 | 0.00% |
@@ -125,16 +122,13 @@ js compatibility: 413/699 (59.08%)
125122
| js/for/in.js | 💥 | 50.00% |
126123
| js/for/parentheses.js | 💥 | 72.00% |
127124
| js/for-of/async-identifier.js | 💥 | 90.00% |
128-
| js/function-comments/params-trail-comments.js | 💥 | 95.83% |
129-
| js/function-single-destructuring/array.js | 💥 | 35.42% |
130125
| js/functional-composition/functional_compose.js | 💥 | 93.20% |
131126
| js/functional-composition/pipe-function-calls-with-comments.js | 💥 | 77.08% |
132127
| js/functional-composition/pipe-function-calls.js | 💥 | 61.11% |
133128
| js/functional-composition/rxjs_pipe.js | 💥 | 45.45% |
134129
| js/identifier/for-of/await.js | 💥 | 33.33% |
135130
| js/identifier/for-of/let.js | 💥 | 61.54% |
136-
| js/identifier/parentheses/const.js | 💥💥 | 0.00% |
137-
| js/identifier/parentheses/let.js | 💥💥 | 79.09% |
131+
| js/identifier/parentheses/let.js | 💥💥 | 79.55% |
138132
| js/if/comment_before_else.js | 💥 | 61.54% |
139133
| js/if/expr_and_same_line_comments.js | 💥 | 86.67% |
140134
| js/if/if_comments.js | 💥 | 54.72% |
@@ -153,7 +147,6 @@ js compatibility: 413/699 (59.08%)
153147
| js/last-argument-expansion/edge_case.js | 💥 | 83.97% |
154148
| js/last-argument-expansion/issue-7518.js | 💥 | 85.71% |
155149
| js/last-argument-expansion/jsx.js | 💥 | 25.00% |
156-
| js/last-argument-expansion/object.js | 💥 | 94.74% |
157150
| js/line-suffix-boundary/boundary.js | 💥 | 30.43% |
158151
| js/logical_expressions/issue-7024.js | 💥 | 66.67% |
159152
| js/member/conditional.js | 💥 | 0.00% |
@@ -192,7 +185,6 @@ js compatibility: 413/699 (59.08%)
192185
| js/object-property-ignore/ignore.js | 💥💥💥 | 84.78% |
193186
| js/object-property-ignore/issue-5678.js | 💥💥💥 | 52.50% |
194187
| js/objects/right-break.js | 💥 | 70.27% |
195-
| js/objects/assignment-expression/object-value.js | 💥 | 85.71% |
196188
| js/optional-chaining/chaining.js | 💥 | 59.77% |
197189
| js/optional-chaining/comments.js | 💥 | 16.90% |
198190
| js/preserve-line/member-chain.js | 💥 | 23.30% |
@@ -201,7 +193,7 @@ js compatibility: 413/699 (59.08%)
201193
| js/quote-props/with_numbers.js | 💥💥✨✨ | 46.43% |
202194
| js/quotes/objects.js | 💥💥 | 80.00% |
203195
| js/require/require.js | 💥 | 90.67% |
204-
| js/rest/trailing-commas.js | 💥 | 84.75% |
196+
| js/rest/trailing-commas.js | 💥 | 95.08% |
205197
| js/return/binaryish.js | 💥 | 90.91% |
206198
| js/return/comment.js | 💥 | 63.01% |
207199
| js/sequence-break/break.js | 💥 | 46.85% |

0 commit comments

Comments
 (0)