Skip to content

fix: gate sideEffects:false modules' side effects on body demand#10080

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/body-demand-side-effect-inclusion
Jul 2, 2026
Merged

fix: gate sideEffects:false modules' side effects on body demand#10080
graphite-app[bot] merged 1 commit into
mainfrom
fix/body-demand-side-effect-inclusion

Conversation

@IWANABETHATGUY

@IWANABETHATGUY IWANABETHATGUY commented Jul 2, 2026

Copy link
Copy Markdown
Member

What this PR solves

Any wrap trigger — strictExecutionOrder, require() of ESM, or an inlined dynamic import under codeSplitting: false — force-includes a sideEffects: false ESM module through its wrapper ref, retaining binding-reading side-effect statements that plain (unwrapped) tree-shaking provably drops. With experimental.lazyBarrel, those retained statements reference import records the loader correctly never loaded (it defers records by requested exports), so the emitted chunk contains free identifiers and throws ReferenceError at load. The tree-shaking inconsistency between strictExecutionOrder: true and false is the shared root cause of the family:

fixes #9691, fixes #9806, fixes #9961, fixes #9964, fixes #10013, fixes #10048

How

A binding-reading side-effect statement of a user-declared side-effect-free ESM module is now included iff the module's body is demanded: one of its own (non-re-export) exports or its namespace object becomes used. This mirrors the lazy-barrel loader's local/All classification exactly — the loader loads every plain import record precisely in those cases — so a kept statement can never reference an unloaded record, and a deferred record is only ever referenced by dropped statements. The loader itself needs no changes.

