fix: mark import binding reads as execution-order sensitive#10169
Closed
hyfdev wants to merge 1 commit into
Closed
fix: mark import binding reads as execution-order sensitive#10169hyfdev wants to merge 1 commit into
hyfdev wants to merge 1 commit into
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. |
hyfdev
force-pushed
the
codex/split-order-effects
branch
from
July 7, 2026 09:12
81d614e to
81656d0
Compare
hyfdev
force-pushed
the
codex/import-binding-order-read
branch
from
July 7, 2026 09:12
ad8ec67 to
99a493e
Compare
hyfdev
force-pushed
the
codex/import-binding-order-read
branch
from
July 7, 2026 09:40
99a493e to
7a5a564
Compare
hyfdev
force-pushed
the
codex/split-order-effects
branch
from
July 7, 2026 09:40
81656d0 to
092fcb4
Compare
Member
Author
|
wrong direction, it's not sound. |
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.
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
This PR adds an analyzer-local
ImportBindingReadorder-sensitive reason.It marks top-level reads of imported values as execution-order sensitive without adding them to the public tree-shaking
StmtEvalFlags:fooobj.valuelocal[key]ns.fooIt intentionally does not mark bare namespace object reads, and it does not walk into function bodies for this top-level order analysis. Computed member keys only propagate
ImportBindingRead; this does not bring back general global or pure-annotation propagation for member expressions.Why
An imported binding read is not a tree-shaking side effect by itself. Removing or retaining the statement should still be decided by the tree-shaking side-effect analysis.
It is execution-order sensitive, though, because the observed value can depend on when the importing module runs relative to the exporting module. The generated chunk order needs that information.
Tests
cargo test -p rolldown stmt_eval_analyzer --libcargo check -p rolldown