Skip to content

fix: mark import binding reads as execution-order sensitive#10169

Closed
hyfdev wants to merge 1 commit into
codex/split-order-effectsfrom
codex/import-binding-order-read
Closed

fix: mark import binding reads as execution-order sensitive#10169
hyfdev wants to merge 1 commit into
codex/split-order-effectsfrom
codex/import-binding-order-read

Conversation

@hyfdev

@hyfdev hyfdev commented Jul 7, 2026

Copy link
Copy Markdown
Member

Stacked on #10168.

What

This PR adds an analyzer-local ImportBindingRead order-sensitive reason.

It marks top-level reads of imported values as execution-order sensitive without adding them to the public tree-shaking StmtEvalFlags:

  • direct imported binding reads, such as foo
  • member chains rooted at an imported binding, such as obj.value
  • imported bindings used in computed member keys, such as local[key]
  • namespace import member reads, such as ns.foo

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

@hyfdev hyfdev changed the title fix: mark import binding reads order sensitive fix: mark import binding reads as execution-order sensitive Jul 7, 2026
@hyfdev
hyfdev force-pushed the codex/split-order-effects branch from 81d614e to 81656d0 Compare July 7, 2026 09:12
@hyfdev
hyfdev force-pushed the codex/import-binding-order-read branch from ad8ec67 to 99a493e Compare July 7, 2026 09:12
@hyfdev
hyfdev force-pushed the codex/import-binding-order-read branch from 99a493e to 7a5a564 Compare July 7, 2026 09:40
@hyfdev
hyfdev force-pushed the codex/split-order-effects branch from 81656d0 to 092fcb4 Compare July 7, 2026 09:40
@hyfdev

hyfdev commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

wrong direction, it's not sound.

@hyfdev hyfdev closed this Jul 7, 2026
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.
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.

1 participant