Skip to content

fix(treeshake): preserve eagerly evaluated child effects of manual pure chains#10427

Merged
IWANABETHATGUY merged 4 commits into
rolldown:mainfrom
Nic-Polumeyv:fix/manual-pure-eager-children
Jul 24, 2026
Merged

fix(treeshake): preserve eagerly evaluated child effects of manual pure chains#10427
IWANABETHATGUY merged 4 commits into
rolldown:mainfrom
Nic-Polumeyv:fix/manual-pure-eager-children

Conversation

@Nic-Polumeyv

@Nic-Polumeyv Nic-Polumeyv commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #10397, at both drop paths named in the issue's Cause section.

One note for review. Two unit tests from #10387 encoded the old behavior as their premise, their comments said so explicitly, and they now expect conservative retention in both analysis dimensions.

@Nic-Polumeyv
Nic-Polumeyv marked this pull request as ready for review July 24, 2026 02:27
@codspeed-hq

codspeed-hq Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing Nic-Polumeyv:fix/manual-pure-eager-children (56da358) with main (df12b36)

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.

@IWANABETHATGUY

Copy link
Copy Markdown
Member

I’ll make a few small changes directly on your branch to avoid unnecessary back-and-forth and help move this forward more efficiently.

@netlify

netlify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

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

@IWANABETHATGUY
IWANABETHATGUY enabled auto-merge (squash) July 24, 2026 04:24
Nic-Polumeyv and others added 4 commits July 24, 2026 12:24
Preserve eagerly evaluated children of manual pure chains.

Fixes rolldown#10397
…d helpers

`walk_member_expr_like` read like a unit-returning visitor but actually
returned the chain's root identifier, and each caller used only one of its two
outputs — the manual-pure check wanted the return value, the eager-child
analyzer wanted the visitor side effect.

