[turbopack] Treat Object.defineProperty(exports, …, { value: … }) as side-effect free (#96273)

Treat code like this:

```
Object.defineProperty(exports, '__esModule', { value: true });
exports.foo = 1;
```

As side-effect free. This is an extension of the work on
https://github.com/vercel/next.js/pull/94293 and will allow
https://github.com/vercel/next.js/pull/95718 to drop more dead requires.

---------

Co-authored-by: Benjamin Woodruff <[email protected]>

turbopack/crates/turbopack-ecmascript/src/analyzer/cjs_ast.rs

@@ -51,9 +51,12 @@ pub(crate) fn is_module_exports_chain(expr: &Expr, unresolved_mark: Mark) -> boo
     }
 }
 
-/// The exported name of `Object.defineProperty(exports, "<name>", { … })` — the
-/// shape transpilers emit for named exports and the `__esModule` marker.
-pub(crate) fn as_exports_define_property(call: &CallExpr, unresolved_mark: Mark) -> Option<RcStr> {
+/// The exported name and descriptor of `Object.defineProperty(exports, "<name>", { … })` —
+/// the shape transpilers emit for named exports and the `__esModule` marker.
+pub(crate) fn as_exports_define_property(
+    call: &CallExpr,
+    unresolved_mark: Mark,
+) -> Option<(RcStr, &ObjectLit)> {
     let Callee::Expr(callee) = &call.callee else {
         return None;
     };
@@ -78,10 +81,13 @@ pub(crate) fn as_exports_define_property(call: &CallExpr, unresolved_mark: Mark)
     let Expr::Lit(Lit::Str(name)) = unparen(&name.expr) else {
         return None;
     };
-    if !matches!(unparen(&descriptor.expr), Expr::Object(_)) {
+    let Expr::Object(descriptor) = unparen(&descriptor.expr) else {
         return None;
-    }
-    Some(RcStr::from(name.value.to_string_lossy().into_owned()))
+    };
+    Some((
+        RcStr::from(name.value.to_string_lossy().into_owned()),
+        descriptor,
+    ))
 }
 
 /// Whether the `Object.defineProperty` descriptor sets `value: true` (the

turbopack/crates/turbopack-ecmascript/src/analyzer/graph/visitor.rs

@@ -1548,7 +1548,8 @@ impl VisitAstPath for Analyzer<'_, '_> {
         // `Object.defineProperty(exports, …)` is a CommonJS export write; recognize
         // it so unused entries drop (its `exports` argument is guarded below).
         let is_cjs_define_property = if self.cjs_exports_enabled()
-            && let Some(name) = as_exports_define_property(n, self.eval_context.unresolved_mark)
+            && let Some((name, _)) =
+                as_exports_define_property(n, self.eval_context.unresolved_mark)
         {
             self.recognize_cjs_define_property(&name, n, ast_path);
             true

turbopack/crates/turbopack-ecmascript/src/analyzer/side_effects.rs

@@ -43,6 +43,7 @@ use swc_core::{
     common::{Mark, comments::Comments},
     ecma::{
         ast::*,
+        utils::prop_name_eq,
         visit::{Visit, VisitWith, noop_visit_type},
     },
 };
@@ -50,7 +51,8 @@ use turbopack_core::module::ModuleSideEffects;
 
 use crate::{
     analyzer::cjs_ast::{
-        is_cjs_export_member, is_global, is_module_dot_exports, is_module_exports_chain,
+        as_exports_define_property, is_cjs_export_member, is_global, is_module_dot_exports,
+        is_module_exports_chain,
     },
     utils::unparen,
 };
@@ -333,8 +335,9 @@ fn collect_safe_assignment_constant_ids(program: &Program) -> HashSet<Id> {
     // Drop any binding that later has an accessor attached to its object graph
     // (e.g. `o.x = { set y(v) {} }`): a subsequent write through that property
     // could invoke the accessor, so the binding is no longer safe to mutate.
-    for_each_top_level_assign(program, |assign| {
-        if assign.op == AssignOp::Assign
+    for_each_top_level_expr(program, |expr| {
+        if let Expr::Assign(assign) = expr
+            && assign.op == AssignOp::Assign
             && let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left
             && contains_getters_or_setters(&assign.right)
             && let Some(root) = root_identifier(&member.obj)
@@ -371,7 +374,7 @@ fn contains_getters_or_setters(expr: &Expr) -> bool {
     }
 }
 
-/// Calls `f` for every assignment that executes during module evaluation.
+/// Calls `f` for every expression that executes during module evaluation.
 ///
 /// This descends through all expressions — conditionals, logical/binary
 /// operators, sequences, assignment chains, call arguments, etc. — so an
@@ -379,13 +382,13 @@ fn contains_getters_or_setters(expr: &Expr) -> bool {
 /// still seen. It does *not* descend into function/method bodies: those don't
 /// run at module-evaluation time (calling such a function would itself be a side
 /// effect), so assignments inside them are irrelevant here.
-fn for_each_top_level_assign(program: &Program, f: impl FnMut(&AssignExpr)) {
+fn for_each_top_level_expr(program: &Program, f: impl FnMut(&Expr)) {
     struct Collector<F> {
         f: F,
     }
-    impl<F: FnMut(&AssignExpr)> Visit for Collector<F> {
+    impl<F: FnMut(&Expr)> Visit for Collector<F> {
         noop_visit_type!();
-        fn visit_assign_expr(&mut self, n: &AssignExpr) {
+        fn visit_expr(&mut self, n: &Expr) {
             (self.f)(n);
             n.visit_children_with(self);
         }
@@ -404,8 +407,9 @@ fn for_each_top_level_assign(program: &Program, f: impl FnMut(&AssignExpr)) {
 /// `module.exports` have side effects.
 fn module_exports_is_tainted(program: &Program, unresolved_mark: Mark) -> bool {
     let mut tainted = false;
-    for_each_top_level_assign(program, |assign| {
-        if assign.op == AssignOp::Assign
+    for_each_top_level_expr(program, |expr| {
+        if let Expr::Assign(assign) = expr
+            && assign.op == AssignOp::Assign
             && let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left
             && is_module_dot_exports(member, unresolved_mark)
             && !is_object_or_array_literal(&assign.right)
@@ -417,7 +421,7 @@ fn module_exports_is_tainted(program: &Program, unresolved_mark: Mark) -> bool {
 }
 
 /// Whether any value assigned to the module's CommonJS exports carries a getter
-/// or setter.
+/// or setter, or an `Object.defineProperty` on them installs one.
 ///
 /// Attaching an accessor to the exports object makes later member access (read or
 /// write) potentially effectful — a subsequent `module.exports.foo = 1` could
@@ -425,14 +429,44 @@ fn module_exports_is_tainted(program: &Program, unresolved_mark: Mark) -> bool {
 /// longer be treated as plain data assignments.
 fn module_exports_has_accessor(program: &Program, unresolved_mark: Mark) -> bool {
     let mut found = false;
-    for_each_top_level_assign(program, |assign| {
-        if assign.op == AssignOp::Assign
-            && let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left
-            && is_cjs_export_member(member, unresolved_mark)
-            && contains_getters_or_setters(&assign.right)
-        {
-            found = true;
+    for_each_top_level_expr(program, |expr| match expr {
+        Expr::Assign(assign) => {
+            if assign.op == AssignOp::Assign
+                && let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left
+                && is_cjs_export_member(member, unresolved_mark)
+                && contains_getters_or_setters(&assign.right)
+            {
+                found = true;
+            }
         }
+        // look for `Object.defineProperty(exports, …)`
+        Expr::Call(call) => {
+            let is_accessor_key = |key: &PropName| {
+                // A `get`/`set` in the descriptor installs an accessor, and a spread or a
+                // computed key could carry one.
+                matches!(key, PropName::Computed(_))
+                    || prop_name_eq(key, "get")
+                    || prop_name_eq(key, "set")
+            };
+            if let Some((_, descriptor)) = as_exports_define_property(call, unresolved_mark)
+                && descriptor.props.iter().any(|prop| match prop {
+                    PropOrSpread::Prop(prop) => match &**prop {
+                        // The attached value carrying one matters too: a write
+                        // through it (`exports.foo.a = 1`) could invoke a setter.
+                        Prop::KeyValue(kv) => {
+                            is_accessor_key(&kv.key) || contains_getters_or_setters(&kv.value)
+                        }
+                        Prop::Method(method) => is_accessor_key(&method.key),
+                        Prop::Shorthand(ident) => matches!(ident.sym.as_ref(), "get" | "set"),
+                        _ => false,
+                    },
+                    PropOrSpread::Spread(_) => true,
+                })
+            {
+                found = true;
+            }
+        }
+        _ => {}
     });
     found
 }
@@ -592,6 +626,26 @@ impl<'a> SideEffectVisitor<'a> {
         self.safe_assignment_constant_ids.contains(&root.to_id())
     }
 
+    /// Whether `call` is an `Object.defineProperty(exports, …)` touching only this
+    /// module's own exports — equivalent to `exports.<x> = …`.
+    fn define_property_target_is_pure(&self, call: &CallExpr) -> bool {
+        let Some((_, descriptor)) = as_exports_define_property(call, self.unresolved_mark) else {
+            return false;
+        };
+        let [target, ..] = &call.args[..] else {
+            return false;
+        };
+        if self.module_exports_tainted
+            && is_module_exports_chain(&target.expr, self.unresolved_mark)
+        {
+            return false;
+        }
+        // Bail on `{ get value() { … } }`
+        !descriptor.props.iter().any(|prop| {
+            matches!(prop, PropOrSpread::Prop(prop) if matches!(&**prop, Prop::Getter(_) | Prop::Setter(_)))
+        })
+    }
+
     /// Check if an expression is a known pure built-in function.
     ///
     /// This checks for:
@@ -955,6 +1009,8 @@ impl<'a> Visit for SideEffectVisitor<'a> {
                     // It would be weird to have a side effect in a require(...) statement, but not
                     // impossible.
                     call.args.visit_children_with(self);
+                } else if self.define_property_target_is_pure(call) {
+                    call.args.visit_children_with(self);
                 } else {
                     // Unmarked calls are considered to have side effects
                     self.mark_side_effect();
@@ -2736,6 +2792,76 @@ mod tests {
             "class C { static x = (module.exports = { set foo(v) { sideEffect() } }); } \
              module.exports.foo = 1;"
         );
+
+        no_side_effects!(
+            test_cjs_export_define_property,
+            "Object.defineProperty(exports, '__esModule', { value: true }); exports.foo = 1;"
+        );
+        no_side_effects!(
+            test_cjs_export_define_property_module_exports,
+            "Object.defineProperty(module.exports, 'foo', { value: 1 });"
+        );
+        no_side_effects!(
+            test_cjs_export_define_property_enumerable,
+            "Object.defineProperty(exports, 'foo', { value: 1, enumerable: true });"
+        );
+        no_side_effects!(
+            test_cjs_export_define_property_getter,
+            "Object.defineProperty(exports, 'foo', { get() { return 1; } });"
+        );
+        no_side_effects!(
+            test_cjs_export_define_property_getter_function,
+            "Object.defineProperty(exports, 'foo', { get: function() { return 1; } });"
+        );
+        // The shape a transpiler emits for `export { a } from './a'`.
+        module_evaluation_is_side_effect_free!(
+            test_cjs_export_define_property_reexport_barrel,
+            "Object.defineProperty(exports, '__esModule', { value: true }); var _a = \
+             require('./a'); Object.defineProperty(exports, 'a', { enumerable: true, get: \
+             function() { return _a.a; } });"
+        );
+        side_effects!(
+            test_cjs_export_define_property_setter_then_write,
+            "Object.defineProperty(exports, 'foo', { set(v) { sideEffect() } }); exports.foo = 1;"
+        );
+        side_effects!(
+            test_cjs_export_define_property_getter_then_write,
+            "Object.defineProperty(exports, 'foo', { get() { return 1; } }); exports.bar = 1;"
+        );
+        side_effects!(
+            test_cjs_export_define_property_spread_descriptor_then_write,
+            "Object.defineProperty(exports, 'foo', { ...descriptor }); exports.bar = 1;"
+        );
+        side_effects!(
+            test_cjs_export_define_property_descriptor_getter,
+            "Object.defineProperty(exports, 'foo', { get value() { return sideEffect(); } });"
+        );
+        side_effects!(
+            test_cjs_export_define_property_computed_descriptor_key_then_write,
+            "Object.defineProperty(exports, 'foo', { [key]: fn }); exports.bar = 1;"
+        );
+        side_effects!(
+            test_cjs_export_define_property_nested_setter_then_write,
+            "Object.defineProperty(exports, 'foo', { value: { set a(v) { sideEffect() } } }); \
+             exports.foo.a = 1;"
+        );
+        side_effects!(
+            test_cjs_export_define_property_computed_key,
+            "Object.defineProperty(exports, someVar, { value: 1 });"
+        );
+        side_effects!(
+            test_define_property_foreign_target,
+            "Object.defineProperty(globalThis, 'foo', { value: 1 });"
+        );
+        side_effects!(
+            test_cjs_export_define_property_impure_value,
+            "Object.defineProperty(exports, 'foo', { value: sideEffect() });"
+        );
+        side_effects!(
+            test_cjs_export_define_property_after_reexport,
+            "module.exports = require('./other'); Object.defineProperty(module.exports, 'foo', { \
+             value: 1 });"
+        );
     }
 
     mod local_variable_mutation_tests {

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/assign.js

@@ -0,0 +1,2 @@
+exports.__esModule = true
+exports.foo = 1

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/barrel-target.js

@@ -0,0 +1 @@
+exports.a = 1

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/barrel.js

@@ -0,0 +1,9 @@
+// The shape a transpiler emits for `export { a } from './barrel-target.js'`.
+Object.defineProperty(exports, '__esModule', { value: true })
+var _target = require('./barrel-target.js')
+Object.defineProperty(exports, 'a', {
+  enumerable: true,
+  get: function () {
+    return _target.a
+  },
+})

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/defineprop-foreign.js

@@ -0,0 +1,2 @@
+// A foreign target is observable, so this require is kept.
+Object.defineProperty(globalThis, 'x', { value: 1 })

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/defineprop.js

@@ -0,0 +1,2 @@
+Object.defineProperty(exports, '__esModule', { value: true })
+exports.foo = 1

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/index.js

@@ -0,0 +1,7 @@
+// A discarded require becomes `0` when its target is side-effect free.
+require('./defineprop.js')
+require('./assign.js')
+require('./barrel.js')
+require('./defineprop-foreign.js')
+
+console.log('kept')

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/options.json

@@ -0,0 +1,6 @@
+{
+  "removeUnusedImports": true,
+  "removeUnusedExports": true,
+  "cjsTreeShaking": true,
+  "treeShakingMode": "reexports-only"
+}

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/output/0nf9_cjs-remove-unused-imports_bare-require-define-property_input_index_0deqatz.js

@@ -0,0 +1,5 @@
+(globalThis["TURBOPACK"] || (globalThis["TURBOPACK"] = [])).push([
+    "output/0nf9_cjs-remove-unused-imports_bare-require-define-property_input_index_0deqatz.js",
+    {"otherChunks":["output/0p5q_snapshot_cjs-remove-unused-imports_bare-require-define-property_input_1qxhvhg._.js"],"runtimeModuleIds":["[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/index.js [test] (ecmascript)"]}
+]);
+// Dummy runtime

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/output/0p5q_snapshot_cjs-remove-unused-imports_bare-require-define-property_input_1qxhvhg._.js

@@ -0,0 +1,20 @@
+(globalThis["TURBOPACK"] || (globalThis["TURBOPACK"] = [])).push(["output/0p5q_snapshot_cjs-remove-unused-imports_bare-require-define-property_input_1qxhvhg._.js",
+"[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/defineprop-foreign.js [test] (ecmascript)", ((__turbopack_context__, module, exports) => {
+
+// A foreign target is observable, so this require is kept.
+Object.defineProperty(globalThis, 'x', {
+    value: 1
+});
+}),
+"[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/index.js [test] (ecmascript)", ((__turbopack_context__, module, exports) => {
+
+// A discarded require becomes `0` when its target is side-effect free.
+0;
+0;
+0;
+__turbopack_context__.r("[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/defineprop-foreign.js [test] (ecmascript)");
+console.log('kept');
+}),
+]);
+
+//# sourceMappingURL=0p5q_snapshot_cjs-remove-unused-imports_bare-require-define-property_input_1qxhvhg._.js.map

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/output/0p5q_snapshot_cjs-remove-unused-imports_bare-require-define-property_input_1qxhvhg._.js.map

@@ -0,0 +1,7 @@
+{
+  "version": 3,
+  "sources": [],
+  "sections": [
+    {"offset": {"line": 3, "column": 0}, "map": {"version":3,"sources":["turbopack:///[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/defineprop-foreign.js"],"sourcesContent":["// A foreign target is observable, so this require is kept.\nObject.defineProperty(globalThis, 'x', { value: 1 })\n"],"names":["Object","defineProperty","globalThis","value"],"mappings":"AAAA,2DAA2D;AAC3DA,OAAOC,cAAc,CAACC,YAAY,KAAK;IAAEC,OAAO;AAAE"}},
+    {"offset": {"line": 10, "column": 0}, "map": {"version":3,"sources":["turbopack:///[project]/turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/input/index.js"],"sourcesContent":["// A discarded require becomes `0` when its target is side-effect free.\nrequire('./defineprop.js')\nrequire('./assign.js')\nrequire('./barrel.js')\nrequire('./defineprop-foreign.js')\n\nconsole.log('kept')\n"],"names":["console","log"],"mappings":"AAAA,uEAAuE;;;;;AAMvEA,QAAQC,GAAG,CAAC"}}]
+}

turbopack/crates/turbopack-tests/tests/snapshot/cjs-remove-unused-imports/bare-require-define-property/output/154e_cjs-remove-unused-imports_bare-require-define-property_input_index_0deqatz.js.map

@@ -0,0 +1,5 @@
+{
+  "version": 3,
+  "sources": [],
+  "sections": []
+}