fix(minifier): mark peephole loop changed when dropping dead-after-throw statement#22722
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
fc8c193 to
cfd6da1
Compare
Monitor Oxc
|
cfd6da1 to
f773b40
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. |
f27adf6 to
d46a46a
Compare
Merge activity
|
…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.
d46a46a to
40ff611
Compare
…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.
… to helpers After rebasing onto origin/main, three new `state.changed = true` writes added by upstream #22722 (dead-after-throw drop) survived the auto-merge into our migrated `minimize_statements.rs`. The lockdown gate at `tools/check_state_changed.sh` flags them. Convert each to its appropriate per-helper equivalent: - Dead-stmt-drop site: `ctx.drop_statement(&stmt)` — walks the dropped subtree to record dead references in PassDirty, satisfies the Pattern E contract this site was named after in the spec. - Coalesced identity drops: `ctx.notice_change()` — the underlying mutations were already accounted for by `stmts.push` and inside `remove_unused_variable_declaration`; this is a pure 'flag the loop' intent. - Re-hoist-then-remove None branch: `ctx.notice_change()` — same reasoning. 506 minifier tests pass (504 baseline + 2 new upstream tests: dead_after_throw_drop_triggers_unused_declarator_removal and test_preserve_cjs_module_lexer_hint).
### 🚀 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]>
… to helpers After rebasing onto origin/main, three new `state.changed = true` writes added by upstream #22722 (dead-after-throw drop) survived the auto-merge into our migrated `minimize_statements.rs`. The lockdown gate at `tools/check_state_changed.sh` flags them. Convert each to its appropriate per-helper equivalent: - Dead-stmt-drop site: `ctx.drop_statement(&stmt)` — walks the dropped subtree to record dead references in PassDirty, satisfies the Pattern E contract this site was named after in the spec. - Coalesced identity drops: `ctx.notice_change()` — the underlying mutations were already accounted for by `stmts.push` and inside `remove_unused_variable_declaration`; this is a pure 'flag the loop' intent. - Re-hoist-then-remove None branch: `ctx.notice_change()` — same reasoning. 506 minifier tests pass (504 baseline + 2 new upstream tests: dead_after_throw_drop_triggers_unused_declarator_removal and test_preserve_cjs_module_lexer_hint).

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 inScopinguntil the next iteration kicks in for unrelated reasons — and in some cases, no further iteration ever happens, so the downstream cleanup never runs.Repro
Expected:
export function f() { throw new Error('boom'); }.Actual (before fix):
export function f() { var x = {}; throw new Error('boom'); }— the deadmodule.exports = xis removed, but the now-orphanedvar x = {}survives because the unused-declarator pass reads stale scope counts.Fix
In the dead-stmt-drop branch, set
ctx.state.changed = trueso the next iteration'sLiveUsageCollectorrefreshes scope counts and unused-declarator cleanup runs.To avoid an oscillation where dropping a bare
var x;and immediately re-hoisting an identicalvar x;(viaKeepVar) looks like progress, gate the flag ondead_drop_mutates_ast: drops ofvar <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:
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.var x;is itself unused andremove_unused_variable_declarationdrops it, the net effect is removing a declaration. Flag changed in thatNonebranch.Tests
dead_after_throw_drop_triggers_unused_declarator_removalindead_code_elimination.rs.Stacked under #22729.