Skip to content

fix(code-splitting): deconflict force-included runtime helper statements#10336

Merged
graphite-app[bot] merged 1 commit into
mainfrom
codex/deconflict-forced-runtime-stmts
Jul 20, 2026
Merged

fix(code-splitting): deconflict force-included runtime helper statements#10336
graphite-app[bot] merged 1 commit into
mainfrom
codex/deconflict-forced-runtime-stmts

Conversation

@hyfdev

@hyfdev hyfdev commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

Chunk deconfliction filtered each module's statements on raw stmt_info_included, so a runtime helper statement that tree-shaking excluded but strict order lowering force-includes (e.g. __esmMin) was rendered and symbol-assigned yet never registered with the renamer. When a user module co-hosted with the runtime declares a top-level binding of the same name, the two share the chunk's root scope: the user's initializer overwrites the runtime helper and the next wrapper's __esmMin(...) call throws. This ORs the order overlay into deconfliction's inclusion test, matching the two other consumers.

Verified mechanism

  • compute_cross_chunk_links.rs:438 and module_finalizers/mod.rs:1623 both OR order_wrap_state.forces_runtime_stmt(...) into their inclusion test; deconflict_chunk_symbols.rs:137 filtered on meta.stmt_info_included.has_bit(...) alone.
  • A single-chunk strict build (codeSplitting: false) places the runtime next to user code (order_wrapping.rs:451-457). Pure ESM demands no helper at link time, so tree-shaking drops __esmMin and strict order lowering force-includes it — the exact statement the deconflict filter dropped.

Fix

deconflict_chunk_symbols already receives order_wrap_state and link_output, so the change is a one-line mirror of the existing two sites: has_bit(idx) || forces_runtime_stmt(&link_output.runtime, module.idx, stmt_info) (forces_runtime_stmt self-guards module_idx == runtime.id(), so it is inert for every non-runtime module).

Item 7 (maintainability). I kept the inline || rather than extracting a shared "is this statement rendered?" accessor: forces_runtime_stmt is already the shared half, the other two sites use this exact inline form, and a shared method would have to take meta + stmt_idx + stmt_info (coupling OrderWrapState to LinkingMetadata) and rewrite two already-correct sites — a larger diff than the reviewer's "keep the diff small" guidance wants for a fix. The three sites now read identically.

Behavior / snapshot impact

  • Full suite: zero existing snapshots change. No current fixture co-hosts a user binding named after a force-included runtime helper, so the added disjunct fires nowhere else.
  • One new fixture function/experimental/strict_execution_order/deconflict_forced_runtime_helper (strict, codeSplitting: false): helper.js declares a top-level __esmMin (hoisted to a root-scope var __esmMin by its order wrapper) co-hosted with the runtime; late.js is ordered after it.
    • Broken output verified on the unfixed base: two var __esmMin at root — the runtime helper and helper.js's binding. helper's init reassigns the shared binding to the user string, and init_late = __esmMin(...) then throws TypeError: __esmMin is not a function (the fixture fails to execute).
    • Fixed: deconflict registers the runtime statement, so the renamer renames the runtime helper to __esmMin$1; every wrapper calls __esmMin$1(...) and the user's __esmMin keeps its name. Snapshot pins the rename; _test.mjs asserts the executed result { helper: 'H:USERVAL', late: 'LATE:z' }.

