diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c43c012647..90c2bc1d9d4 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/internal/bundler/bundler_lower_test.go b/internal/bundler/bundler_lower_test.go index f00110da140..d26d3d43719 100644 --- a/internal/bundler/bundler_lower_test.go +++ b/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"}, diff --git a/internal/bundler/snapshots/snapshots_lower.txt b/internal/bundler/snapshots/snapshots_lower.txt index f6650cfa3ed..2ef6fc4e293 100644 --- a/internal/bundler/snapshots/snapshots_lower.txt +++ b/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 diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 802af06174b..c7f8068322e 100644 --- a/internal/js_parser/js_parser.go +++ b/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{} } } diff --git a/internal/js_parser/js_parser_lower.go b/internal/js_parser/js_parser_lower.go index 71bd7cab5c6..e26808da76d 100644 --- a/internal/js_parser/js_parser_lower.go +++ b/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 diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 5f3d59a274b..92ed6330991 100644 --- a/scripts/end-to-end-tests.js +++ b/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';