Skip to content

fix(minifier): re-anchor leading comments when stripping a ParenthesizedExpression alternate#22710

Closed
Dunqing wants to merge 2 commits into
mainfrom
fix/minifier-remap-paren-comment-on-strip
Closed

fix(minifier): re-anchor leading comments when stripping a ParenthesizedExpression alternate#22710
Dunqing wants to merge 2 commits into
mainfrom
fix/minifier-remap-paren-comment-on-strip

Conversation

@Dunqing

@Dunqing Dunqing commented May 25, 2026

Copy link
Copy Markdown
Member

Summary

A single DCE pass produced output where a second pass dropped the explanatory comment on a conditional-expression alternate. Verified reproduction (one of 8 in monitor-oxc's npm corpus):

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

// First DCE pass — comment preserved between `:` and the arrow:
export const x = foo ? bar :
// explanatory comment
((a, b) => a + b);

// Second DCE pass on the first pass's output — comment dropped:
export const x = foo ? bar : ((a, b) => a + b);

Root cause

The parser anchors a leading comment at the next token's start (crates/oxc_parser/src/lexer/trivia_builder.rs). Between the two parses the next token after the comment shifts:

Pass Source around comment Next token comment.attached_to
1 : ( /*c*/ (a, b) => … inner ( of arrow params ArrowFn.span.start
2 : \n/*c*/\n ((a, b) => … outer ( of parens ParenthesizedExpression.span.start

Normalize::exit_expression unconditionally strips the ParenthesizedExpression (codegen re-emits the wrapping parens via the inner arrow's pife: true flag, so the strip is normally invisible). After the pass-2 strip the comment is keyed at a position that no AST node starts at; codegen's ConditionalExpression alternate uses an exact-match get_comments(alternate.span.start) lookup and finds nothing.

Fix

When Normalize strips a ParenthesizedExpression whose ( anchors a normal (non-jsdoc / non-annotation / non-legal) comment and the paren sits in a ConditionalExpressionAlternate slot, record a (paren_start → inner_start) re-anchoring in the visitor and apply it to program.comments in build. The remap resolves transitively so nested ((x)) strips both parens.

The scope is intentionally narrow:

  • Normal comments only. JSDoc type casts (/** @type {*} */ (x)) and annotation comments (/* @__PURE__ */) are tied to the paren position by tooling that processes them; legal comments (@license / @preserve) have their own statement-level emission pipeline. Re-anchoring those silently shifts verbatim text from a statement boundary into a call's callee, breaking other idempotency tests.

  • ConditionalExpressionAlternate parent only. When the paren is a direct child of ExpressionStatement / VariableDeclarator / similar, the statement-level print_comments_at(span.start) already pulls the comment via the parent's own start (which coincides with the paren start), so re-anchoring there would lose the comment. Verified failure mode on top-of-file legal-style line comments before IIFEs in d3-min.js. The conditional-expression alternate is the one expression-context site with its own get_comments lookup.

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_minifier → 505 passed (was 504; +1 new test), 0 failed
  • just minsize → snapshot unchanged across all 12 fixtures
  • just allocs → small bump from one FxHashSet allocation per Normalize run plus an O(comments) scan

Tests:

  • dead_code_elimination::comment_preserved_when_paren_around_arrow_alternate_is_stripped

…zedExpression alternate

A single DCE pass produced output where a second pass *dropped* the
explanatory comment on a conditional-expression alternate. Verified
reproduction (one of 8 in monitor-oxc's npm corpus):

    export const x = foo ? bar : (
      // explanatory comment
      (a, b) => a + b
    );

    // First DCE pass — comment preserved between `:` and the arrow:
    export const x = foo ? bar :
    // explanatory comment
    ((a, b) => a + b);

    // Second DCE pass on the first pass's output — comment dropped:
    export const x = foo ? bar : ((a, b) => a + b);

## Root cause

The parser anchors a leading comment at the *next token's start*
(`crates/oxc_parser/src/lexer/trivia_builder.rs`). Between the two
parses the next token after the comment shifts:

  Pass 1: `: ( /*c*/ (a, b) => …`   → `attached_to` = inner `(` of arrow params
  Pass 2: `: \n/*c*/\n ((a, b) => …` → `attached_to` = outer `(` of parens

`Normalize::exit_expression` unconditionally strips the
`ParenthesizedExpression` (codegen re-emits the wrapping parens via the
inner arrow's `pife: true` flag, so the strip is normally invisible).
After the pass-2 strip the comment is keyed at a position that no AST
node starts at; codegen's `ConditionalExpression` alternate uses an
exact-match `get_comments(alternate.span.start)` lookup and finds
nothing.

## Fix

When `Normalize` strips a `ParenthesizedExpression` whose `(` anchors a
*normal* (non-jsdoc / non-annotation / non-legal) comment *and* the paren
sits in a `ConditionalExpressionAlternate` slot, record a (paren_start →
inner_start) re-anchoring in the visitor and apply it to
`program.comments` in `build`. The remap resolves transitively so
nested `((x))` strips both parens.

The scope is intentionally narrow:

- **Normal comments only.** JSDoc type casts (`/** @type {*} */ (x)`)
  and annotation comments (`/* @__PURE__ */`) are tied to the paren
  position by tooling that processes them; legal comments
  (`@license` / `@preserve`) have their own statement-level emission
  pipeline. Re-anchoring those silently shifts verbatim text from a
  statement boundary into a call's callee, breaking other idempotency
  tests.

- **`ConditionalExpressionAlternate` parent only.** When the paren is
  a direct child of `ExpressionStatement` / `VariableDeclarator` /
  similar, the statement-level `print_comments_at(span.start)` already
  pulls the comment via the parent's own start (which coincides with
  the paren start), so re-anchoring there would *lose* the comment.
  Verified failure mode on top-of-file legal-style line comments
  before IIFEs in d3-min.js. The conditional-expression alternate is
  the one expression-context site with its own `get_comments` lookup.

## 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_minifier` → 505 passed (was 504; +1 new test), 0 failed.
`just minsize` → snapshot unchanged across all 12 fixtures.
`just allocs` → small bump from one `FxHashSet` allocation per
`Normalize` run plus an `O(comments)` scan.

Tests:
- `dead_code_elimination::comment_preserved_when_paren_around_arrow_alternate_is_stripped`
@github-actions github-actions Bot added the A-minifier Area - Minifier label May 25, 2026
@codspeed-hq

codspeed-hq Bot commented May 25, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 52 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing fix/minifier-remap-paren-comment-on-strip (392569a) with main (9ea4d64)

Open in CodSpeed

Footnotes

  1. 8 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.

@graphite-app graphite-app Bot closed this in 2f7b210 May 25, 2026
@Boshen Boshen deleted the fix/minifier-remap-paren-comment-on-strip branch May 31, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant