Skip to content

fix(finalizer): skip init_*() for tree-shaken wrapped ESM owners#9669

Merged
graphite-app[bot] merged 1 commit into
mainfrom
06-08-fix_9651
Jun 8, 2026
Merged

fix(finalizer): skip init_*() for tree-shaken wrapped ESM owners#9669
graphite-app[bot] merged 1 commit into
mainfrom
06-08-fix_9651

Conversation

@IWANABETHATGUY

@IWANABETHATGUY IWANABETHATGUY commented Jun 8, 2026

Copy link
Copy Markdown
Member

Fixes #9651.

Problem

Under cjs + inlineDynamicImports + tree-shaking, the module finalizer panicked with:

"init_external" is not in any chunk, which is unexpected

This happened when a side-effect-free (sideEffects: false) barrel forwards a binding from a wrapped ESM module that is reached both statically (import * as z from './zod') and through a dynamic import() chain. The dynamic import forces the barrel modules to be wrapped as lazy-init ESM, but tree-shaking then drops external.js because its namespace is never actually read (only z.locales is used, and that resolves straight through to locales.js).

When lowering the static import * as z from the non-wrapped barrel, collect_wrapped_esm_init_modules_for_import_record walked every resolved export of the barrel and emitted an init_*() call for each wrapped owner — including external.js, which tree-shaking had eliminated. Its init_* wrapper statement was never included, so the wrapper symbol had no chunk assignment and the finalizer panicked.

Fix

add_wrapped_esm_init_module_for_symbol now only emits init_*() for wrapped owners that tree-shaking actually kept, by guarding on meta.is_included. This mirrors the existing is_included guard in the sibling generate_transitive_esm_init. A non-included module produces no init_* function in the output, so skipping its init call is required for correctness — there is no case where calling it would be valid.

Test

Adds a runnable regression fixture under crates/rolldown/tests/rolldown/issues/9651 that reproduces the panic without the fix and bundles and executes successfully with it.

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to 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.

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review June 8, 2026 15:01
@IWANABETHATGUY IWANABETHATGUY changed the title fix: 9651 fix(finalizer): skip init_*() for tree-shaken wrapped ESM owners Jun 8, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing 06-08-fix_9651 (45e3623) with main (5cfff49)2

Open in CodSpeed

Footnotes

  1. 10 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 (45e3623) during the generation of this report, so 5cfff49 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@netlify

netlify Bot commented Jun 8, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit 2f02e67
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a26d5bb0b7aa40008db6c90
😎 Deploy Preview https://deploy-preview-9669--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Jun 8, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit 45e3623
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a26f003b4c84f0008efc7d4
😎 Deploy Preview https://deploy-preview-9669--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@graphite-app

graphite-app Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Merge activity

Fixes #9651.

## Problem

Under `cjs` + `inlineDynamicImports` + tree-shaking, the module finalizer panicked with:

```
"init_external" is not in any chunk, which is unexpected
```

This happened when a side-effect-free (`sideEffects: false`) barrel forwards a binding from a wrapped ESM module that is reached **both** statically (`import * as z from './zod'`) **and** through a dynamic `import()` chain. The dynamic import forces the barrel modules to be wrapped as lazy-init ESM, but tree-shaking then drops `external.js` because its namespace is never actually read (only `z.locales` is used, and that resolves straight through to `locales.js`).

When lowering the static `import * as z` from the non-wrapped barrel, `collect_wrapped_esm_init_modules_for_import_record` walked **every** resolved export of the barrel and emitted an `init_*()` call for each wrapped owner — including `external.js`, which tree-shaking had eliminated. Its `init_*` wrapper statement was never included, so the wrapper symbol had no chunk assignment and the finalizer panicked.

## Fix

`add_wrapped_esm_init_module_for_symbol` now only emits `init_*()` for wrapped owners that tree-shaking actually kept, by guarding on `meta.is_included`. This mirrors the existing `is_included` guard in the sibling `generate_transitive_esm_init`. A non-included module produces no `init_*` function in the output, so skipping its init call is required for correctness — there is no case where calling it would be valid.

## Test

Adds a runnable regression fixture under `crates/rolldown/tests/rolldown/issues/9651` that reproduces the panic without the fix and bundles **and executes** successfully with it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a panic in the module finalizer when scope-hoisting lowers imports that forward symbols from wrapped ESM modules which have been tree-shaken away, ensuring init_*() calls are only emitted for wrapped owners that are actually included.

Changes:

  • Guard add_wrapped_esm_init_module_for_symbol with meta.is_included to avoid emitting init_*() for eliminated wrapped ESM owners.
  • Add a new regression fixture for #9651 that reproduces the panic scenario under cjs + inlined dynamic imports + tree-shaking.
  • Snapshot the generated bundle output for the fixture.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/rolldown/src/module_finalizers/mod.rs Adds an is_included guard to prevent emitting invalid init_*() calls for tree-shaken wrapped ESM owners.
