Skip to content

Commit

Permalink
Fix broken edge case of async function lowering (#390)
Browse files Browse the repository at this point in the history
  • Loading branch information
rtsao authored Sep 18, 2020
1 parent b790344 commit b4d808f
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 5 deletions.
2 changes: 1 addition & 1 deletion internal/bundler/snapshots/snapshots_lower.txt
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export default [
}
},
function() {
return (_0) => __async(this, arguments, function* (bar) {
return (_0, ..._1) => __async(this, [_0, ..._1], function* (bar) {
yield bar;
return [this, arguments];
});
Expand Down
4 changes: 2 additions & 2 deletions internal/js_parser/js_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -8987,7 +8987,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
p.pushScopeForVisitPass(js_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, &e.HasRestArg)
p.lowerFunction(&e.IsAsync, &e.Args, e.Body.Loc, &e.Body.Stmts, &e.PreferExpr, &e.HasRestArg, true)
p.popScope()

if p.MangleSyntax && len(e.Body.Stmts) == 1 {
Expand Down Expand Up @@ -9133,7 +9133,7 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc) {
p.pushScopeForVisitPass(js_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, &fn.HasRestArg)
p.lowerFunction(&fn.IsAsync, &fn.Args, fn.Body.Loc, &fn.Body.Stmts, nil, &fn.HasRestArg, false)
p.popScope()

p.fnOrArrowDataVisit = oldFnOrArrowData
Expand Down
13 changes: 11 additions & 2 deletions internal/js_parser/js_parser_lower.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ func (p *parser) lowerFunction(
bodyStmts *[]js_ast.Stmt,
preferExpr *bool,
hasRestArg *bool,
isArrow bool,
) {
// Lower object rest binding patterns in function arguments
if p.UnsupportedFeatures.Has(compat.ObjectRestSpread) {
Expand Down Expand Up @@ -228,7 +229,7 @@ func (p *parser) lowerFunction(
// resulting promise, which needs more complex code to handle
couldThrowErrors := false
for _, arg := range *args {
if _, ok := arg.Binding.Data.(*js_ast.BIdentifier); !ok || arg.Default != nil {
if _, ok := arg.Binding.Data.(*js_ast.BIdentifier); !ok || (arg.Default != nil && couldPotentiallyThrow(arg.Default.Data)) {
couldThrowErrors = true
break
}
Expand Down Expand Up @@ -269,7 +270,7 @@ func (p *parser) lowerFunction(
}

// Forward all arguments from the outer function to the inner function
if p.fnOnlyDataVisit.argumentsRef != nil {
if !isArrow {
// Normal functions can just use "arguments" to forward everything
forwardedArgs = js_ast.Expr{Loc: bodyLoc, Data: &js_ast.EIdentifier{Ref: *p.fnOnlyDataVisit.argumentsRef}}
} else {
Expand Down Expand Up @@ -2083,3 +2084,11 @@ func (p *parser) maybeLowerSuperPropertyAccessInsideCall(call *js_ast.ECall) {
thisExpr := js_ast.Expr{Loc: call.Target.Loc, Data: &js_ast.EThis{}}
call.Args = append([]js_ast.Expr{thisExpr}, call.Args...)
}

func couldPotentiallyThrow(data js_ast.E) bool {
switch data.(type) {
case *js_ast.ENull, *js_ast.EUndefined, *js_ast.EBoolean, *js_ast.ENumber, *js_ast.EBigInt, *js_ast.EString, *js_ast.EFunction, *js_ast.EArrow:
return false
}
return true
}
22 changes: 22 additions & 0 deletions internal/js_parser/js_parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2655,6 +2655,28 @@ func TestLowerLogicalAssign(t *testing.T) {
expectPrintedTarget(t, 2020, "a()[b()] ||= c", "var _a, _b;\n(_a = a())[_b = b()] || (_a[_b] = c);\n")
}

func TestLowerAsyncFunctions(t *testing.T) {
// Lowered non-arrow functions with argument evaluations should merely use
// "arguments" rather than allocating a new array when forwarding arguments
expectPrintedTarget(t, 2015, "async function foo(a, b = couldThrowErrors()) {console.log(a, b);}", `function foo(_0) {
return __async(this, arguments, function* (a, b = couldThrowErrors()) {
console.log(a, b);
});
}
import {
__async
} from "<runtime>";
`)
// Skip forwarding altogether when parameter evaluation obviously cannot throw
expectPrintedTarget(t, 2015, "async (a, b = 123) => {console.log(a, b);}", `(a, b = 123) => __async(this, null, function* () {
console.log(a, b);
});
import {
__async
} from "<runtime>";
`)
}

func TestLowerClassSideEffectOrder(t *testing.T) {
// The order of computed property side effects must not change
expectPrintedTarget(t, 2015, `class Foo {
Expand Down
14 changes: 14 additions & 0 deletions scripts/end-to-end-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,20 @@ in.js:24:30: warning: Writing to getter-only property "#getter" will throw
)
`,
}, { async: true }),
test(['in.js', '--outfile=node.js', '--target=es6'],
{
'in.js': `
function nonArrowWrapper() {
return async (x, paramWithDefault = {}) => {
if (x !== 123) {
throw 'fail';
}
console.log(paramWithDefault);
};
}
exports.async = () => nonArrowWrapper()(123);
`,
}, { async: true }),
test(['in.js', '--outfile=node.js', '--target=es6'], {
'in.js': `
exports.async = async () => {
Expand Down

0 comments on commit b4d808f

Please sign in to comment.