Skip to content

fix(treeshake): propagate pure annotation through compound exprs#9431

Merged
shulaoda merged 4 commits into
mainfrom
fix/treeshake-pure-annotation-compound-expr
May 18, 2026
Merged

fix(treeshake): propagate pure annotation through compound exprs#9431
shulaoda merged 4 commits into
mainfrom
fix/treeshake-pure-annotation-compound-expr

Conversation

@Dunqing

@Dunqing Dunqing commented May 18, 2026

Copy link
Copy Markdown
Contributor

Root cause

SideEffectDetector::detect_side_effect_of_expr's catch-all arm collapses any compound expression (ObjectExpression, ArrayExpression, SequenceExpression, Conditional/Logical/Binary/Unary expressions, TemplateLiteral) into a plain bool via From<bool> for SideEffectDetail:

impl From<bool> for SideEffectDetail {
  fn from(value: bool) -> Self {
    if value { SideEffectDetail::Unknown } else { SideEffectDetail::empty() }
  }
}

That conversion can only encode Unknown or empty. The PureAnnotation and GlobalVarAccess metadata bits on a buried call are silently dropped at the catch-all and never reach the statement-level intersection that drives EcmaViewMeta::ExecutionOrderSensitive.

Concretely, export default { foo: /* @__PURE__ */ (() => globalValue)() } reaches statement level with SideEffectDetail::empty(). wrap_modules (crates/rolldown/src/stages/link_stage/wrapping.rs) takes the avoid_wrapping branch, WrapKind::None is emitted, and the module's top-level var foo_inner_default = ... runs at chunk top level — before sibling modules like setup.js that the IIFE depends on:

var foo_inner_default = { foo: /* @__PURE__ */ (() => globalValue)() };  // globalValue is undefined here
__esmMin((() => {
  globalThis.globalValue = "foo";
  // ...
}))();

Why this was latent before oxc 0.131

oxc 0.130's DCE pass unconditionally inlined /* @__PURE__ */ (() => globalValue)() down to a bare globalValue identifier. After inlining, the detector's Identifier arm independently flagged the unresolved global via GlobalVarAccess (a code path separate from PureAnnotation), the module was marked ExecutionOrderSensitive, and wrapping kicked in. Wrap-correctness was accidentally riding on oxc's IIFE inlining.

