fix(codegen): emit pife-arrow/function leading comments inside the wrap by Dunqing · Pull Request #22720 · oxc-project/oxc

Dunqing · 2026-05-25T13:00:46Z

Summary

When a pife-flagged arrow or function expression is the alternate of a ConditionalExpression, codegen wraps it in parens but was emitting any leading comment before the wrap — moving the comment from inside the parens (where the user wrote it) to outside on every codegen pass.

// Input
export const x = foo ? bar : (
  // explanatory comment   ← INSIDE the parens
  (a, b) => a + b
);

// Before fix (single codegen pass) — comment moved OUTSIDE the parens
export const x = foo ? bar :
// explanatory comment
((a, b) => a + b);

The mis-positioning then breaks idempotency: re-parsing the output anchors the comment at the outer ( (different from the original parse's inner-arrow anchor), and the next codegen's exact-match get_comments(alternate.span.start) lookup misses, silently dropping the comment on the second pass.

Fix

ConditionalExpression::gen_expr defers leading-comment printing when the alternate is a pife arrow/function — those nodes know they will emit a wrap and now print their own leading comment from inside the wrap, before the keyword/params:

p.wrap(precedence >= Precedence::Assign || self.pife, |p| {
    if self.pife
        && let Some(comments) = p.get_comments(self.span.start)
    {
        p.print_comments(&comments);
        p.consume_pending_indent_space();
    }
    // ... rest of arrow/function printing
});

The same handling is added to Function::gen for the function-expression pife case. Both non-pife and statement-start wraps are unchanged.

After the fix:

// Output (single pass, and stable on every re-pass)
export const x = foo ? bar : (
// explanatory comment   ← still inside the parens
(a, b) => a + b);

Why this rather than a comment-lookup workaround

A range-tolerant comment lookup (or comment re-anchoring on paren-strip) can make pass 2 find the dropped comment again — but the first pass would still emit the comment in the wrong source position. This fix addresses the actual first-pass codegen bug, so the comment stays where the user wrote it.

Impact

End-to-end via monitor-oxc DCE runner on the 116,429-file npm-top-3000 corpus: 9 → 1 idempotency failures. The remaining 1 is an unrelated unused-variable fixed-point bug (var __webpack_exports__ = {}; in @jest/globals/build/index.js).

cargo test -p oxc_codegen → 107 passed (was 105; +2 new tests)
cargo test -p oxc_minifier → 504 passed, 0 failed
just minsize → snapshots unchanged
just allocs → snapshots unchanged

Tests:

comments::test_comment_inside_pife_arrow_alternate_of_conditional
comments::test_comment_inside_pife_function_alternate_of_conditional

Closes / supersedes

This supersedes the workaround approach in #22710 (which re-anchored comments on paren-strip from inside Normalize). The pife-wrap fix is at the actual layer where the wrong decision was made — codegen choosing to emit a comment outside a wrap it's about to add — and preserves source-text fidelity rather than tolerating its loss.

codspeed-hq · 2026-05-25T13:09:53Z

Merging this PR will not alter performance

✅ 57 untouched benchmarks
⏩ 3 skipped benchmarks¹

_{Comparing fix/codegen-comment-inside-pife-wrap (db5211a) with main (661132d)}

3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

oxc-guard · 2026-05-25T14:20:24Z

Monitor Oxc

suite	result
Build	✅
Isolated Declarations	✅
(whitespace)	✅
(dce)	❌
(formatter)	❌
(formatter_dcr)	✅
(transformer)	✅
(mangler)	❌
(compressor)	✅
(codegen)	✅

Dunqing · 2026-05-25T14:26:24Z

Confirmed that no comments-related tests failed in DCE now https://github.com/oxc-project/monitor-oxc/actions/runs/26405173743/job/77726975626

Dunqing · 2026-05-25T14:29:08Z

Merge activity

May 25, 2:29 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
May 25, 2:29 PM UTC: Dunqing added this pull request to the Graphite merge queue.
May 25, 2:33 PM UTC: Merged by the Graphite merge queue.

…ap (#22720) close: #22710 ## Summary When a `pife`-flagged arrow or function expression is the alternate of a `ConditionalExpression`, codegen wraps it in parens but was emitting any leading comment **before** the wrap — moving the comment from inside the parens (where the user wrote it) to outside on every codegen pass. ```js // Input export const x = foo ? bar : ( // explanatory comment ← INSIDE the parens (a, b) => a + b ); // Before fix (single codegen pass) — comment moved OUTSIDE the parens export const x = foo ? bar : // explanatory comment ((a, b) => a + b); ``` The mis-positioning then breaks idempotency: re-parsing the output anchors the comment at the outer `(` (different from the original parse's inner-arrow anchor), and the next codegen's exact-match `get_comments(alternate.span.start)` lookup misses, silently dropping the comment on the second pass. ## Fix `ConditionalExpression::gen_expr` defers leading-comment printing when the alternate is a `pife` arrow/function — those nodes know they will emit a wrap and now print their own leading comment from inside the wrap, before the keyword/params: ```rust p.wrap(precedence >= Precedence::Assign || self.pife, |p| { if self.pife && let Some(comments) = p.get_comments(self.span.start) { p.print_comments(&comments); p.consume_pending_indent_space(); } // ... rest of arrow/function printing }); ``` The same handling is added to `Function::gen` for the function-expression `pife` case. Both non-`pife` and statement-start wraps are unchanged. After the fix: ```js // Output (single pass, and stable on every re-pass) export const x = foo ? bar : ( // explanatory comment ← still inside the parens (a, b) => a + b); ``` ## Why this rather than a comment-lookup workaround A range-tolerant comment lookup (or comment re-anchoring on paren-strip) can make pass 2 *find* the dropped comment again — but the first pass would still emit the comment in the wrong source position. This fix addresses the actual first-pass codegen bug, so the comment stays where the user wrote it. ## Impact End-to-end via monitor-oxc DCE runner on the 116,429-file npm-top-3000 corpus: **9 → 1 idempotency failures**. The remaining 1 is an unrelated unused-variable fixed-point bug (`var __webpack_exports__ = {};` in `@jest/globals/build/index.js`). - `cargo test -p oxc_codegen` → 107 passed (was 105; +2 new tests) - `cargo test -p oxc_minifier` → 504 passed, 0 failed - `just minsize` → snapshots unchanged - `just allocs` → snapshots unchanged Tests: - `comments::test_comment_inside_pife_arrow_alternate_of_conditional` - `comments::test_comment_inside_pife_function_alternate_of_conditional` ## Closes / supersedes This supersedes the workaround approach in #22710 (which re-anchored comments on paren-strip from inside `Normalize`). The pife-wrap fix is at the actual layer where the wrong decision was made — codegen choosing to emit a comment outside a wrap it's about to add — and preserves source-text fidelity rather than tolerating its loss.

### 🚀 Features - e857b0c napi/minify: Expose legalComments option and result (#20370) (Boshen) - 661132d parser: More friendly error messages for rest assignment target and rest binding element (#22719) (sapphi-red) - ee659b6 transformer/legacy-decorator: Add `strictNullChecks` option for nullable-union design:type (#22266) (Kyle Cannon) ### 🐛 Bug Fixes - e1d064e transformer/class-properties: Reparent lifted private method helpers (#22716) (Cameron) - 4ac0fca minifier: Preserve `0 && (module.exports = { ... })` cjs-module-lexer hint (#22729) (Dunqing) - 40ff611 minifier: Mark peephole loop changed when dropping dead-after-throw statement (#22722) (Dunqing) - 2f7b210 codegen: Emit pife-arrow/function leading comments inside the wrap (#22720) (Dunqing) - e184f74 parser: Improve invalid `import` property access diagnostic (#22693) (camc314) - 7baed9c transformer/private-method: Clear inherited strict flags (#22508) (camc314) - a9ad27e parser: Keep annotation comments leading without preceding newline (#22711) (Dunqing) - 9ea4d64 minifier: Re-evaluate pure/no-side-effects flags after peephole inlining (#22595) (Dunqing) - 07afbb6 minifier: Drop empty-body IIFE wrapper when called with arguments (#22589) (Dunqing) - fa7c463 semantic: Correct TS enum member symbol spans (#22689) (camc314) - 26b9396 semantic: Resolve parameter decorators outside parameter scope (#22623) (camc314) - b284045 parser: Switch to module goal eagerly on `export` (#22684) (Boshen) - dfa931d semantic: Propagate unresolved auto-increment enum value instead of defaulting to 0 (#22646) (Dunqing) - 69a6ba6 transformer/legacy-decorator: Emit Array for ReadonlyArray<T> in decorator metadata (#22265) (Kyle Cannon) - e421ef0 transformer/legacy-decorator: Return runtime binding for design:type (#22640) (Dunqing) - d61e1d7 codegen: Preserve verbatim text of pure/no-side-effects comments (#22525) (Dunqing) - 702b14e minifier: Preserve IIFE structure in DCE-only mode (#22547) (Dunqing) - 917da24 parser: Apply PURE comment through member-access chains (#22566) (Dunqing) - a069b1c codegen: Preserve quotes for cjs-module-lexer equality strings (#22551) (Dunqing) ### ⚡ Performance - 2f623b0 semantic: Skip unresolved checks for re-exports (#22660) (camc314) - 0d9553d semantic: Early-exit `check_object_expression` for objects with <2 properties (#22668) (Dunqing) - d721ad9 semantic: Use direct grandparent lookup for TS type parameters (#22658) (camc314) - 0aff288 semantic: Reorder numeric literal strict mode checks (#22657) (camc314) - 4d5ddb1 semantic: Reorder binding identifier checks (#22656) (camc314) - e32acd8 semantic: Reorder identifier ambient binding check (#22653) (camc314) - 09fe178 semantic: Reorder ident reference strict mode check (#22652) (camc314) - 4b6add2 semantic: Avoid duplicate ident clone for bindings (#22663) (camc314) - 82f9662 parser: Check identifier kind before context flag (#22662) (camc314) - d7cd951 parser: Fast path identifier parsing and inline operator helpers (#22650) (Boshen) - 7b84314 semantic: Use direct byte access for numeric leading-zero check (#22642) (camc314) - 0345a31 semantic: Pre-size class elements hash map (#22618) (camc314) - 04d3065 minifier: Drop per-call buffers in try_fold_concat (#22596) (Dunqing) - 4f289f1 semantic: Resolve_references_for_current_scope without a temp Vec (#22599) (Dunqing) - e862c15 semantic: Avoid heap alloc for var hoist scope ids (#22603) (Dunqing) - 8ff8674 semantic: Early return if `excess` is `0` in `Stats::increase_by` (#22616) (camc314) - 7a4120e semantic: Pre-reserve unresolved_references using Stats::references (#22580) (Dunqing) Co-authored-by: Dunqing <[email protected]>

github-actions Bot added the A-codegen Area - Code Generation label May 25, 2026

Dunqing force-pushed the fix/codegen-comment-inside-pife-wrap branch 2 times, most recently from 2931137 to 2051197 Compare May 25, 2026 14:06

Dunqing marked this pull request as ready for review May 25, 2026 14:10

Dunqing requested a review from Boshen May 25, 2026 14:10

Dunqing added the run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it label May 25, 2026

Dunqing added the 0-merge Merge with Graphite Merge Queue label May 25, 2026

graphite-app Bot force-pushed the fix/codegen-comment-inside-pife-wrap branch from db5211a to 2f7b210 Compare May 25, 2026 14:29

graphite-app Bot merged commit 2f7b210 into main May 25, 2026
29 checks passed

graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 25, 2026

graphite-app Bot deleted the fix/codegen-comment-inside-pife-wrap branch May 25, 2026 14:33

oxc-guard Bot mentioned this pull request May 26, 2026

release(crates): oxc v0.133.0 #22734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(codegen): emit pife-arrow/function leading comments inside the wrap#22720

fix(codegen): emit pife-arrow/function leading comments inside the wrap#22720
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-comment-inside-pife-wrap

Dunqing commented May 25, 2026 •

edited

Loading

Uh oh!

codspeed-hq Bot commented May 25, 2026 •

edited

Loading

Uh oh!

oxc-guard Bot commented May 25, 2026 •

edited

Loading

Uh oh!

Dunqing commented May 25, 2026

Uh oh!

Dunqing commented May 25, 2026 •

edited by graphite-app Bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Dunqing commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Why this rather than a comment-lookup workaround

Impact

Closes / supersedes

Uh oh!

codspeed-hq Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

oxc-guard Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Monitor Oxc

Uh oh!

Dunqing commented May 25, 2026

Uh oh!

Dunqing commented May 25, 2026 • edited by graphite-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Dunqing commented May 25, 2026 •

edited

Loading

codspeed-hq Bot commented May 25, 2026 •

edited

Loading

oxc-guard Bot commented May 25, 2026 •

edited

Loading

Dunqing commented May 25, 2026 •

edited by graphite-app Bot

Loading