From d121b389256b50b0ebc472a21c0679e3ca9a41d7 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sun, 6 Sep 2020 21:38:47 -0700 Subject: [PATCH] fix subtle issue with "async" transform --- CHANGELOG.md | 13 +++ .../bundler/snapshots/snapshots_lower.txt | 14 +-- internal/parser/parser.go | 4 +- internal/parser/parser_lower.go | 99 ++++++++++++++++--- scripts/end-to-end-tests.js | 90 +++++++++++++++++ 5 files changed, 200 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41a1d5beebe..f283f9e3146 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/bundler/snapshots/snapshots_lower.txt b/internal/bundler/snapshots/snapshots_lower.txt index 031bdb8b555..02e7a5a1e53 100644 --- a/internal/bundler/snapshots/snapshots_lower.txt +++ b/internal/bundler/snapshots/snapshots_lower.txt @@ -27,7 +27,7 @@ function foo(bar) { } class Foo { foo() { - return __async(this, [], function* () { + return __async(this, null, function* () { }); } } @@ -35,18 +35,18 @@ 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 { diff --git a/internal/parser/parser.go b/internal/parser/parser.go index e90b1528fbc..d637ca6c43a 100644 --- a/internal/parser/parser.go +++ b/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 diff --git a/internal/parser/parser_lower.go b/internal/parser/parser_lower.go index a337aebca78..e1d6b7aaa4e 100644 --- a/internal/parser/parser_lower.go +++ b/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}}} } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 876eb348794..106e2f2f877 100644 --- a/scripts/end-to-end-tests.js +++ b/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