Skip to content

feat: treeshake module.exports property write#8659

Open
h-a-n-a wants to merge 1 commit into04-01-feat_support_inlineconst_for_cjs_exports_accessed_through_module.exportsfrom
03-13-feat_detect_module.exports_related_assignment_as_side-effect_free
Open

feat: treeshake module.exports property write#8659
h-a-n-a wants to merge 1 commit into04-01-feat_support_inlineconst_for_cjs_exports_accessed_through_module.exportsfrom
03-13-feat_detect_module.exports_related_assignment_as_side-effect_free

Conversation

@h-a-n-a
Copy link
Copy Markdown
Member

@h-a-n-a h-a-n-a commented Mar 12, 2026

Extended the side effect detector to identify module.exports.property = ... patterns as PureCjs operations.

This adds on top of the existing exports.xxx support.

// side-effect free if xxx is a static property and yyy is side-effect free
module.exports.xxx = yyy

This PR previously detects the side-effect of module.exports as a whole. But there's some issue with tree-shaking. Will take a look at it once I have a thorough knowledge of this.

Copy link
Copy Markdown
Member Author

h-a-n-a commented Mar 12, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


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.

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 12, 2026

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit a0be4d1
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69c7eedf7bcbaf0008c3bc8a

@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch from d24aa33 to 4ce7eaf Compare March 12, 2026 16:39
@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch from 539e74a to 2efe7b1 Compare March 13, 2026 03:37
@Boshen
Copy link
Copy Markdown
Member

Boshen commented Mar 13, 2026

Side effect detector is being moved to Oxc.

@h-a-n-a
Copy link
Copy Markdown
Member Author

h-a-n-a commented Mar 13, 2026

Side effect detector is being moved to Oxc.

Does it include CJS side-effect detection?

@h-a-n-a
Copy link
Copy Markdown
Member Author

h-a-n-a commented Mar 13, 2026

Let's wait for the migration in #8624 complete and come back to this.

@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch 3 times, most recently from a3b062f to a0be4d1 Compare March 28, 2026 15:08
@h-a-n-a h-a-n-a changed the title feat: detect module.exports related assignment as side-effect free feat: treeshake module.exports property write Mar 28, 2026
@h-a-n-a h-a-n-a changed the base branch from main to graphite-base/8659 April 1, 2026 06:50
@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch from a0be4d1 to 4f1d736 Compare April 1, 2026 06:50
@h-a-n-a h-a-n-a changed the base branch from graphite-base/8659 to 04-01-feat_support_inlineconst_for_cjs_exports_accessed_through_module.exports April 1, 2026 06:50
@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch from 4f1d736 to b3330ef Compare April 1, 2026 06:58
@h-a-n-a h-a-n-a marked this pull request as ready for review April 1, 2026 06:59
@h-a-n-a h-a-n-a requested a review from IWANABETHATGUY April 1, 2026 06:59
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 1, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free (fc7b86a) with 04-01-feat_support_inlineconst_for_cjs_exports_accessed_through_module.exports (f1e6e0d)

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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends CommonJS export analysis to treat module.exports.<staticProp> = ... as a side-effect-free (“PureCjs”) operation, enabling additional tree-shaking opportunities similar to existing exports.<prop> handling.

Changes:

  • Updated side-effect detection to classify module.exports.<staticProp> = ... as PureCjs.
  • Enhanced AST scanning to recognize module.exports.<prop> writes as named CommonJS exports and track them via new EcmaModuleAstUsage flags.
  • Updated integration snapshots to reflect new tree-shaking/output behavior.

Reviewed changes

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