Note on wrap-all vs on-demand, and the co-hosting mechanism. The reproduction is a wrap-all strict build (strictExecutionOrder without onDemandWrapping) because it deterministically co-hosts the runtime with user code in one chunk. The collision needs the runtime co-hosted with a user module at deconflict time. try_merge_runtime_chunk can co-host the standalone runtime into a user consumer chunk even in multi-chunk builds, but under strict order wrapping ensure_runtime_module_for_order_wraps (order_wrapping.rs:395-457) relocates a co-hosted runtime (modules.len() > 1) back to a standalone chunk unless code splitting is disabled — so at deconflict time the runtime sits next to user code only in single-chunk (codeSplitting: false) builds. (This stack's own multi-chunk strict on-demand fixtures, e.g. facade_gate_emergent_edge, confirm it — they emit a standalone rolldown-runtime.js chunk.) In a single chunk on-demand finds no cross-chunk execution-order hazard, so it wraps nothing and demands no __esmMin; wrap-all is the frame that both co-hosts the runtime and force-includes the helper. The fix itself is mode-independent: forces_runtime_stmt keys on required_runtime_helpers(), covering on-demand and wrap-all and both the __esmMin and __esm (profiler_names) variants.

Validation

  • cargo test -p rolldown — 1872 passed (+1 new fixture, executed under node); only the 5 known environmental failures (cjs_module_lexer_compat ×3 + npm_packages/util_deprecate need pnpm install; test262_module_code needs the submodule). No existing snapshot changed (the local symbols_ns2 drift is pre-existing node_modules-resolution noise, reproducible with this change reverted, and is not committed).
  • cargo fmt --all --check — clean; cargo clippy -p rolldown --all-targets -- -D warnings — clean.

Follow-up to #10104; addresses item 2 (and the item-7 maintainability note) of #10104 (comment)

@hyfdev
hyfdev force-pushed the codex/facade-gate-post-lowering-edges branch from 0688d2e to 1f73c9f Compare July 17, 2026 07:59
@hyfdev
hyfdev force-pushed the codex/deconflict-forced-runtime-stmts branch from 2bbe119 to 9734a3f Compare July 17, 2026 08:00
@hyfdev
hyfdev force-pushed the codex/facade-gate-post-lowering-edges branch from 1f73c9f to 4206c33 Compare July 17, 2026 08:18
@hyfdev
hyfdev force-pushed the codex/deconflict-forced-runtime-stmts branch from 9734a3f to d57826c Compare July 17, 2026 08:18
@hyfdev
hyfdev force-pushed the codex/facade-gate-post-lowering-edges branch from 4206c33 to 0f463fb Compare July 18, 2026 05:35
@hyfdev
hyfdev force-pushed the codex/deconflict-forced-runtime-stmts branch from d57826c to 2ea8341 Compare July 18, 2026 05:35
@hyfdev
hyfdev force-pushed the codex/facade-gate-post-lowering-edges branch from 0f463fb to ea9b969 Compare July 18, 2026 05:48
@hyfdev
hyfdev force-pushed the codex/deconflict-forced-runtime-stmts branch 2 times, most recently from d62f740 to ba3ba38 Compare July 20, 2026 12:37
@hyfdev
hyfdev changed the base branch from codex/facade-gate-post-lowering-edges to main July 20, 2026 12:37
@hyfdev
hyfdev marked this pull request as ready for review July 20, 2026 13:01
@hyfdev
hyfdev requested a review from IWANABETHATGUY as a code owner July 20, 2026 13:01
Copilot AI review requested due to automatic review settings July 20, 2026 13:01
@hyfdev
hyfdev requested a review from shulaoda as a code owner July 20, 2026 13:01

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing codex/deconflict-forced-runtime-stmts (ba3ba38) with main (29534f1)

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.

hyfdev commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

Merge activity

  • Jul 20, 1:45 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 20, 1:45 PM UTC: hyfdev added this pull request to the Graphite merge queue.
  • Jul 20, 1:51 PM UTC: Merged by the Graphite merge queue.

…nts (#10336)

## Summary

Chunk deconfliction filtered each module's statements on raw `stmt_info_included`, so a runtime helper statement that tree-shaking excluded but strict order lowering force-includes (e.g. `__esmMin`) was rendered and symbol-assigned yet never registered with the renamer. When a user module co-hosted with the runtime declares a top-level binding of the same name, the two share the chunk's root scope: the user's initializer overwrites the runtime helper and the next wrapper's `__esmMin(...)` call throws. This ORs the order overlay into deconfliction's inclusion test, matching the two other consumers.

## Verified mechanism

- `compute_cross_chunk_links.rs:438` and `module_finalizers/mod.rs:1623` both OR `order_wrap_state.forces_runtime_stmt(...)` into their inclusion test; `deconflict_chunk_symbols.rs:137` filtered on `meta.stmt_info_included.has_bit(...)` alone.
- A single-chunk strict build (`codeSplitting: false`) places the runtime next to user code (`order_wrapping.rs:451-457`). Pure ESM demands no helper at link time, so tree-shaking drops `__esmMin` and strict order lowering force-includes it — the exact statement the deconflict filter dropped.

## Fix

`deconflict_chunk_symbols` already receives `order_wrap_state` and `link_output`, so the change is a one-line mirror of the existing two sites: `has_bit(idx) || forces_runtime_stmt(&link_output.runtime, module.idx, stmt_info)` (`forces_runtime_stmt` self-guards `module_idx == runtime.id()`, so it is inert for every non-runtime module).

**Item 7 (maintainability).** I kept the inline `||` rather than extracting a shared "is this statement rendered?" accessor: `forces_runtime_stmt` is already the shared half, the other two sites use this exact inline form, and a shared method would have to take `meta` + `stmt_idx` + `stmt_info` (coupling `OrderWrapState` to `LinkingMetadata`) and rewrite two already-correct sites — a larger diff than the reviewer's "keep the diff small" guidance wants for a fix. The three sites now read identically.

## Behavior / snapshot impact

- **Full suite: zero existing snapshots change.** No current fixture co-hosts a user binding named after a force-included runtime helper, so the added disjunct fires nowhere else.
- **One new fixture** `function/experimental/strict_execution_order/deconflict_forced_runtime_helper` (strict, `codeSplitting: false`): `helper.js` declares a top-level `__esmMin` (hoisted to a root-scope `var __esmMin` by its order wrapper) co-hosted with the runtime; `late.js` is ordered after it.
  - **Broken output verified on the unfixed base:** two `var __esmMin` at root — the runtime helper and helper.js's binding. `helper`'s init reassigns the shared binding to the user string, and `init_late = __esmMin(...)` then throws `TypeError: __esmMin is not a function` (the fixture fails to execute).
  - **Fixed:** deconflict registers the runtime statement, so the renamer renames the runtime helper to `__esmMin$1`; every wrapper calls `__esmMin$1(...)` and the user's `__esmMin` keeps its name. Snapshot pins the rename; `_test.mjs` asserts the executed result `{ helper: 'H:USERVAL', late: 'LATE:z' }`.

**Note on wrap-all vs on-demand, and the co-hosting mechanism.** The reproduction is a wrap-all strict build (`strictExecutionOrder` without `onDemandWrapping`) because it deterministically co-hosts the runtime with user code in one chunk. The collision needs the runtime co-hosted with a user module *at deconflict time*. `try_merge_runtime_chunk` can co-host the standalone runtime into a user consumer chunk even in multi-chunk builds, but under strict order wrapping `ensure_runtime_module_for_order_wraps` (`order_wrapping.rs:395-457`) relocates a co-hosted runtime (`modules.len() > 1`) back to a standalone chunk unless code splitting is disabled — so at deconflict time the runtime sits next to user code only in single-chunk (`codeSplitting: false`) builds. (This stack's own multi-chunk strict on-demand fixtures, e.g. `facade_gate_emergent_edge`, confirm it — they emit a standalone `rolldown-runtime.js` chunk.) In a single chunk on-demand finds no cross-chunk execution-order hazard, so it wraps nothing and demands no `__esmMin`; wrap-all is the frame that both co-hosts the runtime and force-includes the helper. The fix itself is mode-independent: `forces_runtime_stmt` keys on `required_runtime_helpers()`, covering on-demand and wrap-all and both the `__esmMin` and `__esm` (profiler_names) variants.

## Validation

- `cargo test -p rolldown` — 1872 passed (+1 new fixture, executed under node); only the 5 known environmental failures (`cjs_module_lexer_compat` ×3 + `npm_packages/util_deprecate` need `pnpm install`; `test262_module_code` needs the submodule). No existing snapshot changed (the local `symbols_ns2` drift is pre-existing `node_modules`-resolution noise, reproducible with this change reverted, and is not committed).
- `cargo fmt --all --check` — clean; `cargo clippy -p rolldown --all-targets -- -D warnings` — clean.

---

Follow-up to #10104; addresses item 2 (and the item-7 maintainability note) of #10104 (comment)
@graphite-app
graphite-app Bot force-pushed the codex/deconflict-forced-runtime-stmts branch from ba3ba38 to ae2ec2e Compare July 20, 2026 13:46
@netlify

netlify Bot commented Jul 20, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit ae2ec2e
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a5e26a9c4fe3600082ba3f1

@graphite-app
graphite-app Bot merged commit ae2ec2e into main Jul 20, 2026
34 checks passed
@graphite-app
graphite-app Bot deleted the codex/deconflict-forced-runtime-stmts branch July 20, 2026 13:51
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.

3 participants