Rename the shared traversal to `chain_root_visiting_eager_children` (its return
value is the root identifier) and expose two intent-named wrappers:
`chain_root_ident` for callers that only need the root, and
`for_each_eager_chain_child` (returns `()`) for callers that only drive the
visitor. Behavior is unchanged.
@IWANABETHATGUY
IWANABETHATGUY force-pushed the fix/manual-pure-eager-children branch from 37655d9 to 56da358 Compare July 24, 2026 04:24
@IWANABETHATGUY
IWANABETHATGUY merged commit cd742e7 into rolldown:main Jul 24, 2026
34 checks passed
graphite-app Bot pushed a commit that referenced this pull request Jul 24, 2026
…pure chains (#10432)

Follow-up to #10427.

#10427 restored the eagerly-evaluated child effects of manual-pure chains, but its eager-child walk maps *every* spread argument to an unknown side effect. So a manual-pure chain wrapping a statically-safe spread — `make(...[]).value`, `make(...'').value`, `make(...[])()` — is now retained wholesale, even though spreading an array, string, or template literal only runs built-in iterators. Rollup drops these.

## Fix

Surface spread arguments distinctly from plain eager children (new `EagerChild` enum) and analyze them with `analyze_spread_argument`, mirroring oxc's `Argument::may_have_side_effects`:

- array / string / template-literal operands are analyzed through the (oxc-gated) expression analyzer, so a side-effectful element (`make(...[effect()])`) or a nested spread (`make(...[...x])`) still retains the statement;
- any other operand stays conservatively unknown, since evaluating or iterating it may run a user-defined `Symbol.iterator`.

## Tests

New unit test `test_manual_pure_chains_drop_side_effect_free_spreads` covers both directions: safe literal spreads become removable, while effectful or opaque spreads stay retained — including the nested-spread (`...[...x]`) and bare-identifier (`...x`) cases that must not under-retain.
IWANABETHATGUY added a commit that referenced this pull request Jul 24, 2026
…rix to unit tests

The `manual-pure-functions` fixture (from #10427) re-tested the retain/remove
matrix that the `stmt_eval_analyzer` unit tests already own. A JS fixture only
needs to smoke-test that the `manualPureFunctions` option is wired through the
pipeline; the behavioral matrix belongs in fast, precise Rust unit tests.

- Trim `main.js` to two statements: one manual-pure call dropped (no
  side-effectful children) and one retained because an argument has a side
  effect — the latter also exercises the one pipeline behavior a unit test can't
  see, the `import styled from "styled-components"` binding staying alive.
- Add `test_manual_pure_chains_without_eager_children_are_side_effect_free`,
  covering the removed shapes (make.div, make.div`x`, make?.div(), make()(),
  make().div()) that previously had end-to-end-only coverage.

Test-only; no behavior change.
IWANABETHATGUY added a commit that referenced this pull request Jul 24, 2026
…rix to unit tests

The `manual-pure-functions` fixture (from #10427) re-tested the retain/remove
matrix that the `stmt_eval_analyzer` unit tests already own. A JS fixture only
needs to smoke-test that the `manualPureFunctions` option is wired through the
pipeline; the behavioral matrix belongs in fast, precise Rust unit tests.

- Reduce `main.js` to a single smoke case: a listed function's call
  (`styled.div`...``) is dropped, leaving just `import "styled-components";`.
- Add `test_manual_pure_chains_without_eager_children_are_side_effect_free`,
  covering the removed shapes (make.div, make.div`x`, make?.div(), make()(),
  make().div()) that previously had end-to-end-only coverage. The retained-child
  and spread matrices were already unit-tested.

Test-only; no behavior change.
IWANABETHATGUY added a commit that referenced this pull request Jul 24, 2026
…rix to unit tests

The `manual-pure-functions` fixture (from #10427) re-tested the retain/remove
matrix that the `stmt_eval_analyzer` unit tests already own. A JS fixture only
needs to smoke-test that the `manualPureFunctions` option is wired through the
pipeline; the behavioral matrix belongs in fast, precise Rust unit tests.

- Reduce `main.js` to a single smoke case: a listed function's call
  (`styled.div`...``) is dropped, leaving just `import "styled-components";`.
- Add `test_manual_pure_chains_without_eager_children_are_side_effect_free`,
  covering every removed shape the fixture used to own end-to-end — bare call,
  member read, tagged template, and optional/repeated/chained calls — plus a
  scoping check that an unlisted call keeps its side effects. The retained-child
  and spread matrices were already unit-tested.

Test-only; no behavior change.
hyfdev pushed a commit that referenced this pull request Jul 25, 2026
…rix to unit tests

The `manual-pure-functions` fixture (from #10427) re-tested the retain/remove
matrix that the `stmt_eval_analyzer` unit tests already own. A JS fixture only
needs to smoke-test that the `manualPureFunctions` option is wired through the
pipeline; the behavioral matrix belongs in fast, precise Rust unit tests.

- Reduce `main.js` to a single smoke case: a listed function's call
  (`styled.div`...``) is dropped, leaving just `import "styled-components";`.
- Add `test_manual_pure_chains_without_eager_children_are_side_effect_free`,
  covering every removed shape the fixture used to own end-to-end — bare call,
  member read, tagged template, and optional/repeated/chained calls — plus a
  scoping check that an unlisted call keeps its side effects. The retained-child
  and spread matrices were already unit-tested.

Test-only; no behavior change.
graphite-app Bot pushed a commit that referenced this pull request Jul 25, 2026
…move matrix to unit tests (#10436)

Follow-up to #10427 / #10432 (both merged).

There's exactly one JS-side `manualPureFunctions` test — the `manual-pure-functions` fixture (added in #10427). It re-tested the retain/remove behavior matrix that the Rust `stmt_eval_analyzer` unit tests already own. A JS fixture only needs to smoke-test that the option is wired through the pipeline; the exhaustive behavior belongs in fast, precise Rust unit tests.

## Changes

- **Fixture → minimal smoke test.** `main.js` is now a single case: a listed function's call (`` styled.div`...` ``) is dropped, leaving just `import "styled-components";`. That alone proves the `manualPureFunctions` option reaches the pipeline — without it, the call would be retained. Snapshot regenerated against a fresh debug build.
- **Rust unit test.** `test_manual_pure_chains_without_eager_children_are_side_effect_free` covers the removed shapes (`make.div`, `` make.div`x` ``, `make?.div()`, `make()()`, `make().div()`) that previously had end-to-end-only coverage. The retained-child and spread matrices were already unit-tested (`test_manual_pure_chains_keep_eager_child_side_effects`, `test_manual_pure_chains_drop_side_effect_free_spreads`).

Test-only; no behavior change.
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.

[Bug]: manualPureFunctions: a pure call inside a member/new chain drops eagerly-evaluated child effects (arguments, computed keys)

2 participants