fix subtle issue with "async" transform

CHANGELOG.md

@@ -60,6 +60,19 @@
 
     Previously using `async` functions did not cause a compile error when targeting `es5` since if they are unavailable, they are rewritten to use generator functions instead. However, generator functions may also be unsupported. It is now an error to use `async` functions if generator functions are unsupported.
 
+* Fix subtle issue with transforming `async` functions when targeting `es2016` or below
+
+    The TypeScript compiler has a bug where, when the language target is set to `ES2016` or earlier, exceptions thrown during argument evaluation are incorrectly thrown immediately instead of later causing the returned promise to be rejected. Since esbuild replicates TypeScript's `async` function transformation pass, esbuild inherited this same bug. The behavior of esbuild has been changed to match the JavaScript specification.
+
+    Here's an example of code that was affected:
+
+    ```js
+    async function test(value = getDefaultValue()) {}
+    let promise = test()
+    ```
+
+    The call to `test()` here should never throw, even if `getDefaultValue()` throws an exception.
+
 ## 0.6.31
 
 * Invalid source maps are no longer an error ([#367](https://github.com/evanw/esbuild/issues/367))

internal/bundler/snapshots/snapshots_lower.txt

@@ -27,26 +27,26 @@ function foo(bar) {
 }
 class Foo {
   foo() {
-    return __async(this, [], function* () {
+    return __async(this, null, function* () {
     });
   }
 }
 export default [
   foo,
   Foo,
   function() {
-    return __async(this, [], function* () {
+    return __async(this, null, function* () {
     });
   },
-  () => __async(this, [], function* () {
+  () => __async(this, null, function* () {
   }),
   {foo() {
-    return __async(this, [], function* () {
+    return __async(this, null, function* () {
     });
   }},
   class {
     foo() {
-      return __async(this, [], function* () {
+      return __async(this, null, function* () {
       });
     }
   },
@@ -95,7 +95,7 @@ TestLowerAsyncThis2016CommonJS
 ---------- /out.js ----------
 // /entry.js
 var require_entry = __commonJS((exports) => {
-  exports.foo = () => __async(exports, [], function* () {
+  exports.foo = () => __async(exports, null, function* () {
     return exports;
   });
 });
@@ -105,7 +105,7 @@ export default require_entry();
 TestLowerAsyncThis2016ES6
 ---------- /out.js ----------
 // /entry.js
-let foo = () => __async(void 0, [], function* () {
+let foo = () => __async(void 0, null, function* () {
   return void 0;
 });
 export {

internal/parser/parser.go

@@ -8935,7 +8935,7 @@ func (p *parser) visitExprInOut(expr ast.Expr, in exprIn) (ast.Expr, exprOut) {
 		p.pushScopeForVisitPass(ast.ScopeFunctionBody, e.Body.Loc)
 		e.Body.Stmts = p.visitStmtsAndPrependTempRefs(e.Body.Stmts)
 		p.popScope()
-		p.lowerFunction(&e.IsAsync, &e.Args, e.Body.Loc, &e.Body.Stmts, &e.PreferExpr)
+		p.lowerFunction(&e.IsAsync, &e.Args, e.Body.Loc, &e.Body.Stmts, &e.PreferExpr, &e.HasRestArg)
 		p.popScope()
 
 		if p.MangleSyntax && len(e.Body.Stmts) == 1 {
@@ -9078,7 +9078,7 @@ func (p *parser) visitFn(fn *ast.Fn, scopeLoc ast.Loc) {
 	p.pushScopeForVisitPass(ast.ScopeFunctionBody, fn.Body.Loc)
 	fn.Body.Stmts = p.visitStmtsAndPrependTempRefs(fn.Body.Stmts)
 	p.popScope()
-	p.lowerFunction(&fn.IsAsync, &fn.Args, fn.Body.Loc, &fn.Body.Stmts, nil)
+	p.lowerFunction(&fn.IsAsync, &fn.Args, fn.Body.Loc, &fn.Body.Stmts, nil, &fn.HasRestArg)
 	p.popScope()
 
 	p.fnOptsVisit = oldFnOpts

internal/parser/parser_lower.go

@@ -137,6 +137,7 @@ func (p *parser) lowerFunction(
 	bodyLoc ast.Loc,
 	bodyStmts *[]ast.Stmt,
 	preferExpr *bool,
+	hasRestArg *bool,
 ) {
 	// Lower object rest binding patterns in function arguments
 	if p.UnsupportedFeatures.Has(compat.ObjectRestSpread) {
@@ -208,23 +209,99 @@ func (p *parser) lowerFunction(
 			thisValue = ast.Expr{Loc: bodyLoc, Data: &ast.EThis{}}
 		}
 
-		// Only reference the "arguments" variable if it's actually used
-		var arguments ast.Expr
-		if p.argumentsRef != nil && p.symbolUses[*p.argumentsRef].CountEstimate > 0 {
-			arguments = ast.Expr{Loc: bodyLoc, Data: &ast.EIdentifier{Ref: *p.argumentsRef}}
+		// Move the code into a nested generator function
+		fn := ast.Fn{
+			IsGenerator: true,
+			Body:        ast.FnBody{Loc: bodyLoc, Stmts: *bodyStmts},
+		}
+		*bodyStmts = nil
+
+		// Forward the arguments to the wrapper function
+		usesArguments := p.argumentsRef != nil && p.symbolUses[*p.argumentsRef].CountEstimate > 0
+		var forwardedArgs ast.Expr
+		if len(*args) == 0 && !usesArguments {
+			// Don't allocate anything if arguments aren't needed
+			forwardedArgs = ast.Expr{Loc: bodyLoc, Data: &ast.ENull{}}
 		} else {
-			arguments = ast.Expr{Loc: bodyLoc, Data: &ast.EArray{}}
+			// Errors thrown during argument evaluation must reject the
+			// resulting promise, which needs more complex code to handle
+			couldThrowErrors := false
+			for _, arg := range *args {
+				if _, ok := arg.Binding.Data.(*ast.BIdentifier); !ok || arg.Default != nil {
+					couldThrowErrors = true
+					break
+				}
+			}
+
+			if !couldThrowErrors {
+				// Simple case: the arguments can stay on the outer function. It's
+				// worth separating out the simple case because it's the common case
+				// and it generates smaller code.
+				if usesArguments {
+					// If "arguments" is used, make sure to forward all arguments
+					// (even those past the last declared argument variable)
+					forwardedArgs = ast.Expr{Loc: bodyLoc, Data: &ast.EIdentifier{Ref: *p.argumentsRef}}
+				} else {
+					// Don't allocate anything if arguments aren't needed
+					forwardedArgs = ast.Expr{Loc: bodyLoc, Data: &ast.ENull{}}
+				}
+			} else {
+				// Complex case: the arguments must be moved to the inner function
+				fn.Args = *args
+				fn.HasRestArg = *hasRestArg
+				*args = nil
+				*hasRestArg = false
+
+				// Make sure to not change the value of the "length" property
+				for i, arg := range fn.Args {
+					if arg.Default != nil || fn.HasRestArg && i+1 == len(fn.Args) {
+						// Arguments from here on don't add to the "length"
+						break
+					}
+
+					// Generate a dummy variable
+					argRef := p.newSymbol(ast.SymbolOther, fmt.Sprintf("_%d", i))
+					p.currentScope.Generated = append(p.currentScope.Generated, argRef)
+					*args = append(*args, ast.Arg{Binding: ast.Binding{Loc: arg.Binding.Loc, Data: &ast.BIdentifier{Ref: argRef}}})
+				}
+
+				// Forward all arguments from the outer function to the inner function
+				if p.argumentsRef != nil {
+					// Normal functions can just use "arguments" to forward everything
+					forwardedArgs = ast.Expr{Loc: bodyLoc, Data: &ast.EIdentifier{Ref: *p.argumentsRef}}
+				} else {
+					// Arrow functions can't use "arguments", so we need to forward
+					// the arguments manually
+
+					// If the arrow function uses a rest argument, use one during forwarding too
+					if usesArguments || fn.HasRestArg {
+						argRef := p.newSymbol(ast.SymbolOther, fmt.Sprintf("_%d", len(*args)))
+						p.currentScope.Generated = append(p.currentScope.Generated, argRef)
+						*args = append(*args, ast.Arg{Binding: ast.Binding{Loc: bodyLoc, Data: &ast.BIdentifier{Ref: argRef}}})
+						*hasRestArg = true
+					}
+
+					// Forward all of the arguments
+					items := make([]ast.Expr, 0, len(*args))
+					for i, arg := range *args {
+						id := arg.Binding.Data.(*ast.BIdentifier)
+						item := ast.Expr{Loc: arg.Binding.Loc, Data: &ast.EIdentifier{Ref: id.Ref}}
+						if *hasRestArg && i+1 == len(*args) {
+							item.Data = &ast.ESpread{Value: item}
+						}
+						items = append(items, item)
+					}
+					forwardedArgs = ast.Expr{Loc: bodyLoc, Data: &ast.EArray{Items: items, IsSingleLine: true}}
+				}
+			}
 		}
 
-		// "async function foo() { stmts }" => "function foo() { return __async(this, arguments, function* () { stmts }) }"
+		// "async function foo(a, b) { stmts }" => "function foo(a, b) { return __async(this, null, function* () { stmts }) }"
 		*isAsync = false
 		callAsync := p.callRuntime(bodyLoc, "__async", []ast.Expr{
 			thisValue,
-			arguments,
-			{Loc: bodyLoc, Data: &ast.EFunction{Fn: ast.Fn{
-				IsGenerator: true,
-				Body:        ast.FnBody{Loc: bodyLoc, Stmts: *bodyStmts},
-			}}},
+			forwardedArgs,
+			{Loc: bodyLoc, Data: &ast.EFunction{Fn: fn}},
 		})
 		*bodyStmts = []ast.Stmt{{Loc: bodyLoc, Data: &ast.SReturn{Value: &callAsync}}}
 	}

scripts/end-to-end-tests.js

@@ -1044,6 +1044,96 @@ in.js:24:30: warning: Writing to getter-only property "#getter" will throw
         }
       `,
     }, { async: true }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      // The async transform must not change the argument count
+      'in.js': `
+        async function a(x, y) {}
+        if (a.length !== 2) throw 'fail: a'
+
+        async function b(x, y = x(), z) {}
+        if (b.length !== 1) throw 'fail: b'
+
+        async function c(x, y, ...z) {}
+        if (c.length !== 2) throw 'fail: c'
+
+        let d = async function(x, y) {}
+        if (d.length !== 2) throw 'fail: d'
+
+        let e = async function(x, y = x(), z) {}
+        if (e.length !== 1) throw 'fail: e'
+
+        let f = async function(x, y, ...z) {}
+        if (f.length !== 2) throw 'fail: f'
+
+        let g = async (x, y) => {}
+        if (g.length !== 2) throw 'fail: g'
+
+        let h = async (x, y = x(), z) => {}
+        if (h.length !== 1) throw 'fail: h'
+
+        let i = async (x, y, ...z) => {}
+        if (i.length !== 2) throw 'fail: i'
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      // Functions must be able to access arguments past the argument count using "arguments"
+      'in.js': `
+        exports.async = async () => {
+          async function a() { return arguments[2] }
+          async function b(x, y) { return arguments[2] }
+          async function c(x, y = x) { return arguments[2] }
+          let d = async function() { return arguments[2] }
+          let e = async function(x, y) { return arguments[2] }
+          let f = async function(x, y = x) { return arguments[2] }
+          for (let fn of [a, b, c, d, e, f]) {
+            if ((await fn('x', 'y', 'z')) !== 'z') throw 'fail: ' + fn
+          }
+        }
+      `,
+    }, { async: true }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      // Functions must be able to access arguments past the argument count using a rest argument
+      'in.js': `
+        exports.async = async () => {
+          async function a(...rest) { return rest[3] }
+          async function b(x, y, ...rest) { return rest[1] }
+          async function c(x, y = x, ...rest) { return rest[1] }
+          let d = async function(...rest) { return rest[3] }
+          let e = async function(x, y, ...rest) { return rest[1] }
+          let f = async function(x, y = x, ...rest) { return rest[1] }
+          let g = async (...rest) => rest[3]
+          let h = async (x, y, ...rest) => rest[1]
+          let i = async (x, y = x, ...rest) => rest[1]
+          for (let fn of [a, b, c, d, e, f, g, h, i]) {
+            if ((await fn(11, 22, 33, 44)) !== 44) throw 'fail: ' + fn
+          }
+        }
+      `,
+    }, { async: true }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      // Errors in the evaluation of async function arguments should reject the resulting promise
+      'in.js': `
+        exports.async = async () => {
+          let expected = new Error('You should never see this error')
+          let throws = () => { throw expected }
+          async function a(x, y = throws()) {}
+          async function b({ [throws()]: x }) {}
+          let c = async function (x, y = throws()) {}
+          let d = async function ({ [throws()]: x }) {}
+          let e = async (x, y = throws()) => {}
+          let f = async ({ [throws()]: x }) => {}
+          for (let fn of [a, b, c, d, e, f]) {
+            let promise = fn({})
+            try {
+              await promise
+            } catch (e) {
+              if (e === expected) continue
+            }
+            throw 'fail: ' + fn
+          }
+        }
+      `,
+    }, { async: true }),
   )
 
   // Object rest pattern tests