do not use Object.assign for object spread (#1018)

CHANGELOG.md

@@ -2,6 +2,21 @@
 
 ## Unreleased
 
+* Fix an edge case with the object spread transform ([#1017](https://github.com/evanw/esbuild/issues/1017))
+
+    This release fixes esbuild's object spread transform in cases where property assignment could be different than property definition. For example:
+
+    ```js
+    console.log({
+      get x() {},
+      ...{x: 1},
+    })
+    ```
+
+    This should print `{x: 1}` but transforming this through esbuild with `--target=es6` causes the resulting code to throw an error. The problem is that esbuild currently transforms this code to a call to `Object.assign` and that uses property assignment semantics, which causes the assignment to throw (since you can't assign to a getter-only property).
+
+    With this release, esbuild will now transform this into code that manually loops over the properties and copies them over one-by-one using `Object.defineProperty` instead. This uses property definition semantics which better matches the specification.
+
 * Fix a TypeScript parsing edge case with arrow function return types ([#1016](https://github.com/evanw/esbuild/issues/1016))
 
     This release fixes the following TypeScript parsing edge case:

internal/bundler/snapshots/snapshots_splitting.txt

@@ -204,7 +204,7 @@ TestSplittingDynamicAndNotDynamicCommonJSIntoES6
 ---------- /out/entry.js ----------
 import {
   require_foo
-} from "./chunk-D5KVJFGV.js";
+} from "./chunk-HRZ2VI6F.js";
 
 // entry.js
 var import_foo = __toModule(require_foo());
@@ -213,10 +213,10 @@ import("./foo.js").then(({default: {bar: b}}) => console.log(import_foo.bar, b))
 ---------- /out/foo.js ----------
 import {
   require_foo
-} from "./chunk-D5KVJFGV.js";
+} from "./chunk-HRZ2VI6F.js";
 export default require_foo();
 
----------- /out/chunk-D5KVJFGV.js ----------
+---------- /out/chunk-HRZ2VI6F.js ----------
 // foo.js
 var require_foo = __commonJS((exports) => {
   exports.bar = 123;
@@ -310,15 +310,15 @@ TestSplittingHybridCJSAndESMIssue617
 ---------- /out/a.js ----------
 import {
   require_a
-} from "./chunk-OAH3NS3J.js";
+} from "./chunk-IXXZP4MD.js";
 export default require_a();
 
 ---------- /out/b.js ----------
 import {
   __defProp,
   __markAsModule,
   require_a
-} from "./chunk-OAH3NS3J.js";
+} from "./chunk-IXXZP4MD.js";
 
 // b.js
 var import_a = __toModule(require_a());
@@ -327,7 +327,7 @@ export {
   export_foo as foo
 };
 
----------- /out/chunk-OAH3NS3J.js ----------
+---------- /out/chunk-IXXZP4MD.js ----------
 // a.js
 var require_a = __commonJS((exports) => {
   __markAsModule(exports);
@@ -349,21 +349,21 @@ TestSplittingHybridESMAndCJSIssue617
 ---------- /out/a.js ----------
 import {
   require_a
-} from "./chunk-FG25RNWF.js";
+} from "./chunk-ZLCN2PPW.js";
 export default require_a();
 
 ---------- /out/b.js ----------
 import {
   require_a
-} from "./chunk-FG25RNWF.js";
+} from "./chunk-ZLCN2PPW.js";
 
 // b.js
 var bar = require_a();
 export {
   bar
 };
 
----------- /out/chunk-FG25RNWF.js ----------
+---------- /out/chunk-ZLCN2PPW.js ----------
 // a.js
 var require_a = __commonJS((exports) => {
   __markAsModule(exports);
@@ -492,7 +492,7 @@ TestSplittingSharedCommonJSIntoES6
 ---------- /out/a.js ----------
 import {
   require_shared
-} from "./chunk-L66EYPCY.js";
+} from "./chunk-ZJEKWK6W.js";
 
 // a.js
 var {foo} = require_shared();
@@ -501,13 +501,13 @@ console.log(foo);
 ---------- /out/b.js ----------
 import {
   require_shared
-} from "./chunk-L66EYPCY.js";
+} from "./chunk-ZJEKWK6W.js";
 
 // b.js
 var {foo} = require_shared();
 console.log(foo);
 
----------- /out/chunk-L66EYPCY.js ----------
+---------- /out/chunk-ZJEKWK6W.js ----------
 // shared.js
 var require_shared = __commonJS((exports) => {
   exports.foo = 123;

internal/js_parser/js_parser_lower.go

@@ -880,29 +880,34 @@ func (p *parser) lowerNullishCoalescing(loc logger.Loc, left js_ast.Expr, right
 }
 
 // Lower object spread for environments that don't support them. Non-spread
-// properties are grouped into object literals and then passed to __assign()
-// like this (__assign() is an alias for Object.assign()):
+// properties are grouped into object literals and then passed to "__assign"
+// like this:
 //
 //   "{a, b, ...c, d, e}" => "__assign(__assign(__assign({a, b}, c), {d, e})"
 //
 // If the object literal starts with a spread, then we pass an empty object
-// literal to __assign() to make sure we clone the object:
+// literal to "__assign" to make sure we clone the object:
 //
 //   "{...a, b}" => "__assign(__assign({}, a), {b})"
 //
 // It's not immediately obvious why we don't compile everything to a single
-// call to __assign(). After all, Object.assign() can take any number of
+// call to "Object.assign". After all, "Object.assign" can take any number of
 // arguments. The reason is to preserve the order of side effects. Consider
 // this code:
 //
-//   let a = {get x() { b = {y: 2}; return 1 }}
+//   let a = {
+//     get x() {
+//       b = {y: 2}
+//       return 1
+//     }
+//   }
 //   let b = {}
 //   let c = {...a, ...b}
 //
-// Converting the above code to "let c = __assign({}, a, b)" means "c" becomes
-// "{x: 1}" which is incorrect. Converting the above code instead to
-// "let c = __assign(__assign({}, a), b)" means "c" becomes "{x: 1, y: 2}"
-// which is correct.
+// Converting the above code to "let c = Object.assign({}, a, b)" means "c"
+// becomes "{x: 1}" which is incorrect. Converting the above code instead to
+// "let c = Object.assign(Object.assign({}, a), b)" means "c" becomes
+// "{x: 1, y: 2}" which is correct.
 func (p *parser) lowerObjectSpread(loc logger.Loc, e *js_ast.EObject) js_ast.Expr {
 	needsLowering := false
 

internal/runtime/runtime.go

@@ -33,7 +33,36 @@ func code(isES6 bool) string {
 		var __propIsEnum = Object.prototype.propertyIsEnumerable
 
 		export var __pow = Math.pow
-		export var __assign = Object.assign
+
+		var __defNormalProp = (obj, key, value) => key in obj
+			? __defProp(obj, key, {enumerable: true, configurable: true, writable: true, value})
+			: obj[key] = value
+
+		export var __assign = (a, b) => {
+			for (var prop in b ||= {})
+				if (__hasOwnProp.call(b, prop))
+					__defNormalProp(a, prop, b[prop])
+			if (__getOwnPropSymbols)
+		`
+
+	// Avoid "of" when not using ES6
+	if isES6 {
+		text += `
+				for (var prop of __getOwnPropSymbols(b)) {
+		`
+	} else {
+		text += `
+				for (var props = __getOwnPropSymbols(b), i = 0, n = props.length, prop; i < n; i++) {
+					prop = props[i]
+		`
+	}
+
+	text += `
+					if (__propIsEnum.call(b, prop))
+						__defNormalProp(a, prop, b[prop])
+				}
+			return a
+		}
 
 		// Tells importing modules that this can be considered an ES6 module
 		var __markAsModule = target => __defProp(target, '__esModule', { value: true })
@@ -144,9 +173,8 @@ func code(isES6 bool) string {
 
 		// For class members
 		export var __publicField = (obj, key, value) => {
-			if (typeof key !== 'symbol') key += ''
-			if (key in obj) return __defProp(obj, key, {enumerable: true, configurable: true, writable: true, value})
-			return obj[key] = value
+			__defNormalProp(obj, typeof key !== 'symbol' ? key + '' : key, value)
+			return value
 		}
 		var __accessCheck = (obj, member, msg) => {
 			if (!member.has(obj)) throw TypeError('Cannot ' + msg)

scripts/end-to-end-tests.js

@@ -212,6 +212,60 @@
     }),
   )
 
+  // Check object spread lowering
+  // https://github.com/evanw/esbuild/issues/1017
+  const objectAssignSemantics = `
+    var a, b, p, s = Symbol('s')
+
+    // Getter
+    a = { x: 1 }
+    b = { get x() {}, ...a }
+    if (b.x !== a.x) throw 'fail: 1'
+
+    // Symbol getter
+    a = {}
+    a[s] = 1
+    p = {}
+    Object.defineProperty(p, s, { get: () => {} })
+    b = { __proto__: p, ...a }
+    if (b[s] !== a[s]) throw 'fail: 2'
+
+    // Non-enumerable
+    a = {}
+    Object.defineProperty(a, 'x', { value: 1 })
+    b = { ...a }
+    if (b.x === a.x) throw 'fail: 3'
+
+    // Symbol non-enumerable
+    a = {}
+    Object.defineProperty(a, s, { value: 1 })
+    b = { ...a }
+    if (b[s] === a[s]) throw 'fail: 4'
+
+    // Prototype
+    a = Object.create({ x: 1 })
+    b = { ...a }
+    if (b.x === a.x) throw 'fail: 5'
+
+    // Symbol prototype
+    p = {}
+    p[s] = 1
+    a = Object.create(p)
+    b = { ...a }
+    if (b[s] === a[s]) throw 'fail: 6'
+  `
+  tests.push(
+    test(['in.js', '--outfile=node.js'], {
+      'in.js': objectAssignSemantics,
+    }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      'in.js': objectAssignSemantics,
+    }),
+    test(['in.js', '--outfile=node.js', '--target=es5'], {
+      'in.js': objectAssignSemantics,
+    }),
+  )
+
   let simpleCyclicImportTestCase542 = {
     'in.js': `
       import {Test} from './lib';