Skip to content

fix(minifier): mark peephole loop changed when dropping dead-after-throw statement#22722

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/minifier-mark-changed-on-dead-stmt-drop
May 26, 2026
Merged

fix(minifier): mark peephole loop changed when dropping dead-after-throw statement#22722
graphite-app[bot] merged 1 commit into
mainfrom
fix/minifier-mark-changed-on-dead-stmt-drop

Conversation

@Dunqing

@Dunqing Dunqing commented May 25, 2026

Copy link
Copy Markdown
Member

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

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. Coalescingthrow; 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.

@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
⏩ 63 skipped benchmarks1


Comparing fix/minifier-mark-changed-on-dead-stmt-drop (d46a46a) with main (5774052)2

Open in CodSpeed

Footnotes

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

  2. No successful run was found on main (ebe9373) during the generation of this report, so 5774052 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Dunqing Dunqing force-pushed the fix/minifier-mark-changed-on-dead-stmt-drop branch 2 times, most recently from fc8c193 to cfd6da1 Compare May 25, 2026 15:08
@Dunqing Dunqing added the run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it label May 26, 2026
@oxc-guard

oxc-guard Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

@Dunqing Dunqing force-pushed the fix/minifier-mark-changed-on-dead-stmt-drop branch from cfd6da1 to f773b40 Compare May 26, 2026 01:37
@Dunqing Dunqing added run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it and removed run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it labels May 26, 2026

Dunqing commented May 26, 2026

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of 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.

@Dunqing Dunqing force-pushed the fix/minifier-mark-changed-on-dead-stmt-drop branch 2 times, most recently from f27adf6 to d46a46a Compare May 26, 2026 04:53
@graphite-app graphite-app Bot added the 0-merge Merge with Graphite Merge Queue label May 26, 2026
@Dunqing Dunqing marked this pull request as ready for review May 26, 2026 05:30
@graphite-app

graphite-app Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

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.
@graphite-app graphite-app Bot force-pushed the fix/minifier-mark-changed-on-dead-stmt-drop branch from d46a46a to 40ff611 Compare May 26, 2026 05:31
graphite-app Bot pushed a commit that referenced this pull request May 26, 2026
…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.
@graphite-app graphite-app Bot merged commit 40ff611 into main May 26, 2026
29 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 26, 2026
@graphite-app graphite-app Bot deleted the fix/minifier-mark-changed-on-dead-stmt-drop branch May 26, 2026 05:35
Dunqing added a commit that referenced this pull request May 26, 2026
… 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).
Dunqing added a commit that referenced this pull request May 26, 2026
### 🚀 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]>
Dunqing added a commit that referenced this pull request Jun 9, 2026
… 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant