fix(minifier): preserve 0 && (module.exports = { ... }) cjs-module-lexer hint#22729
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Monitor Oxc
|
0a13eb6 to
ec974a2
Compare
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
7c7597b to
8626ad0
Compare
f773b40 to
f27adf6
Compare
836bd21 to
bc5d190
Compare
f27adf6 to
d46a46a
Compare
bc5d190 to
fe67f83
Compare
5d99750 to
aee7f70
Compare
Merge activity
|
…lexer hint (#22729) ## Summary esbuild emits unreachable `0 && (module.exports = { ... })` statements on Node platform as parse-time hints for [`cjs-module-lexer`](https://github.com/nodejs/cjs-module-lexer). The actual exports happen through helper calls (`__export(...)` or similar) that the lexer can't trace, so it scans for the literal `module.exports = { ... }` shape inside this dead-code expression to discover named exports. Exact emission site (esbuild v0.28.0): [`internal/linker/linker.go#L5127-L5138`](https://github.com/evanw/esbuild/blob/v0.28.0/internal/linker/linker.go#L5127-L5138). Always `LogicalAnd(0, Assign(module.exports, ObjectExpression))` — no other shape. oxc's DCE judges the expression by JS-runtime semantics — `false && X` is unreachable, drop it — and is correct on those grounds. But this expression carries **extra-semantic information** the lexer reads. Dropping it silently breaks ESM consumers: ```js import { clearRequireCache } from "@tailwindcss/node/require-cache"; // ^^^^^^^^^^^^^^^^^ // SyntaxError: The requested module does not provide an export named 'clearRequireCache' ``` Observed in monitor-oxc CI: https://github.com/oxc-project/monitor-oxc/actions/runs/26427515847/job/77794148798 This is the same *class* of bug as #22475 (string quotes for `require()`) and #22552 (consolidated codegen helpers) — output properties that matter to downstream tooling, lost by minifier passes that only see JS semantics. ## Fix ### Commit 1: bailout in `try_fold_and_or` Restores the guard added in #4878 and dropped during the #8618 refactor. In the constant-fold shortcut at `fold_constants.rs`, when `lval == false && op == And` and the right operand is a `module.exports = { ... }` assignment, return `None` instead of folding `0 && X → 0`. ### Commit 2: close remaining elimination paths Code review surfaced two bypasses around the first guard: 1. **`remove_unused_logical_expr` (separate elimination path)** — when `treeshake.property_write_side_effects: false` (a documented rolldown/vite option), the assignment reports no side effects, the whole `0 && (module.exports = {...})` reports no side effects, and `try_fold_expression_stmt` → `remove_unused_expression` → `remove_unused_logical_expr` drops the entire ExpressionStatement before the fold-constants guard ever runs. Added a matching guard at `remove_unused_expression.rs`. 2. **Compound assignments + paren-wrapping** — the helper matched `module.exports ||= X` (never emitted) and didn't strip `ParenthesizedExpression` uniformly. Tightened to require `AssignmentOperator::Assign` and apply `get_inner_expression()` at the top. ## Helper scope `is_cjs_module_exports_hint` matches **exactly esbuild's emission shape**: an `=` assignment with LHS `module.exports`. Verified against the source — esbuild never emits `exports.X = v`, `module.exports.X = v`, `||` form, or compound operators as hints. Not handled (intentional): - `0 && (exports.X = v)` — esbuild doesn't emit it. - `0 && (module.exports.X.Y = v)` — esbuild doesn't emit it. - Locally-shadowed `module`/`exports` — the lexical check over-preserves harmlessly. Threading `ctx.is_global_reference` through would expand the helper's surface for no observed bug. ## Tests `test_preserve_cjs_module_lexer_hint` in `tests/peephole/fold_constants.rs` covers the positive shape + negative cases (compound assignments fold; non-export RHS folds). Smoke-tested against `@tailwindcss/node/dist/require-cache.js`: hint line survives DCE. Stacks on #22722.
d46a46a to
40ff611
Compare
…row statement (#22722) ## Summary In `minimize_statements`, the dead-after-jump branch silently drops statements without flagging the peephole fixed-point loop as having changed. That leaves stale reference counts in `Scoping` until the next iteration kicks in for unrelated reasons — and in some cases, no further iteration ever happens, so the downstream cleanup never runs. ### Repro ```js export function f() { var x = {}; throw new Error('boom'); module.exports = x; } ``` Expected: `export function f() { throw new Error('boom'); }`. Actual (before fix): `export function f() { var x = {}; throw new Error('boom'); }` — the dead `module.exports = x` is removed, but the now-orphaned `var x = {}` survives because the unused-declarator pass reads stale scope counts. ### Fix In the dead-stmt-drop branch, set `ctx.state.changed = true` so the next iteration's `LiveUsageCollector` refreshes scope counts and unused-declarator cleanup runs. To avoid an oscillation where dropping a bare `var x;` and immediately re-hoisting an identical `var x;` (via `KeepVar`) looks like progress, gate the flag on `dead_drop_mutates_ast`: drops of `var <id>[, <id>]*;` (no initializers) are byte-identical no-ops and don't flag. Two related cases were also fixed in this commit after code review: 1. **Coalescing** — `throw; var x; var y;` → `throw; var x, y;`. Each individual identity-drop looked byte-identical, but the combined re-emit is a real change. Count identity drops; flag changed when more than one was harvested. 2. **Rehoist-then-remove** — when the rehoisted `var x;` is itself unused and `remove_unused_variable_declaration` drops it, the net effect is removing a declaration. Flag changed in that `None` branch. ### Tests - New `dead_after_throw_drop_triggers_unused_declarator_removal` in `dead_code_elimination.rs`. - All 505 existing minifier tests pass. Stacked under #22729.
007a708 to
4ac0fca
Compare
### 🚀 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]>

Summary
esbuild emits unreachable
0 && (module.exports = { ... })statements on Node platform as parse-time hints forcjs-module-lexer. The actual exports happen through helper calls (__export(...)or similar) that the lexer can't trace, so it scans for the literalmodule.exports = { ... }shape inside this dead-code expression to discover named exports.Exact emission site (esbuild v0.28.0):
internal/linker/linker.go#L5127-L5138. AlwaysLogicalAnd(0, Assign(module.exports, ObjectExpression))— no other shape.oxc's DCE judges the expression by JS-runtime semantics —
false && Xis unreachable, drop it — and is correct on those grounds. But this expression carries extra-semantic information the lexer reads. Dropping it silently breaks ESM consumers:Observed in monitor-oxc CI: https://github.com/oxc-project/monitor-oxc/actions/runs/26427515847/job/77794148798
This is the same class of bug as #22475 (string quotes for
require()) and #22552 (consolidated codegen helpers) — output properties that matter to downstream tooling, lost by minifier passes that only see JS semantics.Fix
Commit 1: bailout in
try_fold_and_orRestores the guard added in #4878 and dropped during the #8618 refactor. In the constant-fold shortcut at
fold_constants.rs, whenlval == false && op == Andand the right operand is amodule.exports = { ... }assignment, returnNoneinstead of folding0 && X → 0.Commit 2: close remaining elimination paths
Code review surfaced two bypasses around the first guard:
remove_unused_logical_expr(separate elimination path) — whentreeshake.property_write_side_effects: false(a documented rolldown/vite option), the assignment reports no side effects, the whole0 && (module.exports = {...})reports no side effects, andtry_fold_expression_stmt→remove_unused_expression→remove_unused_logical_exprdrops the entire ExpressionStatement before the fold-constants guard ever runs. Added a matching guard atremove_unused_expression.rs.Compound assignments + paren-wrapping — the helper matched
module.exports ||= X(never emitted) and didn't stripParenthesizedExpressionuniformly. Tightened to requireAssignmentOperator::Assignand applyget_inner_expression()at the top.Helper scope
is_cjs_module_exports_hintmatches exactly esbuild's emission shape: an=assignment with LHSmodule.exports. Verified against the source — esbuild never emitsexports.X = v,module.exports.X = v,||form, or compound operators as hints.Not handled (intentional):
0 && (exports.X = v)— esbuild doesn't emit it.0 && (module.exports.X.Y = v)— esbuild doesn't emit it.module/exports— the lexical check over-preserves harmlessly. Threadingctx.is_global_referencethrough would expand the helper's surface for no observed bug.Tests
test_preserve_cjs_module_lexer_hintintests/peephole/fold_constants.rscovers the positive shape + negative cases (compound assignments fold; non-export RHS folds).Smoke-tested against
@tailwindcss/node/dist/require-cache.js: hint line survives DCE.Stacks on #22722.