Behavior deliberately kept as-is (pinned by existing tests):

  • statements with no module-symbol references (a bare console.log()) and import/re-export statements (they drive wrapper init_* chains and side-effect imports) keep the unconditional sweep — package_json_side_effects_false_keep_* esbuild fixtures;
  • bodies of modules whose own export is demanded ([Bug]: panic in v1.0.0-beta.55 with react-bootstrap and --minify #7597's top-level asserts) or whose namespace is observed (import * as ns, require(esm)) are retained unchanged;
  • user-defined entries, eval modules, and CommonJS modules are exempt.

Dynamic entries are not exempt: a live dynamic entry's body joins through namespace/own-export demand, while a dead pure dynamic import must not resurrect a body its removal already models as an empty namespace (covered by the new tree_shaking/dead_dynamic_entry_side_effect_free fixture).

Alternatives explored

  • Loader-side fix (force-load records referenced by retained statements): rejected — the deferral strategy is correct; the retention is the bug.
  • Blanket statement-level stripping for sideEffects: false modules: rejected — breaks the esbuild-parity keep_* fixtures (namespace/require demand must keep the body).
  • Statement-granular demand edges (keep a statement once any symbol it references is used): rejected — still dropped [Bug]: panic in v1.0.0-beta.55 with react-bootstrap and --minify #7597's asserts.

Notes for reviewers

  • Inclusion changes only for UserDefined(false) ESM modules reached exclusively through re-exports or wrapper refs; every pre-existing snapshot is byte-identical (full integration suite: 1785 passed, 0 failed).
  • Each issues/* fixture executes its output with node; issues/10013 and the dynamic-entry fixture additionally carry a local node_modules/dep that throws on evaluation, so re-emitting the dropped external import fails execution, not just the snapshot.
  • Residual and deferred (benign): wrapped modules whose body was pruned still emit empty init_x shells; a follow-up no-op-init cleanup can drop them.

@netlify

netlify Bot commented Jul 2, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

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

@IWANABETHATGUY IWANABETHATGUY changed the title Fix/body demand side effect inclusion fix: gate sideEffects:false modules' side effects on body demand Jul 2, 2026
@IWANABETHATGUY IWANABETHATGUY force-pushed the fix/body-demand-side-effect-inclusion branch from ff2be16 to 1b22ce3 Compare July 2, 2026 08:52
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review July 2, 2026 08:57
@shulaoda shulaoda requested review from Copilot and removed request for hyf0 and shulaoda July 2, 2026 08:58
@shulaoda shulaoda requested review from hyf0 and shulaoda July 2, 2026 09:02

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 tree-shaking inconsistency where sideEffects: false ESM modules could have side-effectful, binding-reading statements resurrected by wrap triggers (strictExecutionOrder, require() of ESM, inlined dynamic import under codeSplitting: false). The fix gates those statements so they are included only when the module’s body is demanded (own-export or namespace demand), preventing emitted chunks from referencing bindings whose import records were correctly deferred/omitted (notably with experimental.lazyBarrel).

Changes:

  • Introduces “on-demand” inclusion edges for side-effectful statements that reference module-level symbols in UserDefined(false) ESM modules, keyed off own-export / namespace demand.
  • Replays the same inclusion semantics during chunk optimization to keep link-stage and generate-stage behavior aligned.
  • Adds regression fixtures/snapshots covering the affected issue family (#9691, #9806, #9961, #9964, #10013, #10048) plus a dead pure dynamic-entry case.

Reviewed changes

Copilot reviewed 57 out of 61 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs Implements on-demand gating + inclusion edges for sideEffects:false ESM statements
crates/rolldown/src/stages/link_stage/mod.rs Re-exports compute helper for reuse in generate stage
crates/rolldown/src/stages/generate_stage/chunk_optimizer.rs Replays on-demand inclusion semantics during chunk optimization
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/_config.json New regression config for dead pure dynamic import + strictExecutionOrder
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/artifacts.snap Snapshot asserting leaf body stays dropped
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/entry-a.js Fixture entry with pure dynamic import + live re-export usage
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/entry-b.js Fixture entry importing only the re-exported value
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/helper.js Re-export source used by entries
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/leaf.js Side-effect-free leaf with external import + unused own export
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/node_modules/dep/index.js Local external dep that must remain unreachable
crates/rolldown/tests/rolldown/tree_shaking/dead_dynamic_entry_side_effect_free/node_modules/dep/package.json Declares local dep as ESM export
crates/rolldown/tests/rolldown/issues/9964/_config.json Regression config for CJS->ESM require wrap + export* barrel
crates/rolldown/tests/rolldown/issues/9964/artifacts.snap Snapshot asserting unused star-source is pruned under wrap
crates/rolldown/tests/rolldown/issues/9964/entry.cjs CJS entry requiring ESM consumer (wrap trigger)
crates/rolldown/tests/rolldown/issues/9964/lib/account.js sideEffects:false star-source with derived initializers reading imports
crates/rolldown/tests/rolldown/issues/9964/lib/eg.js Defines binding that previously got pruned while still referenced
crates/rolldown/tests/rolldown/issues/9964/lib/index.js Barrel with export * plus unrelated local export
crates/rolldown/tests/rolldown/issues/9964/package.json Marks fixture package as ESM + sideEffects:false
crates/rolldown/tests/rolldown/issues/9964/story.js ESM consumer importing only unrelated util
crates/rolldown/tests/rolldown/issues/9961/_config.json Regression config for strictExecutionOrder + sideEffects:false top-level call
crates/rolldown/tests/rolldown/issues/9961/artifacts.snap Snapshot asserting the problematic top-level call is not emitted
crates/rolldown/tests/rolldown/issues/9961/core/checkGlobals.js Imported function that was previously dropped while call remained
crates/rolldown/tests/rolldown/issues/9961/core/http.js Used export to keep barrel reachable
crates/rolldown/tests/rolldown/issues/9961/core/index.js Barrel calling imported binding at module top level
crates/rolldown/tests/rolldown/issues/9961/core/package.json Declares sideEffects:false for fixture package
crates/rolldown/tests/rolldown/issues/9961/core/setupWorker.js Used export to keep barrel reachable
crates/rolldown/tests/rolldown/issues/9961/main.js App entry using only selected exports from barrel
crates/rolldown/tests/rolldown/issues/9961/package.json Root fixture package.json
crates/rolldown/tests/rolldown/issues/9806/_config.json Regression config for lazyBarrel + derived initializer in sideEffects:false barrel
crates/rolldown/tests/rolldown/issues/9806/artifacts.snap Snapshot asserting bundle executes and unused derived reads are not kept
crates/rolldown/tests/rolldown/issues/9806/main.js Entry importing only unrelated re-export to trigger pruning
crates/rolldown/tests/rolldown/issues/9806/pkg-barrel/foo.cjs CJS submodule used only by derived initializer
crates/rolldown/tests/rolldown/issues/9806/pkg-barrel/index.mjs sideEffects:false barrel with derived property read
crates/rolldown/tests/rolldown/issues/9806/pkg-barrel/other.cjs Unrelated re-export that stays used
crates/rolldown/tests/rolldown/issues/9806/pkg-barrel/package.json Declares sideEffects:false for barrel package
crates/rolldown/tests/rolldown/issues/9691/_config.json Regression config for codeSplitting:false dynamic wrap propagation through export*
crates/rolldown/tests/rolldown/issues/9691/artifacts.snap Snapshot asserting dead star-source is pruned to avoid dangling bindings
crates/rolldown/tests/rolldown/issues/9691/lib/dep-a.js Used re-export path (external builtin import)
crates/rolldown/tests/rolldown/issues/9691/lib/dep-b.js Deferred import path that must not be referenced
crates/rolldown/tests/rolldown/issues/9691/lib/handler-a.js Used star-source providing the requested export
crates/rolldown/tests/rolldown/issues/9691/lib/handler-b.js Dead star-source with top-level binding read (previously kept)
crates/rolldown/tests/rolldown/issues/9691/lib/index.js Barrel with export * from used and unused sources
crates/rolldown/tests/rolldown/issues/9691/lib/package.json Declares sideEffects:false for barrel package
crates/rolldown/tests/rolldown/issues/9691/main.js Entry that triggers wrap via inlined dynamic import
crates/rolldown/tests/rolldown/issues/9691/trigger.js Dynamic import target that pulls barrel local export
crates/rolldown/tests/rolldown/issues/10048/_config.json Regression config for missing helper bindings under strictExecutionOrder
crates/rolldown/tests/rolldown/issues/10048/_test.mjs Runtime test asserting built entry import does not throw
crates/rolldown/tests/rolldown/issues/10048/artifacts.snap Snapshot asserting helper-only module is pruned safely
crates/rolldown/tests/rolldown/issues/10048/data.js Helper-independent passthrough export (themes)
crates/rolldown/tests/rolldown/issues/10048/entry.js Entry using only passthrough export but keeping module reachable
crates/rolldown/tests/rolldown/issues/10048/helpers.js Helper-only module defining __commonJS / __toESM exports
crates/rolldown/tests/rolldown/issues/10048/package.json Declares sideEffects:false for fixture package
crates/rolldown/tests/rolldown/issues/10048/theming.js Module calling helpers at top-level while re-exporting passthrough
crates/rolldown/tests/rolldown/issues/10013/_config.json Regression config for external bindings referenced without import under strictExecutionOrder
crates/rolldown/tests/rolldown/issues/10013/artifacts.snap Snapshot asserting leaf body (and external import) is fully dropped
crates/rolldown/tests/rolldown/issues/10013/entry-a.js Entry importing only re-exported value
crates/rolldown/tests/rolldown/issues/10013/entry-b.js Second entry importing only re-exported value
crates/rolldown/tests/rolldown/issues/10013/helper.js Re-export source used by entries
crates/rolldown/tests/rolldown/issues/10013/leaf.js Leaf with unused own export referencing external bindings
crates/rolldown/tests/rolldown/issues/10013/node_modules/dep/index.js Local external dep that must remain unreachable
crates/rolldown/tests/rolldown/issues/10013/node_modules/dep/package.json Declares local dep as ESM export

@codspeed-hq

codspeed-hq Bot commented Jul 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing fix/body-demand-side-effect-inclusion (9897950) with main (a952b80)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 (b07f953) during the generation of this report, so a952b80 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@hyf0 hyf0 self-assigned this Jul 2, 2026

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.

@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.

I found some issues but I'm not sure if it's correct to fix them in this direction. The treeshking part becomes too complicated, it's very hard to manually reason if the behavior is correct.

However, tests are passed, it's possitive to prevent future regressions after merging them.

hyf0 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Merge activity

  • Jul 2, 10:57 AM 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 2, 10:58 AM 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 2, 10:58 AM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jul 2, 11:03 AM UTC: Merged by the Graphite merge queue.

)

### What this PR solves

Any wrap trigger — `strictExecutionOrder`, `require()` of ESM, or an inlined dynamic import under `codeSplitting: false` — force-includes a `sideEffects: false` ESM module through its wrapper ref, retaining binding-reading side-effect statements that plain (unwrapped) tree-shaking provably drops. With `experimental.lazyBarrel`, those retained statements reference import records the loader correctly never loaded (it defers records by requested exports), so the emitted chunk contains free identifiers and throws `ReferenceError` at load. The tree-shaking inconsistency between `strictExecutionOrder: true` and `false` is the shared root cause of the family:

fixes #9691, fixes #9806, fixes #9961, fixes #9964, fixes #10013, fixes #10048

### How

A binding-reading side-effect statement of a user-declared side-effect-free ESM module is now included **iff the module's body is demanded**: one of its own (non-re-export) exports or its namespace object becomes used. This mirrors the lazy-barrel loader's `local`/`All` classification exactly — the loader loads *every* plain import record precisely in those cases — so a kept statement can never reference an unloaded record, and a deferred record is only ever referenced by dropped statements. The loader itself needs no changes.

Behavior deliberately kept as-is (pinned by existing tests):

- statements with no module-symbol references (a bare `console.log()`) and import/re-export statements (they drive wrapper `init_*` chains and side-effect imports) keep the unconditional sweep — `package_json_side_effects_false_keep_*` esbuild fixtures;
- bodies of modules whose own export is demanded (#7597's top-level asserts) or whose namespace is observed (`import * as ns`, `require(esm)`) are retained unchanged;
- user-defined entries, `eval` modules, and CommonJS modules are exempt.

Dynamic entries are **not** exempt: a live dynamic entry's body joins through namespace/own-export demand, while a dead pure dynamic import must not resurrect a body its removal already models as an empty namespace (covered by the new `tree_shaking/dead_dynamic_entry_side_effect_free` fixture).

### Alternatives explored

- Loader-side fix (force-load records referenced by retained statements): rejected — the deferral strategy is correct; the retention is the bug.
- Blanket statement-level stripping for `sideEffects: false` modules: rejected — breaks the esbuild-parity `keep_*` fixtures (namespace/require demand must keep the body).
- Statement-granular demand edges (keep a statement once any symbol it references is used): rejected — still dropped #7597's asserts.

### Notes for reviewers

- Inclusion changes only for `UserDefined(false)` ESM modules reached exclusively through re-exports or wrapper refs; every pre-existing snapshot is byte-identical (full integration suite: 1785 passed, 0 failed).
- Each `issues/*` fixture executes its output with node; `issues/10013` and the dynamic-entry fixture additionally carry a local `node_modules/dep` that throws on evaluation, so re-emitting the dropped external import fails execution, not just the snapshot.
- Residual and deferred (benign): wrapped modules whose body was pruned still emit empty `init_x` shells; a follow-up no-op-init cleanup can drop them.
@graphite-app graphite-app Bot force-pushed the fix/body-demand-side-effect-inclusion branch from 9897950 to fdb678e Compare July 2, 2026 10:59
@graphite-app graphite-app Bot merged commit fdb678e into main Jul 2, 2026
34 checks passed
@graphite-app graphite-app Bot deleted the fix/body-demand-side-effect-inclusion branch July 2, 2026 11:03
@IWANABETHATGUY

IWANABETHATGUY commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

I found some issues but I'm not sure if it's correct to fix them in this direction. The treeshking part becomes too complicated, it's very hard to manually reason if the behavior is correct.

However, tests are passed, it's possitive to prevent future regressions after merging them.

Do you mean this issue #10085?

@hyf0

hyf0 commented Jul 2, 2026

Copy link
Copy Markdown
Member

I found some issues but I'm not sure if it's correct to fix them in this direction. The treeshking part becomes too complicated, it's very hard to manually reason if the behavior is correct.
However, tests are passed, it's possitive to prevent future regressions after merging them.

Do you mean this issue #10085?

No.

graphite-app Bot pushed a commit that referenced this pull request Jul 3, 2026
… binding but keeps its property reads) (#10109)

## What

Adds a Rust integration-test fixture at `crates/rolldown/tests/rolldown/issues/10099/` closes #10099.

## Why

#10099: importing only `dayjs` from `element-plus` emitted a bundle that threw `ReferenceError: defaults_default is not defined`. The `sideEffects:false` barrel (`index.mjs`) re-exports an **external** module's default (`dayjs`) as a passthrough while reading `defaults_default.install`/`.version` at the top level. Importing only that passthrough force-includes the barrel; under `experimental.lazyBarrel` the tree-shaker dropped the `defaults_default` import **and** its definition but kept the property reads → dangling reference.

Fixed by #10080 (gate `sideEffects:false` modules' side effects on body demand). This PR adds the regression test for the specific element-plus shape, which none of #10080's fixtures cover: here the inclusion driver is an **external-default passthrough** (the sibling fixtures use local passthroughs or dynamic-import wrapping).

## The fixture

- `_config.json`: `external:["extdep"]`, `experimental.lazyBarrel:true`, `treeshake.moduleSideEffects:false`
- barrel `index.js` default-imports `defaults.js`, reads `.install`/`.version`, and re-exports the external default as `extdep`
- entry `main.js` imports only `{ extdep }` and asserts it resolves
- `node_modules/extdep/` stub so the external passthrough resolves at runtime

`experimental.lazyBarrel: true` is load-bearing: the bug is lazyBarrel-gated and lazyBarrel is default-off since #10071, so without it the test would pass on both the base and the fix.

## Verification

- **Green** on current `main` (fix present): the harness executes the compiled entry (`expectExecuted` default) and the snapshot is clean — no `defaults_default`.
- **Red** on the pre-fix base: the exact fixture source + config through published `rolldown@1.1.3` throws `ReferenceError: defaults_default is not defined`.

Test-only; no source changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment