-
-
Notifications
You must be signed in to change notification settings - Fork 788
fix(formatter): pass more prettier conformance cases #17995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…eading comments When a return/throw statement has a sequence expression argument with leading comments, the sequence expression needs inner parentheses to match Prettier's output. Example: ```js // Input return ( // comment a, b ); // Before (incorrect) return ( // comment a, b ) // After (matches Prettier) return ( // comment (a, b) ) ``` Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ow functions
When an `in` expression is inside a block-body arrow function that's in a
for loop initializer, it needs parentheses to avoid ambiguity.
Example:
```js
// Input
for (var a = (() => { b in c; });;);
// Before (incorrect)
for (
var a = () => {
b in c;
};
;
);
// After (matches Prettier)
for (
var a = () => {
(b in c);
};
;
);
```
The fix extends `is_in_for_initializer` to handle both expression-body
arrows (`() => b in c`) and block-body arrows (`() => { b in c; }`).
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add parentheses around `let` identifier in for-in loop left side when needed: - `let[a]` needs parentheses: `(let)[a]` (looks like lexical declaration otherwise) - `let.a` doesn't need parentheses (unambiguous member access) For-of already correctly handles all cases. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…unction sequence expressions
When an arrow function body is a sequence expression with leading comments,
format the comments before the parentheses, not inside them.
Example:
```js
// Before
func(() =>
(
// comment
a, b, c
),
);
// After
func(() =>
// comment
(a, b, c),
);
```
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove FormatCommentForEmptyStatement from ForOfStatement - Add space before EmptyStatement with leading comments in FormatStatementBody This fixes: - js/for/9812.js - js/comments/dangling_for.js - js/explicit-resource-management/for-await-using-of-comments.js JS compatibility: 739/761 → 742/761 (97.11% → 97.50%) Co-Authored-By: Claude Opus 4.5 <[email protected]>
`let[a]` at statement start looks like a lexical declaration, so it needs parentheses. This fix properly detects when `let` is at the leftmost position of a statement by traversing the "left-spine" of the AST (member expressions, call expressions, binary/logical expressions, etc.). Co-Authored-By: Claude Opus 4.5 <[email protected]>
…w function bodies When a sequence expression is used as the body of an arrow function with a leading `prettier-ignore` comment, the formatter now correctly preserves the original source including the parentheses. Example: ```javascript const a = ()=>()=> // prettier-ignore (a,b) ``` The fix finds the parentheses positions around the sequence expression since the SequenceExpression span doesn't include them, and uses `FormatSuppressedNode` to print the original source verbatim. JS conformance improved from 742/761 (97.50%) to 743/761 (97.63%). Co-Authored-By: Claude Opus 4.5 <[email protected]>
When a sequence expression in an arrow function body has a leading comment, keep the sequence expression on a single line instead of breaking it across multiple lines. JS conformance improved from 743/761 (97.63%) to 744/761 (97.77%). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Exclude supplementary Unicode characters (codepoints > 0xFFFF) from
being unquoted as property keys. This matches ES5 identifier rules
used by Prettier for compatibility.
Example: `{ "𐊧": true }` now correctly keeps the quotes instead of
being converted to `{ 𐊧: true }`.
JS conformance improved from 744/761 (97.77%) to 745/761 (97.90%).
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Expressions starting with keywords like `new`, `await`, and `yield` don't need leading semicolons for ASI protection because the keyword at line start prevents ASI issues. Example: `new (let[0] = 1)()` no longer gets a leading semicolon with `semi: false`. JS conformance improved from 745/761 (97.90%) to 746/761 (98.03%). Co-Authored-By: Claude Opus 4.5 <[email protected]>
When quoteProps is "consistent" and some property requires quotes, numeric keys should also be quoted where possible. This handles: - Simple numeric keys like 1, 1.5 get quoted - Partial decimal forms like .1, 1. are normalized (to 0.1, 1) - Complex forms (1.0, 1E2, 0b10, 0xf) stay unquoted to preserve intent - Numbers that lose precision (999999999999999999999) stay unquoted Co-Authored-By: Claude Opus 4.5 <[email protected]>
When a line comment appears between a control structure (for-of, while, etc.)
and its block body, Prettier formats it as an end-of-line comment with the
block starting on a new line:
```js
for (x of y) // comment
{
```
Block comments do not force this line break behavior.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the formatter's Prettier conformance by fixing several formatting issues related to comments, identifier parenthesization, numeric property keys, and ASI (Automatic Semicolon Insertion) protection. The changes significantly improve JavaScript compatibility (96.85% → 98.42%) but introduce a minor TypeScript regression (95.70% → 95.54%).
Changes:
- Added supplementary Unicode character filtering for ES5 identifier compatibility
- Enhanced comment handling for empty statements and block statements in control structures
- Implemented numeric key quoting logic for the
quoteProps: "consistent"option - Fixed
letidentifier parenthesization to prevent ambiguous lexical declarations - Improved ASI protection by excluding keyword-prefixed expressions
- Fixed sequence expression formatting in return/throw statements with comments
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/prettier_conformance/snapshots/prettier.js.snap.md | Documents JS compatibility improvement from 96.85% to 98.42% (12 tests fixed) |
| tasks/prettier_conformance/snapshots/prettier.ts.snap.md | Documents TS compatibility decrease from 95.70% to 95.54% (1 new failure) |
| crates/oxc_formatter/src/utils/string.rs | Adds supplementary Unicode character filtering for ES5 identifier rules |
| crates/oxc_formatter/src/utils/statement_body.rs | Enhances comment handling for empty and block statements |
| crates/oxc_formatter/src/utils/object.rs | Implements numeric key quoting logic with normalization |
| crates/oxc_formatter/src/print/sequence_expression.rs | Adds suppressed node handling for sequence expressions |
| crates/oxc_formatter/src/print/return_or_throw_statement.rs | Fixes sequence expression parenthesization with leading comments |
| crates/oxc_formatter/src/print/mod.rs | Removes ForOfStatement empty comment handling and improves ASI protection |
| crates/oxc_formatter/src/print/arrow_function_expression.rs | Handles suppressed sequence expressions in arrow functions |
| crates/oxc_formatter/src/parentheses/expression.rs | Refines let identifier parenthesization and fixes in operator handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // These should be printed as end-of-line comments with the block on a new line | ||
| // Block comments (`/* */`) don't force a new line | ||
| let own_line_comments = | ||
| f.context().comments().own_line_comments_before(block.span.start); | ||
| let has_line_comment = own_line_comments.iter().any(|c| c.is_line()); | ||
|
|
||
| if has_line_comment { | ||
| // Format comments as end-of-line (on the same line as header, not preserving newlines) | ||
| for comment in own_line_comments { | ||
| f.context_mut().comments_mut().increment_printed_count(); | ||
| let content = format_with(|f| write!(f, [space(), comment])); | ||
| if comment.is_line() { | ||
| write!(f, [line_suffix(&content), expand_parent()]); | ||
| } else { | ||
| write!(f, content); | ||
| } | ||
| } | ||
| write!(f, [hard_line_break()]); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment handling logic for line comments before block statements is complex and could benefit from additional inline comments explaining why line comments are treated as end-of-line comments with line_suffix while block comments are formatted directly. The distinction between is_line() checks on lines 54, 61 could be clearer with more explanation of the formatting strategy.
| // These should be printed as end-of-line comments with the block on a new line | |
| // Block comments (`/* */`) don't force a new line | |
| let own_line_comments = | |
| f.context().comments().own_line_comments_before(block.span.start); | |
| let has_line_comment = own_line_comments.iter().any(|c| c.is_line()); | |
| if has_line_comment { | |
| // Format comments as end-of-line (on the same line as header, not preserving newlines) | |
| for comment in own_line_comments { | |
| f.context_mut().comments_mut().increment_printed_count(); | |
| let content = format_with(|f| write!(f, [space(), comment])); | |
| if comment.is_line() { | |
| write!(f, [line_suffix(&content), expand_parent()]); | |
| } else { | |
| write!(f, content); | |
| } | |
| } | |
| write!(f, [hard_line_break()]); | |
| // These should be printed as end-of-line comments with the block on a new line. | |
| // | |
| // `own_line_comments_before` may return both line (`//`) and block (`/* */`) comments. | |
| // We only *change* the layout when there is at least one line comment: line comments | |
| // conceptually belong to the end of the control header line, so we later attach them | |
| // using `line_suffix`. Block comments in this position don't force a new layout and | |
| // are formatted as regular inline trivia. | |
| let own_line_comments = | |
| f.context().comments().own_line_comments_before(block.span.start); | |
| let has_line_comment = own_line_comments.iter().any(|c| c.is_line()); | |
| if has_line_comment { | |
| // When there is at least one line comment, we print *all* the collected comments | |
| // in a single pass before the block: | |
| // - line comments are attached as end-of-line comments via `line_suffix`, so they | |
| // appear on the same line as the control header; | |
| // - block comments are emitted directly and do not participate in `line_suffix`. | |
| // | |
| // This is why we first compute `has_line_comment` (to decide this layout) and then | |
| // do a per-comment `is_line()` check inside the loop. | |
| for comment in own_line_comments { | |
| f.context_mut().comments_mut().increment_printed_count(); | |
| let content = format_with(|f| write!(f, [space(), comment])); | |
| if comment.is_line() { | |
| // Treat the line comment as an end-of-line comment of the header rather than | |
| // preserving the original newline between the header and the block. | |
| write!(f, [line_suffix(&content), expand_parent()]); | |
| } else { | |
| // Block comments are formatted directly without `line_suffix`, so they keep | |
| // behaving like regular inline trivia. | |
| write!(f, content); | |
| } | |
| } | |
| write!(f, [hard_line_break()]); | |
| // After printing the header and its trailing comments, place the block on the next | |
| // line. We call `block.write(f)` so that the block is printed without re-emitting | |
| // its leading comments (those are handled inside the block itself, see the branch | |
| // below which also uses `block.write(f)` for the same reason). |
| space(), | ||
| "of", | ||
| space(), | ||
| right, |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of FormatCommentForEmptyStatement(body) from the ForOfStatement formatting creates an inconsistency with ForInStatement (line 693) and ForStatement (lines 657, 661, 665). While FormatStatementBody now handles empty statement comments before the body itself, ForInStatement still explicitly includes FormatCommentForEmptyStatement(body) before the closing parenthesis. Either both ForInStatement and ForOfStatement should include this, or both should remove it. Please verify this is intentional and the behavior is correct for all cases.
| right, | |
| right, | |
| FormatCommentForEmptyStatement(body), |
| if init.is_none() && test.is_none() && update.is_none() { | ||
| let comments = f.context().comments().comments_before(body.span().start); | ||
| if !comments.is_empty() { | ||
| write!( | ||
| f, | ||
| [ | ||
| FormatDanglingComments::Comments { | ||
| comments, | ||
| indent: DanglingIndentMode::None | ||
| }, | ||
| soft_line_break_or_space() | ||
| ] | ||
| ); | ||
| } | ||
| return write!(f, [group(&format_args!("for", space(), "(;;)", format_body))]); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early return for empty for loops for (;;) on line 644 now skips comment handling that was previously done before this return. If the body is an empty statement with leading comments (e.g., for (;;) /* comment */ ;), those comments should still be formatted. The FormatStatementBody::new(body) should handle this, but verify that this change produces correct output for cases like for (;;) /* comment */ ; or for (;;) // comment\n;.
| | typescript/multiparser-css/issue-6259.ts | 💥 | 57.14% | | ||
| | typescript/non-null/optional-chain.ts | 💥 | 72.22% | | ||
| | typescript/property-signature/consistent-with-flow/comments.ts | 💥 | 80.00% | | ||
| | typescript/quote-props/types.ts | 💥✨✨ | 29.63% | |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces a new test failure in typescript/quote-props/types.ts (line 30), reducing TypeScript compatibility from 95.70% to 95.54%. While the overall JS compatibility improves significantly, introducing a new TypeScript regression should be investigated. The quote-props changes in object.rs may be causing this. Consider whether the numeric key quoting logic needs TypeScript-specific handling or if this is an acceptable tradeoff.
| if f.context().comments().is_suppressed(sequence.span().start) { | ||
| // Find the parentheses around the sequence expression | ||
| let source_text = f.source_text(); | ||
| // Search backward for '(' | ||
| let paren_start = source_text | ||
| .slice_to(sequence.span().start) | ||
| .rfind('(') | ||
| .map(|i| i as u32) | ||
| .unwrap_or(sequence.span().start); | ||
| // Search forward for ')' | ||
| let seq_end = sequence.span().end as usize; | ||
| let paren_end = source_text | ||
| .slice_from(sequence.span().end) | ||
| .find(')') | ||
| .map(|i| (seq_end + i + 1) as u32) | ||
| .unwrap_or(sequence.span().end); | ||
| let suppressed_span = Span::new(paren_start, paren_end); | ||
| write!( | ||
| f, | ||
| [group(&format_args!( | ||
| formatted_signature, | ||
| group(&format_args!(indent(&format_args!( | ||
| hard_line_break(), | ||
| format_leading_comments(sequence.span()), | ||
| FormatSuppressedNode(suppressed_span) | ||
| )))) | ||
| ))] | ||
| ); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for finding parentheses around a suppressed sequence expression (lines 149-164) is duplicated in lines 601-616. Consider extracting this into a helper function to reduce code duplication and improve maintainability. The helper could be named something like find_parentheses_span_around_sequence and return a Span.
| if source.starts_with('.') { | ||
| // ".1" style - can quote | ||
| return true; | ||
| } | ||
| if after_dot.is_empty() { | ||
| // "1." style - can quote | ||
| return true; | ||
| } | ||
| // Check if any trailing zeros would be lost | ||
| if after_dot.ends_with('0') { | ||
| return false; | ||
| } |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic has an issue with early returns. Lines 143-145 return true early if the source starts with '.' (e.g., ".1"), but this code is only reached if dot_pos exists (line 140). Since source.starts_with('.') means dot_pos would be 0, the variable after_dot would be the substring after position 0, which is everything after the dot. This early return prevents checking whether the trailing zeros logic (line 152) should apply.
For example, if the source is ".10", this would return true early even though the trailing zero should prevent quoting. The logic should be restructured to handle all decimal cases consistently.
| if source.starts_with('.') { | |
| // ".1" style - can quote | |
| return true; | |
| } | |
| if after_dot.is_empty() { | |
| // "1." style - can quote | |
| return true; | |
| } | |
| // Check if any trailing zeros would be lost | |
| if after_dot.ends_with('0') { | |
| return false; | |
| } | |
| if after_dot.is_empty() { | |
| // "1." style - can quote | |
| return true; | |
| } | |
| // Check if any trailing zeros would be lost (e.g., "1.0", ".10") | |
| if after_dot.ends_with('0') { | |
| return false; | |
| } | |
| if source.starts_with('.') { | |
| // ".1" style without trailing zeros - can quote | |
| return true; | |
| } |
| format!("{}", value as i64) | ||
| } else { | ||
| format!("{value}") | ||
| } | ||
| } | ||
|
|
||
| /// Normalize source text to a comparable numeric string. | ||
| /// Handles cases like ".1" -> "0.1", "1." -> "1" | ||
| fn normalize_source_numeric(source: &str) -> String { | ||
| // Handle ".1" style (leading dot) | ||
| if source.starts_with('.') { | ||
| return format!("0{source}"); | ||
| } | ||
|
|
||
| // Handle "1." style (trailing dot) | ||
| if let Some(stripped) = source.strip_suffix('.') { |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normalization in normalize_source_numeric does not match JavaScript's actual behavior for all edge cases. For example, "00123" would be returned as-is and compared against the f64-normalized value "123", which would fail the comparison even though both represent the same number. Consider parsing the source as an f64 and normalizing it using the same normalize_numeric_key function for consistency, or handle leading zeros explicitly.
TypeScript class property declarations with type annotations (e.g., `"username": string`) should preserve quotes on string keys, while JavaScript class properties with value assignments should follow the quoteProps option. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
I would upgrade Prettier first to avoid some outdated matches, as some of the test outputs I have already sent issues to Prettier to request changes. |
This PR is split by commit.