crates/rolldown/tests/rolldown/issues/9651/_config.json Introduces fixture configuration for the regression reproduction.
crates/rolldown/tests/rolldown/issues/9651/main.js Entry script that triggers the static + dynamic reachability pattern causing the prior panic.
crates/rolldown/tests/rolldown/issues/9651/artifacts.snap Snapshots the bundled output for the new regression fixture.
crates/rolldown/tests/rolldown/issues/9651/src/dyn/a.js Dynamic-import target module used to force the wrapped/lazy-init scenario.
crates/rolldown/tests/rolldown/issues/9651/src/dyn/b.js Imports the wrapped module as part of the dynamic chain in the reproduction.
crates/rolldown/tests/rolldown/issues/9651/src/zod/package.json Marks the fixture “package” as side-effect-free via sideEffects: false.
crates/rolldown/tests/rolldown/issues/9651/src/zod/index.js Barrel module that forwards exports and a namespace binding.
crates/rolldown/tests/rolldown/issues/9651/src/zod/external.js Wrapped ESM owner that can be tree-shaken away in the repro.
crates/rolldown/tests/rolldown/issues/9651/src/zod/locales.js Minimal leaf module used by the test assertion (en()).

@@ -0,0 +1 @@
import { z } from '../zod/external.js';
Comment on lines +6 to +8
"codeSplitting": false,
"shimMissingExports": true
}
@graphite-app graphite-app Bot merged commit 45e3623 into main Jun 8, 2026
34 checks passed
@graphite-app graphite-app Bot deleted the 06-08-fix_9651 branch June 8, 2026 16:44
Comment on lines +17 to +18
__esmMin((() => {}))();
Promise.resolve().then(() => /* @__PURE__ */ Object.freeze({ __proto__: null }));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit on the emitted output: this fixture now produces a no-op __esmMin((() => {}))() (an empty wrapped-ESM init being called) plus an empty inlined dynamic-import Promise.resolve().then(() => Object.freeze({ __proto__: null })).

The fix is correct and the snapshot is valid/runnable — just flagging that both lines are dead weight. Once a wrapped ESM module tree-shakes down to an empty body, its init_* call (and the empty inlined import()) could be dropped entirely. Not this PR's job; might be worth a follow-up cleanup so these degenerate cases emit nothing rather than a no-op call.

@hyf0 hyf0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the finalizer change in depth (traced the panic site, code_splitting chunk assignment, and canonical_ref_resolving_namespace semantics). The is_included guard's premise is airtight: a non-included module never gets its wrapper symbol assigned to a chunk, so emitting its init_*() call would always panic, and the namespace-resolution path guarantees skipping it can never miss a deeper needed init. Mirrors the existing guard in generate_transitive_esm_init, only ever removes init calls (no regression risk), and CI is green with no snapshot changes.

LGTM. One non-blocking follow-up nit left inline re: the no-op init_*() / empty inlined import() in the snapshot.

graphite-app Bot pushed a commit that referenced this pull request Jun 9, 2026
Follow-up to #9669 addressing the Copilot review feedback on the regression fixture: #9669 (review)

## What

The `#9651` regression fixture had `src/dyn/b.js` import a named `z` from `../zod/external.js`, but `external.js` never exports `z`. That import was only valid because the fixture enabled `shimMissingExports`, which is unrelated to the bug under test. The binding is never used — only the `b.js → external.js` import **edge** matters (it pulls the wrapped `external.js` into the dynamic-import chain).

This PR:
- switches `b.js` to a bare side-effect import (`import '../zod/external.js';`), so the fixture is semantically valid without missing-export shimming, and
- drops the now-unnecessary `"shimMissingExports": true` from `_config.json`.

## Why it's safe

