Skip to content

refactor: separate tree-shaking side effects from execution order sensitivity#10168

Merged
graphite-app[bot] merged 1 commit into
mainfrom
codex/split-order-effects
Jul 7, 2026
Merged

refactor: separate tree-shaking side effects from execution order sensitivity#10168
graphite-app[bot] merged 1 commit into
mainfrom
codex/split-order-effects

Conversation

@hyfdev

@hyfdev hyfdev commented Jul 7, 2026

Copy link
Copy Markdown
Member

What

This PR splits the statement eval analyzer result into two separate answers:

  • tree-shaking side effects, stored in the public StmtEvalFlags
  • execution-order sensitivity, represented by analyzer-local order-sensitive reasons

StmtInfo::side_effects() continues to carry only the tree-shaking flags. EcmaViewMeta::ExecutionOrderSensitive now comes from StmtEvalFacts::is_order_sensitive(), which combines unknown runtime side effects with order-sensitive reasons such as global reads and pure annotations.

The cross-module optimization pass still re-runs the analyzer, but it consumes only tree_shaking_flags() because that pass mutates removal-related metadata, not module execution-order metadata.

Why

Rolldown needs to answer two different questions:

  • Can this statement or module be removed without observable behavior?
  • Does this statement or module need to keep its relative execution timing?

Keeping both answers in the same public bitset makes later order-sensitive facts look like tree-shaking side effects. That can affect chunk dependency edges or removal decisions for the wrong reason.

This PR is intended as a refactor that keeps existing behavior while making those two answers explicit.

Tests

  • cargo test -p rolldown stmt_eval_analyzer --lib
  • cargo check -p rolldown

hyfdev commented Jul 7, 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.

@netlify

netlify Bot commented Jul 7, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit 63931c0
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a4d2bc7a036550008dd315a
😎 Deploy Preview https://deploy-preview-10168--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.

@hyfdev hyfdev changed the title refactor: split stmt eval order sensitivity refactor: separate tree-shaking side effects from execution order sensitivity Jul 7, 2026
@hyfdev
hyfdev force-pushed the codex/split-order-effects branch 2 times, most recently from 81656d0 to 092fcb4 Compare July 7, 2026 09:40
@hyfdev
hyfdev marked this pull request as ready for review July 7, 2026 11:14
Copilot AI review requested due to automatic review settings July 7, 2026 11:14

hyfdev commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Merge activity

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 refactors Rolldown’s statement evaluation analysis to explicitly separate tree-shaking side-effect facts (public StmtEvalFlags) from execution-order sensitivity (analyzer-local reasons), so execution-order metadata doesn’t accidentally influence removal decisions.

Changes:

  • Introduces StmtEvalFacts (tree-shaking flags + order-sensitive reasons) and updates the statement analyzer to return facts rather than a shared bitset.
  • Updates AST scanning to set EcmaViewMeta::ExecutionOrderSensitive based on StmtEvalFacts::is_order_sensitive(), while storing only tree-shaking flags in StmtInfo.
  • Adjusts cross-module optimization to refresh only tree_shaking_flags() since it mutates removal-related metadata, not execution-order metadata.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
crates/rolldown/src/stages/link_stage/cross_module_optimization.rs Uses the analyzer’s new return type but persists only tree-shaking flags during mutation refresh.
crates/rolldown/src/ast_scanner/stmt_eval_analyzer/mod.rs Adds StmtEvalFacts + order-sensitive reasons and rewires analysis/folding logic to keep channels separate.
crates/rolldown/src/ast_scanner/impl_visit.rs Updates module scanning to store only tree-shaking flags in StmtInfo and to mark execution-order sensitivity via is_order_sensitive().
crates/rolldown_common/src/types/stmt_eval_flags.rs Narrows StmtEvalFlags to tree-shaking-relevant flags only.

@codspeed-hq

