fix #956: preserve the value of object rest assign

CHANGELOG.md

@@ -18,6 +18,24 @@
 
     This release causes object spread (e.g. `a = {...b}`) and object rest (e.g. `{...a} = b`) to always be lowered to a manual implementation instead of using native syntax when the `--target=` parameter includes a V8-based JavaScript runtime such as `chrome`, `edge`, or `node`. It turns out this feature is implemented inefficiently in V8 and copying properties over to a new object is around a 2x performance improvement. In addition, doing this manually instead of using the native implementation generates a lot less work for the garbage collector. You can see [V8 bug 11536](https://bugs.chromium.org/p/v8/issues/detail?id=11536) for details. If the V8 performance bug is eventually fixed, the translation of this syntax will be disabled again for V8-based targets containing the bug fix.
 
+* Fix object rest return value ([#956](https://github.com/evanw/esbuild/issues/956))
+
+    This release fixes a bug where the value of an object rest assignment was incorrect if the object rest assignment was lowered:
+
+    ```js
+    // This code was affected
+    let x, y
+    console.log({x, ...y} = {x: 1, y: 2})
+    ```
+
+    Previously this code would incorrectly print `{y: 2}` (the value assigned to `y`) when the object rest expression was lowered (i.e. with `--target=es2017` or below). Now this code will correctly print `{x: 1, y: 2}` instead. This bug did not affect code that did not rely on the return value of the assignment expression, such as this code:
+
+    ```js
+    // This code was not affected
+    let x, y
+    ({x, ...y} = {x: 1, y: 2})
+    ```
+
 ## 0.9.0
 
 **This release contains backwards-incompatible changes.** Since esbuild is before version 1.0.0, these changes have been released as a new minor version to reflect this (as [recommended by npm](https://docs.npmjs.com/cli/v6/using-npm/semver/)). You should either be pinning the exact version of `esbuild` in your `package.json` file or be using a version range syntax that only accepts patch upgrades such as `^0.8.0`. See the documentation about [semver](https://docs.npmjs.com/cli/v6/using-npm/semver/) for more information.

internal/bundler/bundler_lower_test.go

@@ -1153,6 +1153,13 @@ func TestTSLowerObjectRest2017NoBundle(t *testing.T) {
 
 				({ ...assign } = {});
 				({ obj_method({ ...x }) {} });
+
+				// Check for used return values
+				({ ...x } = x);
+				for ({ ...x } = x; 0; ) ;
+				console.log({ ...x } = x);
+				console.log({ x, ...xx } = { x });
+				console.log({ x: { ...xx } } = { x });
 			`,
 		},
 		entryPaths: []string{"/entry.ts"},
@@ -1194,6 +1201,13 @@ func TestTSLowerObjectRest2018NoBundle(t *testing.T) {
 
 				({ ...assign } = {});
 				({ obj_method({ ...x }) {} });
+
+				// Check for used return values
+				({ ...x } = x);
+				for ({ ...x } = x; 0; ) ;
+				console.log({ ...x } = x);
+				console.log({ x, ...xx } = { x });
+				console.log({ x: { ...xx } } = { x });
 			`,
 		},
 		entryPaths: []string{"/entry.ts"},

internal/bundler/snapshots/snapshots_lower.txt

@@ -915,7 +915,7 @@ Foo.s_foo = 123;
 ================================================================================
 TestTSLowerObjectRest2017NoBundle
 ---------- /out.js ----------
-var _o, _p;
+var _o, _p, _r, _s, _t, _u, _v;
 const local_const = __rest({}, []);
 let local_let = __rest({}, []);
 var local_var = __rest({}, []);
@@ -987,6 +987,12 @@ assign = __rest({}, []);
 ({obj_method(_q) {
   var x2 = __rest(_q, []);
 }});
+x = __rest(x, []);
+for (x = __rest(x, []); 0; )
+  ;
+console.log((x = __rest(_r = x, []), _r));
+console.log((_t = _s = {x}, {x} = _t, xx = __rest(_t, ["x"]), _s));
+console.log(({x: _v} = _u = {x}, xx = __rest(_v, []), _u));
 
 ================================================================================
 TestTSLowerObjectRest2018NoBundle
@@ -1042,6 +1048,12 @@ for ({...x} = {}; x; x = null) {
 ({...assign} = {});
 ({obj_method({...x2}) {
 }});
+({...x} = x);
+for ({...x} = x; 0; )
+  ;
+console.log({...x} = x);
+console.log({x, ...xx} = {x});
+console.log({x: {...xx}} = {x});
 
 ================================================================================
 TestTSLowerPrivateFieldAndMethodAvoidNameCollision2015

internal/js_parser/js_parser.go

@@ -136,6 +136,7 @@ type parser struct {
 	// The visit pass binds identifiers to declared symbols, does constant
 	// folding, substitutes compile-time variable definitions, and lowers certain
 	// syntactic constructs as appropriate.
+	stmtExprValue     js_ast.E
 	callTarget        js_ast.E
 	deleteTarget      js_ast.E
 	loopBody          js_ast.S
@@ -7435,6 +7436,7 @@ func (p *parser) visitForLoopInit(stmt js_ast.Stmt, isInOrOf bool) js_ast.Stmt {
 		if isInOrOf {
 			assignTarget = js_ast.AssignTargetReplace
 		}
+		p.stmtExprValue = s.Value.Data
 		s.Value, _ = p.visitExprInOut(s.Value, exprIn{assignTarget: assignTarget})
 
 	case *js_ast.SLocal:
@@ -8207,7 +8209,7 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			for _, decl := range s.Decls {
 				if decl.Value != nil {
 					target := p.convertBindingToExpr(decl.Binding, wrapIdentifier)
-					if result, ok := p.lowerObjectRestInAssign(target, *decl.Value); ok {
+					if result, ok := p.lowerObjectRestInAssign(target, *decl.Value, objRestReturnValueIsUnused); ok {
 						target = result
 					} else {
 						target = js_ast.Assign(target, *decl.Value)
@@ -8232,6 +8234,7 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 		}
 
 	case *js_ast.SExpr:
+		p.stmtExprValue = s.Value.Data
 		s.Value = p.visitExpr(s.Value)
 
 		// Trim expressions without side effects
@@ -10024,6 +10027,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 
 	case *js_ast.EBinary:
 		isCallTarget := e == p.callTarget
+		isStmtExpr := e == p.stmtExprValue
 		wasAnonymousNamedExpr := p.isAnonymousNamedExpr(e.Right)
 		e.Left, _ = p.visitExprInOut(e.Left, exprIn{assignTarget: e.Op.BinaryAssignTarget()})
 
@@ -10400,7 +10404,11 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			// binding patterns, so only do this if we're not ourselves the target of
 			// an assignment. Example: "[a = b] = c"
 			if in.assignTarget == js_ast.AssignTargetNone {
-				if result, ok := p.lowerObjectRestInAssign(e.Left, e.Right); ok {
+				mode := objRestMustReturnInitExpr
+				if isStmtExpr {
+					mode = objRestReturnValueIsUnused
+				}
+				if result, ok := p.lowerObjectRestInAssign(e.Left, e.Right, mode); ok {
 					return result, exprOut{}
 				}
 			}

internal/js_parser/js_parser_lower.go

@@ -1151,7 +1151,7 @@ func (p *parser) lowerObjectRestInForLoopInit(init js_ast.Stmt, body *js_ast.Stm
 		// "for ({...x} of y) {}"
 		if exprHasObjectRest(s.Value) {
 			ref := p.generateTempRef(tempRefNeedsDeclare, "")
-			if expr, ok := p.lowerObjectRestInAssign(s.Value, js_ast.Expr{Loc: init.Loc, Data: &js_ast.EIdentifier{Ref: ref}}); ok {
+			if expr, ok := p.lowerObjectRestInAssign(s.Value, js_ast.Expr{Loc: init.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, objRestReturnValueIsUnused); ok {
 				s.Value.Data = &js_ast.EIdentifier{Ref: ref}
 				bodyPrefixStmt = js_ast.Stmt{Loc: expr.Loc, Data: &js_ast.SExpr{Value: expr}}
 			}
@@ -1199,14 +1199,24 @@ func (p *parser) lowerObjectRestInCatchBinding(catch *js_ast.Catch) {
 	}
 }
 
-func (p *parser) lowerObjectRestInAssign(rootExpr js_ast.Expr, rootInit js_ast.Expr) (js_ast.Expr, bool) {
+type objRestMode uint8
+
+const (
+	objRestReturnValueIsUnused objRestMode = iota
+	objRestMustReturnInitExpr
+)
+
+func (p *parser) lowerObjectRestInAssign(rootExpr js_ast.Expr, rootInit js_ast.Expr, mode objRestMode) (js_ast.Expr, bool) {
 	var expr js_ast.Expr
 
 	assign := func(left js_ast.Expr, right js_ast.Expr) {
 		expr = maybeJoinWithComma(expr, js_ast.Assign(left, right))
 	}
 
-	if p.lowerObjectRestHelper(rootExpr, rootInit, assign, tempRefNeedsDeclare) {
+	if initWrapFunc, ok := p.lowerObjectRestHelper(rootExpr, rootInit, assign, tempRefNeedsDeclare, mode); ok {
+		if initWrapFunc != nil {
+			expr = initWrapFunc(expr)
+		}
 		return expr, true
 	}
 
@@ -1222,7 +1232,7 @@ func (p *parser) lowerObjectRestToDecls(rootExpr js_ast.Expr, rootInit js_ast.Ex
 		decls = append(decls, js_ast.Decl{Binding: binding, Value: &right})
 	}
 
-	if p.lowerObjectRestHelper(rootExpr, rootInit, assign, tempRefNoDeclare) {
+	if _, ok := p.lowerObjectRestHelper(rootExpr, rootInit, assign, tempRefNoDeclare, objRestReturnValueIsUnused); ok {
 		return decls, true
 	}
 
@@ -1234,16 +1244,17 @@ func (p *parser) lowerObjectRestHelper(
 	rootInit js_ast.Expr,
 	assign func(js_ast.Expr, js_ast.Expr),
 	declare generateTempRefArg,
-) bool {
+	mode objRestMode,
+) (wrapFunc func(js_ast.Expr) js_ast.Expr, ok bool) {
 	if !p.options.unsupportedJSFeatures.Has(compat.ObjectRestSpread) {
-		return false
+		return nil, false
 	}
 
 	// Check if this could possibly contain an object rest binding
 	switch rootExpr.Data.(type) {
 	case *js_ast.EArray, *js_ast.EObject:
 	default:
-		return false
+		return nil, false
 	}
 
 	// Scan for object rest bindings and initalize rest binding containment
@@ -1276,7 +1287,7 @@ func (p *parser) lowerObjectRestHelper(
 	}
 	findRestBindings(rootExpr)
 	if len(containsRestBinding) == 0 {
-		return false
+		return nil, false
 	}
 
 	// If there is at least one rest binding, lower the whole expression
@@ -1474,8 +1485,34 @@ func (p *parser) lowerObjectRestHelper(
 		assign(expr, init)
 	}
 
+	// Capture and return the value of the initializer if this is an assignment
+	// expression and the return value is used:
+	//
+	//   // Input:
+	//   console.log({...x} = x);
+	//
+	//   // Output:
+	//   var _a;
+	//   console.log((x = __rest(_a = x, []), _a));
+	//
+	// This isn't necessary if the return value is unused:
+	//
+	//   // Input:
+	//   ({...x} = x);
+	//
+	//   // Output:
+	//   x = __rest(x, []);
+	//
+	if mode == objRestMustReturnInitExpr {
+		initFunc, initWrapFunc := p.captureValueWithPossibleSideEffects(rootInit.Loc, 2, rootInit, valueCouldBeMutated)
+		rootInit = initFunc()
+		wrapFunc = func(expr js_ast.Expr) js_ast.Expr {
+			return initWrapFunc(js_ast.JoinWithComma(expr, initFunc()))
+		}
+	}
+
 	visit(rootExpr, rootInit, nil)
-	return true
+	return wrapFunc, true
 }
 
 // Save a copy of the key for the call to "__rest" later on. Certain

scripts/end-to-end-tests.js

@@ -190,6 +190,18 @@
     }),
   )
 
+  // Check object rest lowering
+  // https://github.com/evanw/esbuild/issues/956
+  tests.push(
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      'in.js': `
+        let v, o = {b: 3, c: 5}, e = ({b: v, ...o} = o);
+        console.log(JSON.stringify([o !== e, o, e, v]));
+        if (o === e || o.b !== void 0 || o.c !== 5 || e.b !== 3 || e.c !== 5 || v !== 3) throw 'fail'
+      `,
+    }),
+  )
+
   let simpleCyclicImportTestCase542 = {
     'in.js': `
       import {Test} from './lib';