- The bundled output is **unchanged** — `artifacts.snap` does not move.
- The fixture still reproduces the original panic on the pre-fix commit: I checked out `5cfff49` (the commit right before #9669 landed), dropped in this updated fixture, and it still panics with `"init_external" is not in any chunk`.

Test-only change; keeps the regression focused on #9651.
IWANABETHATGUY added a commit that referenced this pull request Jun 9, 2026
Address the review feedback on #9669: `src/dyn/b.js` imported a named
`z` that `external.js` never exports, which only worked because the
fixture enabled `shimMissingExports`. The binding is unused — only the
import edge matters for the repro — so switch it to a bare side-effect
import and drop the now-unnecessary `shimMissingExports` option. This
keeps the regression focused on #9651 without relying on missing-export
shimming. The bundled output (snapshot) is unchanged, and the fixture
still reproduces the original panic on the pre-fix commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
graphite-app Bot pushed a commit that referenced this pull request Jun 9, 2026
## Summary

Follow-up to the review nit on #9669: when a wrapped ESM module tree-shakes down to an
empty body, rolldown still emitted a no-op call to its `__esm`/`__esmMin` wrapper, e.g.

```js
function en() { return "en"; }
__esmMin((() => {}))();   // ← dead weight: empty closure, calling it does nothing
```

This PR marks such no-op init calls `@__PURE__` so the default `minify: 'dce-only'`
pass drops them (and, once the wrapper is unreferenced, the wrapper declaration and the
`__esm`/`__esmMin` runtime helper too).

**Before**
```js
var __esmMin = (fn, res) => () => (fn && (res = fn(fn = 0)), res);
function en() { return "en"; }
__esmMin((() => {}))();
```
**After**
```js
function en() { return "en"; }
```

It also collapses the `require(esm)` sequence form `(init_x(), __toCommonJS(x_exports))`
→ `__toCommonJS(x_exports)` via the same mechanism.

## Approach

1. A small generate-stage pre-pass, `compute_init_is_noop`, flags wrapped (`WrapKind::Esm`)
   modules whose `__esm` closure body is provably empty — every top-level statement is a
   hoisted function declaration or a source-less export clause, so nothing lands inside the
   closure. The classification runs in parallel (`par_iter_enumerated`).
2. The finalizer sets `pure = true` on the init `CallExpression` at the three emission sites
   when the callee module is flagged; `dce-only` does the rest.

The classifier is deliberately conservative (a non-qualifying statement only ever *keeps* a
redundant init call — it never drops a needed one), and a `debug_assert!` in the finalizer
cross-checks every flagged module against the *actual* `stmts_inside_closure`, so any
misclassification fails loudly across the fixture suite. Guards exclude shimmed missing
exports (their `x = void 0` lands in the closure) and concatenated wrapper groups.

## Out of scope (kept on purpose)

The empty inlined dynamic import `Promise.resolve().then(() => Object.freeze({ __proto__: null }))`
is **not** removed — Rollup keeps it for `import('./empty.js')` of an empty module, so it's
Rollup-equivalent and stays.

## Why the detection lives in its own pass (and not elsewhere)

The flag is consumed **cross-module** — an importer marks `init_importee()` pure based on the
*importee's* status — which constrains where it can be computed.

### Not in the link stage (`cross_module_optimization.rs`)

That pass runs **before** the two inputs the decision needs even exist:

- **`is_included`** is produced by tree-shaking (`include_statements`), which runs *after*
  `cross_module_optimization`. A module that's tree-shaken away has no init to reason about.
- **`concatenated_wrapped_module_kind`** is assigned during the generate stage's
  chunk/concatenation analysis. We must restrict the optimization to standalone wrappers
  (`ConcatenateWrappedModuleKind::None`): in a concatenated group the shared `init_*` runs the
  *whole group's* closure, so one module's empty body wouldn't prove the call is a no-op. That
  isn't knowable before chunks are formed.

Folding it here would force the decision with incomplete information (pre-tree-shaking,
pre-chunking).

### Not in the finalizer (`module_finalizers/`)

`finalize_modules` runs the per-module finalizers in parallel (`par_iter_mut_enumerated`):

1. **Immutable borrow** — the finalizer holds `&LinkingMetadata` / `&LinkingMetadataVec`
   (`ScopeHoistingFinalizerContext`), read-only, so it *cannot write* the flag mid-pass.
2. **Determinism** — even if it could, an importer finalizing concurrently with its importee
   could read a not-yet-computed (default `false`) value → non-deterministic `@__PURE__`
   placement → flaky snapshots.

So the flag must be settled as a barrier *before* finalization begins.

### The fit

The pre-pass sits in the generate stage **after** tree-shaking and chunk generation (so
`is_included` and `concatenated_wrapped_module_kind` are known) but **immediately before** the
parallel `finalize_modules` (so every importer reads a settled, deterministic value).

## Testing

- Updated 5 fixture snapshots (incl. the #9651 regression); each remains runnable.
- Full `rolldown` + `esbuild` integration suites pass; the `init_is_noop` debug-assert holds
  across all ~1.6k fixtures.
- `just lint-rust` clean (clippy `--deny warnings`, fmt, `cargo check --all-features`).
@shulaoda shulaoda mentioned this pull request Jun 11, 2026
@rolldown-guard rolldown-guard Bot mentioned this pull request Jun 11, 2026
shulaoda pushed a commit that referenced this pull request Jun 11, 2026
## [1.1.1] - 2026-06-11

### 🚀 Features

- ecmascript_utils: introduce AstFactory for AST construction (#9682) by @hyf0
- drop no-op init calls for empty wrapped-ESM modules (#9678) by @IWANABETHATGUY

### 🐛 Bug Fixes

- resolver: honor package.json#type for .jsx/.tsx extensions (#9690) (#9699) by @IWANABETHATGUY
- hmr: report the full-reload reason for the invalidate-loop case (#9708) by @hyf0
- explicit `moduleSideEffects` from a hook must take priority over the `package.json#sideEffects` (#9688) by @sapphi-red
- keep the `rolldown-runtime` name for the standalone runtime chunk (#9685) by @shulaoda
- lazy-barrel: request all exports for entry barrels on first encounter (#9672) by @shulaoda
- finalizer: skip init_*() for tree-shaken wrapped ESM owners (#9669) by @IWANABETHATGUY
- order `chunk.imports` by execution order (#9654) by @chuganzy
- vite-resolve: preserve slash separators for Sass partial exports (#9546) by @cjc0013
- cross-chunk CJS wrapper shadowed by author-local binding (#9648) by @IWANABETHATGUY

### 🚜 Refactor

- precompute wrapped-ESM init metadata in generate stage (#9712) by @IWANABETHATGUY
- ecmascript_utils: fold construction ext traits onto AstFactory and delete AstSnippet (#9702) by @hyf0
- finalizer: finish ScopeHoistingFinalizer migration to AstFactory (#9701) by @hyf0
- finalizer: migrate module_finalizers/mod.rs to AstFactory (#9700) by @hyf0
- hmr: migrate hmr finalizer to AstFactory (#9695) by @hyf0
- plugin: migrate vite_build_import_analysis to AstFactory (#9693) by @hyf0
- scanner: migrate tweak_ast_for_scanning to AstFactory (#9683) by @hyf0
- always split runtime module first (#9419) by @IWANABETHATGUY

### 📚 Documentation

- tsconfig: correct auto-discovery resolution to match TypeScript (#9714) by @shulaoda
- design: plan to unify all internal AST construction (#9673) by @hyf0
- tsconfig: align reference resolution docs with TypeScript behavior (#9641) by @shulaoda

### ⚡ Performance

- avoid per-module join Strings in scope-hoisting concatenation (#9645) by @Boshen
- avoid intermediate Strings in the ESM export clause (#9644) by @Boshen
- reuse a scratch buffer for facade namespace names in the scanner (#9642) by @Boshen
- reuse the import-matching tracker stack across named imports (#9643) by @Boshen
- avoid cloning the per-chunk export-items map in render_chunk_exports (#9639) by @Boshen
- avoid CompactStr allocation in sorted-exports membership check (#9640) by @Boshen

### 🧪 Testing

- add more `moduleSideEffects` precedence tests (#9689) by @sapphi-red
- 9651: drop shimMissingExports from regression fixture (#9674) by @IWANABETHATGUY

### ⚙️ Miscellaneous Tasks

- update react repo links (#9711) by @iiio2
- remove outdated pnpm configurations (#9666) by @btea
- deps: update github actions to v2.81.5 (#9665) by @renovate[bot]
- testing: migrate test harness off deprecated inlineDynamicImports (#9710) by @IWANABETHATGUY
- hmr: delete commented-out dead code left from old register-module codegen (#9707) by @hyf0
- deps: update napi-rs toolchain (#9706) by @shulaoda
- deps: update rollup submodule for tests to v4.61.1 (#9676) by @rolldown-guard[bot]
- deps: update test262 submodule for tests (#9677) by @rolldown-guard[bot]
- deps: update oxc to 0.135.0 (#9670) by @Boshen
- pass ALGOLIA_APP_ID and ALGOLIA_API_KEY to void deploy (#9667) by @Boshen
- deps: update github actions (#9662) by @renovate[bot]
- deps: update rust crates (#9663) by @renovate[bot]
- deps: update rolldown-plugin-dts to v0.25.2 (#9661) by @renovate[bot]
- deps: update typos to v1.47.2 (#9660) by @renovate[bot]
- deps: update docs dependencies (#9657) by @bddjr
- deps: update typos to v1.47.1 (#9655) by @renovate[bot]
- deploy website in its own workflow, only on docs output change (#9650) by @Boshen
- publish to pkg.pr.new add pm and `commentWithDev` option (#9638) by @btea

### ❤️ New Contributors

* @chuganzy made their first contribution in [#9654](#9654)
* @cjc0013 made their first contribution in [#9546](#9546)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Panic]: "init_from_json_schema" is not in any chunk, which is unexpected

4 participants