codspeed-hq Bot commented Jul 7, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing codex/split-order-effects (092fcb4) with main (72ce2b9)

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.

Comment thread crates/rolldown/src/stages/link_stage/cross_module_optimization.rs Outdated
…sitivity (#10168)

### What

This PR splits the statement eval analyzer result into two separate answers:

- tree-shaking side effects, stored in the public `StmtEvalFlags`
- execution-order sensitivity, represented by analyzer-local order-sensitive reasons

`StmtInfo::side_effects()` continues to carry only the tree-shaking flags. `EcmaViewMeta::ExecutionOrderSensitive` now comes from `StmtEvalFacts::is_order_sensitive()`, which combines unknown runtime side effects with order-sensitive reasons such as global reads and pure annotations.

The cross-module optimization pass still re-runs the analyzer, but it consumes only `tree_shaking_flags()` because that pass mutates removal-related metadata, not module execution-order metadata.

### Why

Rolldown needs to answer two different questions:

- Can this statement or module be removed without observable behavior?
- Does this statement or module need to keep its relative execution timing?

Keeping both answers in the same public bitset makes later order-sensitive facts look like tree-shaking side effects. That can affect chunk dependency edges or removal decisions for the wrong reason.

This PR is intended as a refactor that keeps existing behavior while making those two answers explicit.

### Tests

- `cargo test -p rolldown stmt_eval_analyzer --lib`
- `cargo check -p rolldown`
@graphite-app
graphite-app Bot force-pushed the codex/split-order-effects branch from c4ce8b9 to 63931c0 Compare July 7, 2026 16:39
@graphite-app
graphite-app Bot merged commit 63931c0 into main Jul 7, 2026
34 checks passed
@graphite-app
graphite-app Bot deleted the codex/split-order-effects branch July 7, 2026 16:44
hyfdev added a commit that referenced this pull request Jul 8, 2026
…ive (#10180)

Stacked on #10168.

### What

Adds `TopLevelImportReadDetector` — a dedicated, uniform AST walk that
marks a top-level statement as execution-order sensitive when its
module-evaluation-time execution reads an imported binding — and OR-es
it into `EcmaViewMeta::ExecutionOrderSensitive`.

### Why

On-demand wrapping should eventually decide "wrap or not" from
execution-order sensitivity alone, so that a module which imports
dependencies but only uses them inside function/class bodies can stay
unwrapped. For that, the order-sensitivity signal must be **complete**:
it may never miss a top-level read of an imported binding, or such a
module could be unwrapped and reordered across a mutation.

The earlier approach (#10169) threaded this fact through the
per-expression-form side-effect analyzer, which is exactly how gaps slip
in — e.g. an imported binding in a mid-chain computed key (`a[imp].y`)
or a namespace aliased into a local (`const x = ns; x.foo`). This
replaces that with a single uniform walk that visits every sub-node
once, so no expression form can be missed and newly added syntax is
covered by default. It is a deliberate over-approximation, which is the
sound direction for wrapping (under-reporting is the only dangerous
mistake).

Skipped, and why each stays sound:

- **Function / arrow bodies** (incl. class method bodies and parameter
defaults) run at call time. If such a body runs during module evaluation
it does so through a call / `new`, which the side-effect /
pure-annotation analysis already reports as order-sensitive.
- **Import declarations and re-export specifiers** (`export { a }`,
`export { a } from '...'`) forward bindings without reading a value, so
pure barrels stay unmarked.
- **Type annotations** are erased at runtime.

Known limitation, inherent to any syntactic analysis: under
`treeshake.propertyReadSideEffects: false`, a getter / `Proxy` trap that
reads an imported binding is invisible here — that is the option's own
"reads are side-effect-free" promise, not a gap this walk can close.

### Currently inert

Every unwrap / chunk-grouping consumer of `ExecutionOrderSensitive` is
additionally gated by `import_records.is_empty()`, and a module that
reads an import necessarily has an import record, so no wrap or chunk
decision changes yet. This lands the detection ahead of the follow-up
that drops the `import_records.is_empty()` gate and lets the flag decide
unwrapping on its own.

### Tests

- `cargo test -p rolldown --lib top_level_import_read` — 6 new unit
tests: direct / member / computed-key / compound reads; the `a[key].y`
mid-chain computed-key and `const x = ns; x.foo` namespace-alias cases;
function / arrow / method bodies skipped; re-export barrels and locals
not flagged; static field initializer and `extends` clause.
- `cargo fmt --check` and `cargo clippy -p rolldown --lib --tests`
clean.
- Full integration suite unchanged — the change is inert; the
strict-execution-order / wrapping / on-demand fixtures are green.
@rolldown-guard rolldown-guard Bot mentioned this pull request Jul 8, 2026
shulaoda added a commit that referenced this pull request Jul 8, 2026
## [1.1.5] - 2026-07-08

### 🚀 Features

- detect top-level import-binding reads as execution-order sensitive (#10180) by @hyf0
- sourcemap_filenames: add a sourcemapFileNames option (#9271) by @V1OL3TF0X
- binding: record plugin hook result kind in tracing spans (#10154) by @IWANABETHATGUY
- linking: skip side-effect-free modules in per-entry reachability (#10111) by @IWANABETHATGUY
- improve error message for unresolved virtual imports (#10156) by @sapphi-red
- add descriptive metadata to plugin API (#10106) by @sapphi-red
- add `--configLoader=native` option (#10118) by @sapphi-red

### 🐛 Bug Fixes

- improve invalid annotation warnings (#10185) by @hyf0
- keep deduplicated asset filenames stable once they can be observed (#10191) by @shulaoda
- sourcemap_filenames: use public option name in pattern errors (#10188) by @IWANABETHATGUY
- sourcemap_filenames: hash prepared sourcemap content (#10178) by @hyf0
- tree-shake unused circular declarators exported via export list (#10166) by @IWANABETHATGUY
- dev: don't panic when an HMR rebuild hits an unresolved import (#10162) by @shulaoda
- propagate errors from output.globals function (#9880) by @shulaoda
- dev: revert cache mutations when a partial scan fails (#10110) by @shulaoda
- dev: update importer relationships of cached modules in incremental build (#10107) by @shulaoda
- hmr: fall back to full reload when a changed module is not registered as executed (#10132) by @shulaoda
- chunk-optimizer: follow entry facade edges in runtime placement cycle check (#10101) by @hyf0
- dev: ignore watcher events after close (#10113) by @hyf0
- emit async wrapper for TLA modules under onDemandWrapping (#10086) by @IWANABETHATGUY
- gate sideEffects:false modules' side effects on body demand (#10080) by @IWANABETHATGUY
- rolldown_plugin_vite_resolve: return empty object for `browser: false` mapped modules (#10082) by @sapphi-red
- reset the word-boundary state on newline in Hires::Boundary sourcemaps (#10025) by @shulaoda
- trim an emptied chunk's outro/intro instead of skipping past it (#10029) by @shulaoda
- test each edited chunk's own start against indent exclude ranges (#10026) by @shulaoda
- preserve sourcemap mappings for indented lines when a CJS module shares the chunk (#10074) by @hyf0

### 🚜 Refactor

- separate tree-shaking side effects from execution order sensitivity (#10168) by @hyf0
- type construct_vite_preload_call to take an ObjectPattern (#10135) by @shulaoda
- treeshake: single-source the own-export classification shared with the lazy-barrel loader (#10098) by @IWANABETHATGUY
- dev: reuse Vite's bundledDev server (#10081) by @h-a-n-a
- clippy: ban std HashMap/HashSet in favour of FxHashMap/FxHashSet (#10108) by @Boshen
- treeshake: make body demand a second module bit instead of a stmt multimap (#10097) by @IWANABETHATGUY
- seal used_symbol_refs by construction after its last writer (#10091) by @hyf0
- treeshake: replace inclusion mutual recursion with a worklist engine (#10096) by @IWANABETHATGUY
- treeshake: split include_statements.rs into focused modules (#10095) by @IWANABETHATGUY
- drop redundant is_user_defined filter on partitioned entries (#10050) by @shulaoda
- project the retained export interface out of used_symbol_refs (#10089) by @hyf0
- track used external symbols separately from used_symbol_refs (#10088) by @hyf0
- make module namespace inclusion an explicit linking metadata field (#10087) by @hyf0
- rename statement evaluation metadata (#10078) by @hyf0

### 📚 Documentation

- virtual modules user-facing id convention (#10155) by @sapphi-red
- cli: clarify disabling boolean/object flags like codeSplitting (#10153) by @IWANABETHATGUY
- chore: remove Vite+ alpha banner (#10105) by @mdong1909
- write down the used_symbol_refs contract (#10090) by @hyf0
- dev/lazy: update design and implementation (#10079) by @h-a-n-a

### ⚡ Performance

- ast_scanner: stop order-sensitivity checks once a module is flagged (#10190) by @IWANABETHATGUY
- return impl ExactSizeIterator from slice-backed accessors (#10133) by @Boshen
- binding: box dev and watcher napi futures (#10103) by @Boshen

### 🧪 Testing

- move string_wizard replace unit tests to the JS magic-string suite (#10176) by @IWANABETHATGUY
- dev: assert incremental scan state matches a fresh full build after each HMR step (#10115) by @shulaoda
- dev: restore runtime assertions of delete_file_not_used_anymore (#10112) by @shulaoda
- dev: fix flaky dev server tests in CI (#10152) by @h-a-n-a
- add regression test for #10099 (lazyBarrel drops default-import binding but keeps its property reads) (#10109) by @IWANABETHATGUY

### ⚙️ Miscellaneous Tasks

- deploy website to Void via GitHub OIDC (#10192) by @Boshen
- deps: update oxc to 0.139.0 (#10161) by @shulaoda
- deps: update test262 submodule for tests (#10160) by @rolldown-guard[bot]
- rolldown_plugin_utils: remove dead asset-url and css scaffolding (#10131) by @shulaoda
- deps: revert vite-plus to v0.2.1 (#10148) by @shulaoda
- deps: update github actions (#10141) by @renovate[bot]
- deps: update dependency rust to v1.96.1 (#10145) by @renovate[bot]
- deps: update npm packages (#10142) by @renovate[bot]
- deps: update rust crates (#10143) by @renovate[bot]
- deps: update napi to v3.10.3 (#10121) by @renovate[bot]
- rolldown_utils: remove unused time module (#10138) by @shulaoda
- remove dead CopyModulePlugin::is_active method (#10129) by @shulaoda
- remove dead LazyCompilationContext::is_lazy_module method (#10128) by @shulaoda
- remove dead BuildDiagnostic::downcast_ref method (#10127) by @shulaoda
- deps: update dependency vite-plus to v0.2.2 (#10084) by @renovate[bot]
- deps: update rust crate oxc_sourcemap to v8.1.0 (#10122) by @renovate[bot]
- deps: update crate-ci/typos action to v1.48.0 (#10124) by @renovate[bot]
- enable more clippy restriction lints (#10114) by @Boshen
- deps: update rust dependencies (#10100) by @Boshen
- deps: update oxc resolver to v11.23.0 (#10083) by @renovate[bot]

### ◀️ Revert

- Revert "chore(deps): revert vite-plus to v0.2.1" (#10157) by @h-a-n-a
- "fix(hmr): fall back to full reload when a changed module is not registered as executed (#10132)" (#10151) by @shulaoda

### ❤️ New Contributors

* @V1OL3TF0X made their first contribution in [#9271](#9271)

Co-authored-by: shulaoda <[email protected]>
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