Fix broken edge case of async function lowering #390
Conversation
| @@ -268,35 +268,26 @@ func (p *parser) lowerFunction( | |||
| *args = append(*args, js_ast.Arg{Binding: js_ast.Binding{Loc: arg.Binding.Loc, Data: &js_ast.BIdentifier{Ref: argRef}}}) | |||
| } | |||
|
|
|||
| // Forward all arguments from the outer function to the inner function | |||
| if p.fnOnlyDataVisit.argumentsRef != nil { | |||
There was a problem hiding this comment.
The problem seems to be argumentsRef is non-nil if the async arrow function is nested inside a regular function. This code path only triggers when the parameters have a default value (and hence couldThrowErrors = true), so both conditions are required for this bug.
| @@ -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) | |||
There was a problem hiding this comment.
I suppose adding something like &e.isArrow might be more readable. But I figured omitting such a property on Fn was intentional?
There was a problem hiding this comment.
Yes, that's correct. This change sounds good.
| { | ||
| 'in.js': ` | ||
| function nonArrowWrapper() { | ||
| return async (x, paramWithDefault = {}) => { |
There was a problem hiding this comment.
Some expressions can be statically determined to never throw. Of these, it might be worth handling the subset that are both fairly common and trivial to identify (e.g. false, true, etc.)
Fixes #388 and adds regression test.
Also adds an optimization to skip argument forwarding altogether when it is trivial to determine that all default parameter expressions cannot throw.