feat: detect top-level import-binding reads as execution-order sensitive#10180
Merged
Conversation
Member
Author
How to use the Graphite Merge QueueAdd 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. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated AST walk (TopLevelImportReadDetector) to conservatively detect top-level statements whose module-evaluation-time execution reads an imported binding, and ORs that signal into EcmaViewMeta::ExecutionOrderSensitive to make execution-order sensitivity detection more complete for future on-demand wrapping decisions.
Changes:
- Introduces
TopLevelImportReadDetectorto detect imported-binding reads via a uniform AST traversal (with intentional skips for deferred contexts). - Wires the detector into
AstScannerso import reads contribute toExecutionOrderSensitivealongside the existing statement eval analyzer. - Adds unit tests covering several import-read patterns and deferred-body exclusions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/rolldown/src/ast_scanner/top_level_import_read.rs | New detector implementation + unit tests for top-level import-binding read detection. |
| crates/rolldown/src/ast_scanner/mod.rs | Registers the new top_level_import_read module. |
| crates/rolldown/src/ast_scanner/impl_visit.rs | Integrates the detector into the scan loop to set ExecutionOrderSensitive. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
graphite-app
Bot
changed the base branch from
codex/split-order-effects
to
graphite-base/10180
July 7, 2026 16:39
Member
|
It looks like the pr stacked on the wrong base branch |
Member
|
The detection logic and the inertness reasoning both look solid to me. One small thing on the test side: The doc-comment explicitly calls out a couple of behaviors that don't have tests pinning them down yet — worth locking in since they're easy to regress if the walk behavior ever drifts:
Both are cheap to add and would nail down exactly the contract the doc promises. Not blocking — the existing 8 tests already cover the interesting cases (the computed-key and namespace-alias ones especially). |
IWANABETHATGUY
approved these changes
Jul 8, 2026
Add `TopLevelImportReadDetector`, a dedicated uniform AST walk that flags a top-level statement as execution-order sensitive when its module-evaluation-time execution reads an imported binding, OR-ed into `EcmaViewMeta::ExecutionOrderSensitive`. The signal is complete by construction. Instead of threading the fact through the per-expression-form side-effect analyzer — where gaps slip in (a mid-chain computed key `a[imp].y`, a namespace aliased into a local) — it 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). Function/arrow bodies (call-time), import declarations and re-export specifiers (no value read), and type annotations are skipped. 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 change that will let the flag decide unwrapping on its own.
hyfdev
force-pushed
the
feat/top-level-import-read-walk
branch
from
July 8, 2026 03:53
f61c701 to
4d3054b
Compare
hyfdev
force-pushed
the
graphite-base/10180
branch
from
July 8, 2026 03:53
092fcb4 to
63931c0
Compare
✅ Deploy Preview for rolldown-rs canceled.
|
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 8, 2026
…gged (#10190) Follow-up to #10180. `ExecutionOrderSensitive` is a module-level flag, so a single positive is enough. This guards the per-statement check in the scanner loop with `!contains(ExecutionOrderSensitive)`, so once a module has been flagged the remaining statements skip both the analyzer's order-sensitive check and the `TopLevelImportReadDetector` walk. Pure perf — no behavior change (the flag's final value is identical).
Merged
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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 intoEcmaViewMeta::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:
new, which the side-effect / pure-annotation analysis already reports as order-sensitive.export { a },export { a } from '...') forward bindings without reading a value, so pure barrels stay unmarked.Known limitation, inherent to any syntactic analysis: under
treeshake.propertyReadSideEffects: false, a getter /Proxytrap 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
ExecutionOrderSensitiveis additionally gated byimport_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 theimport_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; thea[key].ymid-chain computed-key andconst x = ns; x.foonamespace-alias cases; function / arrow / method bodies skipped; re-export barrels and locals not flagged; static field initializer andextendsclause.cargo fmt --checkandcargo clippy -p rolldown --lib --testsclean.