feat: treeshake module.exports property write#8659
Conversation
|
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.
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. |
✅ Deploy Preview for rolldown-rs canceled.
|
d24aa33 to
4ce7eaf
Compare
539e74a to
2efe7b1
Compare
|
Side effect detector is being moved to Oxc. |
Does it include CJS side-effect detection? |
|
Let's wait for the migration in #8624 complete and come back to this. |
a3b062f to
a0be4d1
Compare
module.exports related assignment as side-effect freemodule.exports property write
a0be4d1 to
4f1d736
Compare
4f1d736 to
b3330ef
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
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> = ...asPureCjs. - Enhanced AST scanning to recognize
module.exports.<prop>writes as named CommonJS exports and track them via newEcmaModuleAstUsageflags. - 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. |
| self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsPropWriteRef); | ||
| } | ||
| _ => self.result.ast_usage.insert(EcmaModuleAstUsage::ModuleExportsNonPropWriteRef), |
There was a problem hiding this comment.
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).
| 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), |
| // `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)); | ||
| } | ||
|
|
There was a problem hiding this comment.
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).
| // | ||
| // 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); |
There was a problem hiding this comment.
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.
b3330ef to
fc7b86a
Compare

Extended the side effect detector to identify
module.exports.property = ...patterns asPureCjsoperations.This adds on top of the existing
exports.xxxsupport.This PR previously detects the side-effect of
module.exportsas a whole. But there's some issue with tree-shaking. Will take a look at it once I have a thorough knowledge of this.