Show a summary per file
File Description
crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs Detects module.exports.<staticProp> assignments as PureCjs and adds unit tests.
crates/rolldown/src/ast_scanner/impl_visit.rs Tracks module.exports.<prop> writes as CommonJS named exports; introduces new AST usage flags and normalization logic.
crates/rolldown_common/src/ecmascript/ecma_view.rs Adds EcmaModuleAstUsage flags to distinguish module.exports property writes vs other module usages.
crates/rolldown/tests/rolldown/topics/exports/entry_cjs_named_default/artifacts.snap Snapshot updated for new generated output/export wiring.
crates/rolldown/tests/esbuild/default/warn_common_js_exports_in_esm_convert/artifacts.snap Snapshot updated showing module.exports.<prop> write can now be tree-shaken from emitted output.

Comment on lines +605 to +607
self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsPropWriteRef);
}
_ => self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsNonPropWriteRef),
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In process_identifier_ref_by_scope for the global module identifier, CommonJsAstType::EsModuleFlag currently falls into the default arm and sets ModuleExportsNonPropWriteRef. That means module.exports.__esModule = true is treated like a non-property write and will later clear AllStaticExportPropertyAccess, which defeats the intent of allowing module.exports.<static> writes to remain treeshakeable. Consider treating EsModuleFlag as a static module.exports property write for the purpose of setting ModuleExportsPropWriteRef (without necessarily incrementing cjs_named_exports_usage).

Suggested change
self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsPropWriteRef);
}
_ => self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsNonPropWriteRef),
self.result
.ast_usage
.insert(EcmaModuleAstUsage::ModuleExportsPropWriteRef);
}
Some(CommonJsAstType::EsModuleFlag) => {
// Treat `module.exports.__esModule = true` as a static property write so it
// doesn't clear `AllStaticExportPropertyAccess`, but don't track it as a
// named export in `cjs_named_exports_usage`.
self
.result
.ast_usage
.insert(EcmaModuleAstUsage::ModuleExportsPropWriteRef);
}
_ => self
.result
.ast_usage
.insert(EcmaModuleAstUsage::ModuleExportsNonPropWriteRef),

Copilot uses AI. Check for mistakes.
Comment on lines +321 to +332
// `module.exports.test = ...` — create facade symbol like `exports.test = ...`
if let Some((span, export_name)) = member_expr.static_property_info() {
let exported_symbol =
self.result.symbol_ref_db.create_facade_root_symbol_ref(export_name);

self.declare_link_only_symbol_ref(exported_symbol.symbol);

if let Some(value) = self.extract_constant_value_from_expr(Some(&node.right)) {
self
.add_constant_symbol(exported_symbol.symbol, ConstExportMeta::new(value, true));
}

Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new module.exports.<prop> = ... handling records a constant export based solely on node.right. For compound assignments like module.exports.foo += 1, node.right can be a literal but the exported value is not that literal, so this would incorrectly mark foo as a constant and may cause invalid inlining. Gate the constant extraction on the assignment operator being a simple = assignment (and consider applying the same guard to the existing exports.<prop> branch too).

Copilot uses AI. Check for mistakes.
Comment on lines +172 to 179
//
// Access apart from module.exports.xxx or exports.xxx
// will be considered as non-static property access
if self.result.ast_usage.contains(EcmaModuleAstUsage::ModuleExportsNonPropWriteRef)
|| (!self.result.ast_usage.contains(EcmaModuleAstUsage::ModuleExportsPropWriteRef)
&& !self.result.ast_usage.contains(EcmaModuleAstUsage::ExportsRef))
{
self.result.ast_usage.remove(EcmaModuleAstUsage::AllStaticExportPropertyAccess);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above the AllStaticExportPropertyAccess normalization says “Access apart from module.exports.xxx or exports.xxx will be considered as non-static property access”, but the actual logic is driven by ModuleExports{Prop,NonProp}WriteRef and ExportsRef and will also clear the flag for cases like module.exports.foo reads and module.exports = ... assignments. Updating the comment to reflect that this is specifically about static property writes (and that other module/exports uses bail out) would make the intent clearer.

Copilot uses AI. Check for mistakes.
@h-a-n-a h-a-n-a force-pushed the 03-13-feat_detect_module.exports_related_assignment_as_side-effect_free branch from b3330ef to fc7b86a Compare April 1, 2026 07:58
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.

3 participants