oxc 0.131 (oxc-project/oxc#22349) intentionally preserves pure-annotated IIFEs in DCE-only mode so downstream tree-shakers can see the annotation. That is the right call on oxc's side — but it removes the inline-to-bare-global behavior rolldown was implicitly leaning on, exposing this detector gap.

Changes

  • Add explicit arms in detect_side_effect_of_expr for the transparent compound expression types so metadata propagates: ObjectExpression, ArrayExpression, SequenceExpression, ConditionalExpression, LogicalExpression, BinaryExpression, UnaryExpression, TemplateLiteral.
  • Each arm goes through a single helper fold_compound(expr, children) that:
    1. Asks oxc's may_have_side_effects for the spec-level boolean (handles ToPrimitive coercion, getter triggers in spread, Symbol checks in templates, etc.). If oxc says yes, return Unknown and stop.
    2. Otherwise recurses through the children via rolldown's own detector to harvest PureAnnotation / GlobalVarAccess flags.
    3. Strips Unknown from the harvested result since oxc already certified the parent has no side effect — only metadata bits should propagate.
  • Peel transparent syntactic wrappers ((x), x as T, x satisfies T, x!, <T>x, x<T>) at the top of detect_side_effect_of_expr via Expression::get_inner_expression() — they add no runtime semantics, so a single peel covers all five wrapper variants (and any future ones oxc adds to that method).

This mirrors the existing gate-and-harvest pattern already used by detect_side_effect_of_call_expr and the NewExpression arm.

Tests

New unit tests in side_effect_detector::test:

  • test_pure_annotation_propagates_through_compound_expr — covers PureAnnotation propagation through Object/Array/Sequence/Conditional/Logical/Binary (===) / Unary (typeof) / computed-key / nested compound, plus GlobalVarAccess propagation through compound, plus TS wrapper passthroughs (as, satisfies, !).
  • test_pure_annotation_not_propagated_through_function_body — verifies a pure annotation inside an arrow/function body does not propagate to the statement level (function bodies aren't evaluated at module init).
  • test_compound_expr_side_effectful_operand_still_unknown — verifies a real side effect anywhere in a compound expression still surfaces as Unknown; the new arms don't weaken side-effect detection.

Existing integration suites kept green without snapshot changes:

cargo test --package rolldown --lib ast_scanner::side_effect_detector::test::
cargo test --package rolldown --test integration -- strict_execution_order  # 24 passed
cargo test --package rolldown --test integration -- tree_shaking             # 72 passed

The two pre-existing integration fixtures called out in the issue (function/experimental/strict_execution_order/concatenate_wrapped_modules and .../issue_5303) pass with their current snapshots — those snapshots already encode the wrapped behavior, so this PR restores conformance to them after the oxc 0.131 bump (#9424) would otherwise break them.

Closes #9425.

The side-effect detector's catch-all collapses any compound expression
(object/array/sequence/conditional/logical/binary/unary/literal) into a
plain bool via `From<bool> for SideEffectDetail`, which can only encode
`Unknown` or `empty`. The `PureAnnotation` and `GlobalVarAccess`
metadata bits on a buried call are silently dropped.

`export default { foo: /* @__PURE__ */ (() => globalValue)() }`
therefore reaches statement level without the `PureAnnotation` flag,
the module fails to be marked `ExecutionOrderSensitive`, and
`wrap_modules` takes the `avoid_wrapping` branch. The module's
top-level `var` initializer is emitted at chunk top level — before
sibling modules like `setup.js` that the IIFE depends on.

Latent before oxc 0.131: oxc 0.130's DCE inlined the pure-annotated
IIFE down to a bare `globalValue` identifier, which the detector's
Identifier arm independently flagged via `GlobalVarAccess`. oxc 0.131
(oxc-project/oxc#22349) deliberately preserves the IIFE so downstream
tools see the annotation — surfacing this detector gap.

Add explicit arms for the transparent compound expression types
(`Object`/`Array`/`Sequence`/`Conditional`/`Logical`/`Binary`/`Unary`/
`TemplateLiteral`) so metadata propagates. Each arm gates via oxc's
`may_have_side_effects` for the spec-level boolean, then folds the
children through rolldown's own detector to harvest the flags,
stripping `Unknown` because oxc already certified the parent has no
side effect. Transparent wrappers (`(x)`, `x as T`, `x satisfies T`,
`x!`, `<T>x`, `x<T>`) are peeled at the top of `detect_side_effect_of_expr`
via `Expression::get_inner_expression`.

Closes #9425.

Dunqing commented May 18, 2026

Copy link
Copy Markdown
Contributor 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.

@netlify

netlify Bot commented May 18, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

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

@Dunqing Dunqing marked this pull request as ready for review May 18, 2026 08:09
@Dunqing Dunqing requested a review from shulaoda May 18, 2026 08:10
@codspeed-hq

codspeed-hq Bot commented May 18, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing fix/treeshake-pure-annotation-compound-expr (a9dd84b) with main (a91bd20)

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.

The trailing `- Unknown` in `fold_compound` is load-bearing: rolldown's
detector can be more conservative than oxc for some sub-expressions
(e.g. CJS-export assignment via `check_pure_cjs_export` returns
`PureCjs` even when oxc considers the assignment side-effect-free).
Once oxc has certified the parent at the gate, we strip any `Unknown`
that a child contributed so only metadata bits propagate.

Also adds a unit-test case for `TSInstantiationExpression` (`f<T>`),
the fifth TS wrapper peeled by `get_inner_expression()` — the original
`<T>x` form (TSTypeAssertion) is ambiguous with JSX under the test
helper's `SourceType::tsx()` and isn't expressible here.
@Dunqing Dunqing force-pushed the fix/treeshake-pure-annotation-compound-expr branch from 60081a6 to 67cbd30 Compare May 18, 2026 08:54
@shulaoda shulaoda merged commit 8db2e76 into main May 18, 2026
31 checks passed
@shulaoda shulaoda deleted the fix/treeshake-pure-annotation-compound-expr branch May 18, 2026 14:47
@rolldown-guard rolldown-guard Bot mentioned this pull request May 20, 2026
shulaoda added a commit that referenced this pull request May 20, 2026
## [1.0.2] - 2026-05-20

### 🚀 Features

- devtools: emit package size in PackageGraphReady (#9434) by @IWANABETHATGUY
- devtools: classify package dependency types (#9427) by @IWANABETHATGUY
- devtools: map packages to modules and chunks (#9426) by @IWANABETHATGUY
- devtools: mark used packages (#9423) by @IWANABETHATGUY
- devtools: make duplicate packages discoverable (#9422) by @IWANABETHATGUY
- devtools: emit package metadata (#9421) by @IWANABETHATGUY
- update oxc to 0.132.0 (#9449) by @shulaoda
- update oxc to 0.131.0 (#9424) by @shulaoda
- allow checks.* to escalate emissions to hard errors (#9388) by @IWANABETHATGUY
- dev: support watcher options `include` and `exclude` (#9395) by @h-a-n-a
- emit warnings for invalid pure annotations (#9381) by @Kyujenius

### 🐛 Bug Fixes

- hash: keep chunk file names stable when an unrelated entry is added (#9444) by @hyf0
- call `codeSplitting.groups[].name` in deterministic order (#9457) by @sapphi-red
- dev/lazy: make `resolve_id` idempotent when the resolved id is already a lazy entry (#9439) by @h-a-n-a
- chunk-optimization: publish absorbed dynamic-entry namespace cross-chunk (#9448) by @IWANABETHATGUY
- treeshake: propagate pure annotation through compound exprs (#9431) by @Dunqing
- finalizer: skip redundant init call when barrel executes in same chunk (#9354) by @IWANABETHATGUY
- linking: initialize wrapped ESM re-export owners (#9353) by @IWANABETHATGUY
- do not inherit __toESM across chunks for named-only external imports (#9333) (#9415) by @IWANABETHATGUY
- watcher: don't write output or emit events after close() (#9328) by @situ2001
- chunk-optimization: avoid unsafe dynamic-only merges (#9398) by @IWANABETHATGUY
- cjs: rename CJS-wrapped locals that would shadow chunk-scope names (#9392) by @hyf0
- dev/lazy: watch lazy modules added in rebuilds (#9391) by @h-a-n-a

### 🚜 Refactor

- rolldown_dev: move dev example to break publish cycle (#9465) by @Boshen
- binding: drop unsafe napi string helper, hoist transform ArcStr (#9456) by @hyf0
- ecmascript_utils: split rewrite_ident_reference off JsxExt trait (#9417) by @IWANABETHATGUY
- use `ThreadsafeFunction::call_async_catch` (#9390) by @sapphi-red

### 📚 Documentation

- devtools: document @rolldown/debug usage and package graph consumption (#9435) by @IWANABETHATGUY
- replace `Inter` with system font stack in OG template SVG (#9240) by @yvbopeng
- remove `output.comments` warning as all issues have been resolved (#9393) by @sapphi-red
- in-depth: clarify @__PURE__ scope and document positions (#9389) by @Kyujenius
- readme: remove release candidate notice (#9387) by @shulaoda

### ⚡ Performance

- vite-resolve: cache importer existence checks (#9443) by @Brooooooklyn
- binding: reduce plugin string clones (#9436) by @Brooooooklyn

### 🧪 Testing

- enable `legal_comments_inline` test (#9394) by @sapphi-red

### ⚙️ Miscellaneous Tasks

- bump pnpm to v11.1.2 (#9447) by @Boshen
- deps: update rust crates (#9461) by @renovate[bot]
- deps: update rollup submodule for tests to v4.60.4 (#9453) by @rolldown-guard[bot]
- deps: update test262 submodule for tests (#9454) by @rolldown-guard[bot]
- deps: update npm packages (#9430) by @renovate[bot]
- deps: update github actions (#9429) by @renovate[bot]
- deps: update dependency rolldown-plugin-dts to v0.25.1 (#9452) by @renovate[bot]
- deps: update rust crates (#9428) by @renovate[bot]
- revert allow checks.* to escalate emissions to hard errors (#9388) (#9438) by @IWANABETHATGUY
- update mimalloc-safe to 0.1.61 (#9413) by @shulaoda
- deny `todo`, `unimplemented`, and `print_stderr` clippy lints (#9412) by @Boshen
- deps: update mimalloc-safe to 0.1.60 (#9410) by @shulaoda
- remove `pip install setuptools` workaround for node-gyp on macOS (#9370) by @sapphi-red
- renovate: disable automerge to require manual approval (#9386) by @shulaoda
- deps: update napi (#9385) by @renovate[bot]

### ❤️ New Contributors

* @yvbopeng made their first contribution in [#9240](#9240)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
V1OL3TF0X pushed a commit to V1OL3TF0X/rolldown that referenced this pull request May 25, 2026
…ldown#9431)

### Root cause

`SideEffectDetector::detect_side_effect_of_expr`'s catch-all arm
collapses any compound expression (`ObjectExpression`,
`ArrayExpression`, `SequenceExpression`,
`Conditional`/`Logical`/`Binary`/`Unary` expressions, `TemplateLiteral`)
into a plain bool via `From<bool> for SideEffectDetail`:

```rust
impl From<bool> for SideEffectDetail {
  fn from(value: bool) -> Self {
    if value { SideEffectDetail::Unknown } else { SideEffectDetail::empty() }
  }
}
```

That conversion can only encode `Unknown` or `empty`. The
`PureAnnotation` and `GlobalVarAccess` metadata bits on a buried call
are silently dropped at the catch-all and never reach the
statement-level intersection that drives
`EcmaViewMeta::ExecutionOrderSensitive`.

Concretely, `export default { foo: /* @__PURE__ */ (() => globalValue)()
}` reaches statement level with `SideEffectDetail::empty()`.
`wrap_modules` (`crates/rolldown/src/stages/link_stage/wrapping.rs`)
takes the `avoid_wrapping` branch, `WrapKind::None` is emitted, and the
module's top-level `var foo_inner_default = ...` runs at chunk top level
— before sibling modules like `setup.js` that the IIFE depends on:

```js
var foo_inner_default = { foo: /* @__PURE__ */ (() => globalValue)() };  // globalValue is undefined here
__esmMin((() => {
  globalThis.globalValue = "foo";
  // ...
}))();
```

### Why this was latent before oxc 0.131

oxc 0.130's DCE pass unconditionally inlined `/* @__PURE__ */ (() =>
globalValue)()` down to a bare `globalValue` identifier. After inlining,
the detector's `Identifier` arm independently flagged the unresolved
global via `GlobalVarAccess` (a code path separate from
`PureAnnotation`), the module was marked `ExecutionOrderSensitive`, and
wrapping kicked in. Wrap-correctness was accidentally riding on oxc's
IIFE inlining.

oxc 0.131 (oxc-project/oxc#22349) intentionally **preserves**
pure-annotated IIFEs in DCE-only mode so downstream tree-shakers can see
the annotation. That is the right call on oxc's side — but it removes
the inline-to-bare-global behavior rolldown was implicitly leaning on,
exposing this detector gap.

### Changes

- Add explicit arms in `detect_side_effect_of_expr` for the transparent
compound expression types so metadata propagates: `ObjectExpression`,
`ArrayExpression`, `SequenceExpression`, `ConditionalExpression`,
`LogicalExpression`, `BinaryExpression`, `UnaryExpression`,
`TemplateLiteral`.
- Each arm goes through a single helper `fold_compound(expr, children)`
that:
1. Asks oxc's `may_have_side_effects` for the spec-level boolean
(handles ToPrimitive coercion, getter triggers in spread, Symbol checks
in templates, etc.). If oxc says yes, return `Unknown` and stop.
2. Otherwise recurses through the children via rolldown's own detector
to harvest `PureAnnotation` / `GlobalVarAccess` flags.
3. Strips `Unknown` from the harvested result since oxc already
certified the parent has no side effect — only metadata bits should
propagate.
- Peel transparent syntactic wrappers (`(x)`, `x as T`, `x satisfies T`,
`x!`, `<T>x`, `x<T>`) at the top of `detect_side_effect_of_expr` via
`Expression::get_inner_expression()` — they add no runtime semantics, so
a single peel covers all five wrapper variants (and any future ones oxc
adds to that method).

This mirrors the existing gate-and-harvest pattern already used by
`detect_side_effect_of_call_expr` and the `NewExpression` arm.

### Tests

New unit tests in `side_effect_detector::test`:

- `test_pure_annotation_propagates_through_compound_expr` — covers
`PureAnnotation` propagation through
`Object`/`Array`/`Sequence`/`Conditional`/`Logical`/`Binary` (`===`) /
`Unary` (`typeof`) / computed-key / nested compound, plus
`GlobalVarAccess` propagation through compound, plus TS wrapper
passthroughs (`as`, `satisfies`, `!`).
- `test_pure_annotation_not_propagated_through_function_body` — verifies
a pure annotation inside an arrow/function body does **not** propagate
to the statement level (function bodies aren't evaluated at module
init).
- `test_compound_expr_side_effectful_operand_still_unknown` — verifies a
real side effect anywhere in a compound expression still surfaces as
`Unknown`; the new arms don't weaken side-effect detection.

Existing integration suites kept green without snapshot changes:

```sh
cargo test --package rolldown --lib ast_scanner::side_effect_detector::test::
cargo test --package rolldown --test integration -- strict_execution_order  # 24 passed
cargo test --package rolldown --test integration -- tree_shaking             # 72 passed
```

The two pre-existing integration fixtures called out in the issue
(`function/experimental/strict_execution_order/concatenate_wrapped_modules`
and `.../issue_5303`) pass with their current snapshots — those
snapshots already encode the wrapped behavior, so this PR restores
conformance to them after the oxc 0.131 bump (rolldown#9424)
would otherwise break them.

Closes rolldown#9425.

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: dalaoshu <165626830+shulaoda@users.noreply.github.com>
V1OL3TF0X pushed a commit to V1OL3TF0X/rolldown that referenced this pull request May 25, 2026
## [1.0.2] - 2026-05-20

### 🚀 Features

- devtools: emit package size in PackageGraphReady (rolldown#9434) by @IWANABETHATGUY
- devtools: classify package dependency types (rolldown#9427) by @IWANABETHATGUY
- devtools: map packages to modules and chunks (rolldown#9426) by @IWANABETHATGUY
- devtools: mark used packages (rolldown#9423) by @IWANABETHATGUY
- devtools: make duplicate packages discoverable (rolldown#9422) by @IWANABETHATGUY
- devtools: emit package metadata (rolldown#9421) by @IWANABETHATGUY
- update oxc to 0.132.0 (rolldown#9449) by @shulaoda
- update oxc to 0.131.0 (rolldown#9424) by @shulaoda
- allow checks.* to escalate emissions to hard errors (rolldown#9388) by @IWANABETHATGUY
- dev: support watcher options `include` and `exclude` (rolldown#9395) by @h-a-n-a
- emit warnings for invalid pure annotations (rolldown#9381) by @Kyujenius

### 🐛 Bug Fixes

- hash: keep chunk file names stable when an unrelated entry is added (rolldown#9444) by @hyf0
- call `codeSplitting.groups[].name` in deterministic order (rolldown#9457) by @sapphi-red
- dev/lazy: make `resolve_id` idempotent when the resolved id is already a lazy entry (rolldown#9439) by @h-a-n-a
- chunk-optimization: publish absorbed dynamic-entry namespace cross-chunk (rolldown#9448) by @IWANABETHATGUY
- treeshake: propagate pure annotation through compound exprs (rolldown#9431) by @Dunqing
- finalizer: skip redundant init call when barrel executes in same chunk (rolldown#9354) by @IWANABETHATGUY
- linking: initialize wrapped ESM re-export owners (rolldown#9353) by @IWANABETHATGUY
- do not inherit __toESM across chunks for named-only external imports (rolldown#9333) (rolldown#9415) by @IWANABETHATGUY
- watcher: don't write output or emit events after close() (rolldown#9328) by @situ2001
- chunk-optimization: avoid unsafe dynamic-only merges (rolldown#9398) by @IWANABETHATGUY
- cjs: rename CJS-wrapped locals that would shadow chunk-scope names (rolldown#9392) by @hyf0
- dev/lazy: watch lazy modules added in rebuilds (rolldown#9391) by @h-a-n-a

### 🚜 Refactor

- rolldown_dev: move dev example to break publish cycle (rolldown#9465) by @Boshen
- binding: drop unsafe napi string helper, hoist transform ArcStr (rolldown#9456) by @hyf0
- ecmascript_utils: split rewrite_ident_reference off JsxExt trait (rolldown#9417) by @IWANABETHATGUY
- use `ThreadsafeFunction::call_async_catch` (rolldown#9390) by @sapphi-red

### 📚 Documentation

- devtools: document @rolldown/debug usage and package graph consumption (rolldown#9435) by @IWANABETHATGUY
- replace `Inter` with system font stack in OG template SVG (rolldown#9240) by @yvbopeng
- remove `output.comments` warning as all issues have been resolved (rolldown#9393) by @sapphi-red
- in-depth: clarify @__PURE__ scope and document positions (rolldown#9389) by @Kyujenius
- readme: remove release candidate notice (rolldown#9387) by @shulaoda

### ⚡ Performance

- vite-resolve: cache importer existence checks (rolldown#9443) by @Brooooooklyn
- binding: reduce plugin string clones (rolldown#9436) by @Brooooooklyn

### 🧪 Testing

- enable `legal_comments_inline` test (rolldown#9394) by @sapphi-red

### ⚙️ Miscellaneous Tasks

- bump pnpm to v11.1.2 (rolldown#9447) by @Boshen
- deps: update rust crates (rolldown#9461) by @renovate[bot]
- deps: update rollup submodule for tests to v4.60.4 (rolldown#9453) by @rolldown-guard[bot]
- deps: update test262 submodule for tests (rolldown#9454) by @rolldown-guard[bot]
- deps: update npm packages (rolldown#9430) by @renovate[bot]
- deps: update github actions (rolldown#9429) by @renovate[bot]
- deps: update dependency rolldown-plugin-dts to v0.25.1 (rolldown#9452) by @renovate[bot]
- deps: update rust crates (rolldown#9428) by @renovate[bot]
- revert allow checks.* to escalate emissions to hard errors (rolldown#9388) (rolldown#9438) by @IWANABETHATGUY
- update mimalloc-safe to 0.1.61 (rolldown#9413) by @shulaoda
- deny `todo`, `unimplemented`, and `print_stderr` clippy lints (rolldown#9412) by @Boshen
- deps: update mimalloc-safe to 0.1.60 (rolldown#9410) by @shulaoda
- remove `pip install setuptools` workaround for node-gyp on macOS (rolldown#9370) by @sapphi-red
- renovate: disable automerge to require manual approval (rolldown#9386) by @shulaoda
- deps: update napi (rolldown#9385) by @renovate[bot]

### ❤️ New Contributors

* @yvbopeng made their first contribution in [rolldown#9240](rolldown#9240)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
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.

side-effect detector loses PureAnnotation flag when a /* @__PURE__ */ call is nested inside ObjectExpression / ArrayExpression

2 participants