refactor(treeshake): migrate SideEffectDetector to Oxc's MayHaveSideEffects trait by Dunqing · Pull Request #8624 · rolldown/rolldown

Dunqing · 2026-03-10T15:04:18Z

Summary

Closes: #7918

Delegate expression/statement side-effect analysis to Oxc's MayHaveSideEffects trait instead of reimplementing it
SideEffectDetector now implements MayHaveSideEffectsContext and GlobalContext directly, keeping only bundler-specific logic
Remove utils.rs (593 lines) and global_reference.rs (828 lines) — logic now lives in Oxc
Remove BundlerSideEffectCtx bridge type, merging its fields and trait impls into SideEffectDetector
Remove phf dependency from rolldown_utils
Fix pre-existing bugs:
- Destructuring assignment targets ([] = rhs, {} = rhs) were incorrectly treated as side-effect-free — now delegated to Oxc which correctly identifies GetIterator/RequireObjectCoercible as observable
- Compound assignment operators (+=, -=, ||=, etc.) were missing a side-effect check — now delegated to Oxc which correctly treats them as always side-effectful

Net: -1555 lines

Why some checks still can't be delegated to Oxc

Oxc's MayHaveSideEffects is a pure JS semantics analysis — it has no concept of bundler-level concerns. The remaining Rolldown-specific overrides are:

Override	Why Oxc can't handle it
`SideEffectDetail` metadata flags (`GlobalVarAccess`, `PureAnnotation`, `PureCjs`)	These are bundler metadata bits propagated through the tree for downstream decisions (e.g. cross-module optimization, CJS interop). Oxc returns a plain `bool`.
*`import.meta.` is side-effect-free**	Oxc treats `import.meta` as a `MetaProperty` with no special semantics. The bundler knows `import.meta.url` etc. are safe reads because it controls their resolution.
CJS `exports.foo = ...` pattern	Oxc sees a property write on an unresolved global (`exports`) — side-effectful. The bundler knows this is a CJS export assignment and marks it `PureCjs`. Must be checked before delegating to Oxc.
Cross-module pure call tracking (`side_effect_free_call_expr_addr`)	The bundler's link stage identifies calls to functions proven pure across modules. Oxc only sees a single file at a time.
Module declarations (`import`/`export`)	Oxc would treat these as side-effectful statements. The bundler knows imports/re-exports are declarative and handles them in the link stage.
`using` declarations	Rolldown has custom logic for `using` declarators (only `null`, `undefined`, or `void expr` initializers are side-effect-free). This is stricter than generic analysis because `using` invokes `Symbol.dispose`.
Variable declarations with destructuring	Rolldown has custom rules: object destructuring depends on `property_read_side_effects`, array destructuring checks for nested patterns. These interact with bundler options that Oxc doesn't model at the statement level.
Pure-annotated call args	For `/@__PURE__/` calls, Rolldown must re-check args through its own detector (not Oxc's) because bundler-specific overrides (e.g. `import.meta.*`) apply to args too.

Test plan

All existing side-effect detection tests pass (33/33)

Related: oxc-project/oxc#19673

🤖 Generated with Claude Code

netlify · 2026-03-10T15:05:52Z

✅ Deploy Preview for rolldown-rs ready!

Name	Link
🔨 Latest commit	`fb14f30`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/69c29a1f0257010008068a20
😎 Deploy Preview	https://deploy-preview-8624--rolldown-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

This PR updates Rolldown’s side-effect analysis by migrating much of SideEffectDetector logic to Oxc’s MayHaveSideEffects machinery (via a new bundler-aware context), bumps Oxc to 0.117.0, and refreshes many integration snapshots to match the new tree-shaking/codegen output.

Changes:

Refactor SideEffectDetector to delegate side-effect decisions to Oxc (MayHaveSideEffects) while preserving bundler-specific overrides via BundlerSideEffectCtx.
Bump Oxc crate versions to 0.117.0 and update Cargo.lock accordingly.
Update numerous test snapshots to reflect new tree-shaking and runtime helper emission.

Reviewed changes

Copilot reviewed 43 out of 44 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs	Refactors side-effect detection to use Oxc `MayHaveSideEffects` with a bundler-aware context and adds parity assertions/tests.
crates/rolldown/src/ast_scanner/side_effect_detector/bundler_ctx.rs	New adapter implementing Oxc’s `MayHaveSideEffectsContext` using Rolldown options/scopes and cross-module pure-call marking.
crates/rolldown/src/ast_scanner/side_effect_detector/utils.rs	Removes most legacy helper logic after migration to Oxc’s implementation.
Cargo.toml	Bumps Oxc crates to `0.117.0` and adds a `[patch.crates-io]` section for local Oxc paths.
Cargo.lock	Updates lockfile for the Oxc bump and dependency graph changes.
crates/rolldown/tests/rolldown/tree_shaking/export_star/artifacts.snap	Snapshot update for changed export-star output/inlining.
crates/rolldown/tests/rolldown/tree_shaking/commonjs_mixed/artifacts.snap	Snapshot update reflecting new tree-shaking of global property accesses.
crates/rolldown/tests/rolldown/tree_shaking/commonjs_inline_const_computed_key_write_object/artifacts.snap	Snapshot update reflecting new tree-shaking of global property accesses.
crates/rolldown/tests/rolldown/topics/keep_names/issue_7481_2/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/rolldown/topics/keep_names/if_stmt/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/rolldown/topics/deconflict/wrapped_esm_export_named_function/artifacts.snap	Snapshot update reflecting changed prelude/helper emission and mapping offsets.
crates/rolldown/tests/rolldown/topics/deconflict/wrapped_esm_default_function/artifacts.snap	Snapshot update reflecting changed prelude/helper emission and mapping offsets.
crates/rolldown/tests/rolldown/plugin/plugin_context/custom_arg_in_resolve/artifacts.snap	Snapshot update reflecting new external `require` wrapper emission.
crates/rolldown/tests/rolldown/issues/5923/artifacts.snap	Snapshot update reflecting new tree-shaking behavior for export-star helpers.
crates/rolldown/tests/rolldown/issues/4472/artifacts.snap	Snapshot update reflecting new tree-shaking behavior for export-star helpers.
crates/rolldown/tests/rolldown/function/shim_missing_exports/basic_wrapped_esm/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/rolldown/function/module_types/issue-6345/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/rolldown/errors/plugin_error/artifacts.snap	Snapshot update; now includes a Rust backtrace in output.
crates/rolldown/tests/rolldown/cjs_compat/require/require_esm/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/rolldown/cjs_compat/esm_require_esm_unused/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/ts/ts_export_missing_es6/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/ts/ts_experimental_decorators_keep_names/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/ts/export_type_issue379/artifacts.snap	Snapshot update reflecting export-all helper inlining/reordering.
crates/rolldown/tests/esbuild/loader/require_custom_extension_string/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/loader/require_custom_extension_data_url/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/loader/require_custom_extension_base64/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/loader/loader_file/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/loader/auto_detect_mime_type_from_extension/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/importstar/issue176/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/importstar_ts/ts_import_star_export_star_capture/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/importstar_ts/ts_import_star_export_star_as_capture/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/importstar_ts/ts_import_star_export_import_star_capture/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/importstar_ts/ts_import_star_common_js_no_capture/artifacts.snap	Snapshot update reflecting new tree-shaking of global property accesses.
crates/rolldown/tests/esbuild/importstar_ts/ts_import_star_capture/artifacts.snap	Snapshot update reflecting export-all helper inlining.
crates/rolldown/tests/esbuild/default/top_level_await_forbidden_require/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/default/top_level_await_forbidden_require_dead_branch/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/default/require_txt/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/default/require_json/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/default/require_fs_browser/artifacts.snap	Snapshot update reflecting new external `require` wrapper emission.
crates/rolldown/tests/esbuild/default/require_child_dir_common_js/artifacts.snap	Snapshot update reflecting inlining of CJS wrapper.
crates/rolldown/tests/esbuild/default/export_forms_common_js/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/default/bundle_esm_with_nested_var_issue4348/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/dce/package_json_side_effects_false_keep_bare_import_and_require_es6/artifacts.snap	Snapshot update reflecting changed emitted helpers/side-effect classification.
crates/rolldown/tests/esbuild/dce/dce_of_symbol_instances/artifacts.snap	Snapshot update reflecting new DCE behavior for `Symbol.*` references.

Cargo.toml

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs

crates/rolldown/tests/rolldown/errors/plugin_error/artifacts.snap

crates/rolldown/tests/rolldown/plugin/plugin_context/custom_arg_in_resolve/artifacts.snap

crates/rolldown/tests/esbuild/default/require_fs_browser/artifacts.snap

hyf0 · 2026-03-13T02:30:38Z

I guess oxc doesn't care about cjs related stuffs? #8659

Dunqing · 2026-03-13T02:33:39Z

I guess oxc doesn't care about cjs related stuffs? #8659

Yes, keep some Rolldown-specific checks as-is, and any others are delegated to Oxc

codspeed-hq · 2026-03-23T02:50:35Z

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks¹

_{Comparing oxc-MayHaveSideEffectsContext (fb14f30) with main (7833704)²}

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. ↩
No successful run was found on main (557d377) during the generation of this report, so 7833704 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Dunqing · 2026-03-23T03:11:43Z

Ready to review now; the PR description explains what checks still remain on the Rolldown side.

Note to reviewer: Due to the Oxc's MayHaveSideEffects trait, it is stricter and more accurate, so some behavior may have changed, and the modified test reflects this, which we should be careful to review.

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs

Boshen

I see some discrepancies, the rule should be always follow the spec going forward.

…ffects trait Delegate expression/statement side-effect analysis to Oxc's `MayHaveSideEffects` trait instead of reimplementing it. SideEffectDetector now implements `MayHaveSideEffectsContext` and `GlobalContext` directly, keeping only bundler-specific logic (CJS exports, import.meta, module declarations, cross-module pure call tracking, metadata flags like GlobalVarAccess/PureAnnotation). - Remove `utils.rs` (593 lines) and `global_reference.rs` (828 lines) - Remove `BundlerSideEffectCtx` bridge type, merging fields/traits into SideEffectDetector - Remove `phf` dependency from rolldown_utils Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

sapphi-red

LGTM on my side

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs

Dunqing · 2026-03-24T14:04:22Z

Thank you everyone for reviewing this! We have 5 approvals now; I am going to merge this!

@hyf0

## [1.0.0-rc.12] - 2026-03-25 ### 🚀 Features - chunk-optimizer: skip circular dependency check when strict execution order is enabled (#8886) by @hyf0 ### 🐛 Bug Fixes - emit build warnings during watch mode rebuilds (#8897) by @IWANABETHATGUY - lazy-barrel: load import-then-export specifiers when barrel has local exports (#8895) by @shulaoda - correct execution order of transferred CJS init calls (#8877) by @IWANABETHATGUY - mcs: `entriesAware` should calculate sizes without duplication (#8887) by @hyf0 - non-deterministic chunk generation (#8882) by @sapphi-red - `is_top_level` incorrectly treats strict-mode scopes as top-level (#8878) by @Dunqing ### 🚜 Refactor - treeshake: migrate SideEffectDetector to Oxc's MayHaveSideEffects trait (#8624) by @Dunqing ### 🧪 Testing - make dev server tests deterministic by replacing fixed sleeps with event-driven polling (#8561) by @Boshen ### ⚙️ Miscellaneous Tasks - deps: update dependency vite-plus to v0.1.14 (#8902) by @camc314 - deps: update dependency oxfmt to ^0.42.0 (#8891) by @renovate[bot] - deps: update rust crate oxc_sourcemap to v6.1.1 (#8890) by @renovate[bot] - remove Rolldown MF plan (#8883) by @shulaoda - deps: update rollup submodule for tests to v4.60.0 (#8881) by @sapphi-red - deps: update test262 submodule for tests (#8880) by @sapphi-red - deps: upgrade oxc crates to 0.122.0 (#8879) by @shulaoda Co-authored-by: shulaoda <[email protected]>

Copilot AI review requested due to automatic review settings March 10, 2026 15:04

Dunqing marked this pull request as draft March 10, 2026 15:04

Copilot started reviewing on behalf of Dunqing March 10, 2026 15:05 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

IWANABETHATGUY mentioned this pull request Mar 10, 2026

Global side effects that are not cleared yet #7918

Closed

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch from 32b1bbc to f215fd6 Compare March 12, 2026 05:30

h-a-n-a mentioned this pull request Mar 13, 2026

feat: treeshake module.exports property write #8659

Closed

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch 2 times, most recently from 3162ca3 to 1ee3871 Compare March 14, 2026 13:13

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch 3 times, most recently from 0b9eb73 to 5ad37d9 Compare March 21, 2026 14:57

Dunqing changed the title ~~refactor: migrate side_effect_detector to use MayHaveSideEffectsContext of Oxc~~ refactor(treeshake): migrate SideEffectDetector to Oxc's MayHaveSideEffects trait Mar 21, 2026

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch 8 times, most recently from 41fb951 to af5df21 Compare March 23, 2026 02:43

Dunqing marked this pull request as ready for review March 23, 2026 02:44

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch from 31ecc48 to 2d56a64 Compare March 23, 2026 02:56

Dunqing requested review from Boshen and h-a-n-a March 23, 2026 03:14

Dunqing requested review from IWANABETHATGUY, hyf0, sapphi-red and shulaoda March 23, 2026 03:14

Dunqing added the test: vite-tests Trigger Vite tests workflow on this PR label Mar 23, 2026

sapphi-red reviewed Mar 23, 2026

View reviewed changes

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs Outdated Show resolved Hide resolved

Boshen approved these changes Mar 24, 2026

View reviewed changes

Boshen assigned IWANABETHATGUY Mar 24, 2026

Dunqing force-pushed the oxc-MayHaveSideEffectsContext branch from 2d56a64 to 82e6b4a Compare March 24, 2026 03:21

sapphi-red approved these changes Mar 24, 2026

View reviewed changes

shulaoda reviewed Mar 24, 2026

View reviewed changes

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs Show resolved Hide resolved

shulaoda approved these changes Mar 24, 2026

View reviewed changes

h-a-n-a approved these changes Mar 24, 2026

View reviewed changes

IWANABETHATGUY reviewed Mar 24, 2026

View reviewed changes

crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs Show resolved Hide resolved

IWANABETHATGUY approved these changes Mar 24, 2026

View reviewed changes

Dunqing enabled auto-merge (squash) March 24, 2026 14:04

Merge branch 'main' into oxc-MayHaveSideEffectsContext

fb14f30

Dunqing merged commit 679f6a9 into main Mar 24, 2026
31 checks passed

Dunqing deleted the oxc-MayHaveSideEffectsContext branch March 24, 2026 14:09

This was referenced Mar 25, 2026

release: v1.0.0-rc.12 #8894

Closed

release: v1.0.0-rc.12 #8903

Merged

Conversation

Dunqing commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why some checks still can't be delegated to Oxc

Test plan

Uh oh!

netlify bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hyf0 commented Mar 13, 2026

Uh oh!

Dunqing commented Mar 13, 2026

Uh oh!

codspeed-hq bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Dunqing commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Boshen left a comment

Choose a reason for hiding this comment

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dunqing commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Dunqing commented Mar 10, 2026 •

edited

Loading

netlify bot commented Mar 10, 2026 •

edited

Loading

codspeed-hq bot commented Mar 23, 2026 •

edited

Loading

Dunqing commented Mar 23, 2026 •

edited

Loading

Dunqing commented Mar 24, 2026 •

edited

Loading