[async await] fix async await desugaring

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index 805b6ef066bfe5ea0cd4669cf2ad2a387b022873..9309a2a3140e362fabbc864b6e6b409fa92f18ef 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -1616,7 +1616,18 @@ Expression* Parser::RewriteReturn(Expression* return_value, int pos) {
   if (is_generator()) {
     return_value = BuildIteratorResult(return_value, true);
   } else if (is_async_function()) {
-    return_value = BuildResolvePromise(return_value, return_value->position());
+    // In an async function,
+    //    return expr;
+    // is rewritten as
+    //    return .async_return_value = expr, undefined;

[neis, 2017/02/08 10:28:41]

Why bother returning undefined? You can just do
  return .async_return_value = expr
or am I missing something?

[gsathya, 2017/02/09 00:55:44]

Yeah it doesn't matter; the returned value is thrown away anyway. I thought
returning undefined made it clear that the return value is unused.

Changed.

+    Assignment* set_async_return_var = factory()->NewAssignment(
+        Token::INIT, factory()->NewVariableProxy(AsyncReturnVariable()),
+        return_value, kNoSourcePosition);

[neis, 2017/02/08 10:28:41]

This must be ASSIGN, not INIT.

[gsathya, 2017/02/09 00:55:44]

Done.

+
+    return_value = factory()->NewBinaryOperation(
+        Token::COMMA, set_async_return_var,
+        factory()->NewUndefinedLiteral(kNoSourcePosition),
+        return_value->position());
   }
   return return_value;
 }
@@ -2977,21 +2988,37 @@ Block* Parser::BuildParameterInitializationBlock(
   return init_block;
 }
 
+Block* Parser::BuildRejectPromiseOnExceptionForParameters(Block* inner_block) {
+  return BuildRejectPromiseOnException(inner_block, true);
+}
+
 Block* Parser::BuildRejectPromiseOnException(Block* inner_block) {
+  return BuildRejectPromiseOnException(inner_block, false);
+}
+
+Block* Parser::BuildRejectPromiseOnException(Block* inner_block,
+                                             bool is_parameter_block) {
   // .promise = %AsyncFunctionPromiseCreate();
+  // .is_rejection = false;
   // try {
   //   <inner_block>
   // } catch (.catch) {
-  //   %RejectPromise(.promise, .catch);
-  //   return .promise;
+  //   .is_rejection = true;
+  //   .async_return_value = .catch;
   // } finally {
+  //   .is_rejection ? %RejectPromise(.promise, .async_return_value)
+  //       : %ResolvePromise(.promise, .async_return_value);

[caitp, 2017/02/07 23:47:17]

Given that the parameter-init-block doesn't need this, do you think it's worth
adding the try-finally outside of this function, and parameterizing whether the
catch block needs to write the field or not? This would avoid an unneeded
try/finally which emits a fair bit of byte code. Maybe we don't care about that,
but i believe we're assuming to minimize the size of interpreted code blocks.

[gsathya, 2017/02/09 00:55:44]

I agree, it is wasteful. I've split this into two separate function instead of
parameterizing since there really isn't any code sharing and it's simpler now. 

   //   %AsyncFunctionPromiseRelease(.promise);
+  //   return .promise;
   // }
   Block* result = factory()->NewBlock(nullptr, 2, true, kNoSourcePosition);
+  Variable* is_rejection_var =
+      scope()->NewTemporary(ast_value_factory()->empty_string());
 
   // .promise = %AsyncFunctionPromiseCreate();
-  Statement* set_promise;
+  // .is_rejection = false;
   {
+    Statement* set_promise;
     Expression* create_promise = factory()->NewCallRuntime(
         Context::ASYNC_FUNCTION_PROMISE_CREATE_INDEX,
         new (zone()) ZoneList<Expression*>(0, zone()), kNoSourcePosition);
@@ -3000,21 +3027,43 @@ Block* Parser::BuildRejectPromiseOnException(Block* inner_block) {
         create_promise, kNoSourcePosition);
     set_promise =
         factory()->NewExpressionStatement(assign_promise, kNoSourcePosition);
+    result->statements()->Add(set_promise, zone());
+
+    Assignment* set_is_rejection = factory()->NewAssignment(
+        Token::INIT, factory()->NewVariableProxy(is_rejection_var),
+        factory()->NewBooleanLiteral(false, kNoSourcePosition),
+        kNoSourcePosition);
+    result->statements()->Add(
+        factory()->NewExpressionStatement(set_is_rejection, kNoSourcePosition),
+        zone());
   }
-  result->statements()->Add(set_promise, zone());
 
-  // catch (.catch) { return %RejectPromise(.promise, .catch), .promise }
+  // catch (.catch) {
+  //   .is_rejection = true;
+  //   .async_return_value = .catch;
+  // }
   Scope* catch_scope = NewScope(CATCH_SCOPE);
   catch_scope->set_is_hidden();
   Variable* catch_variable =
       catch_scope->DeclareLocal(ast_value_factory()->dot_catch_string(), VAR);
   Block* catch_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
+  {
+    Assignment* set_is_rejection = factory()->NewAssignment(
+        Token::INIT, factory()->NewVariableProxy(is_rejection_var),
+        factory()->NewBooleanLiteral(true, kNoSourcePosition),
+        kNoSourcePosition);

[neis, 2017/02/08 10:28:41]

This must be ASSIGN, not INIT.

[gsathya, 2017/02/09 00:55:45]

Done.

+
+    Assignment* set_async_return_var = factory()->NewAssignment(
+        Token::INIT, factory()->NewVariableProxy(AsyncReturnVariable()),
+        factory()->NewVariableProxy(catch_variable), kNoSourcePosition);
 

[neis, 2017/02/08 10:28:41]

This must be ASSIGN, not INIT.

[gsathya, 2017/02/09 00:55:45]

Done.

-  Expression* promise_reject = BuildRejectPromise(
-      factory()->NewVariableProxy(catch_variable), kNoSourcePosition);
-  ReturnStatement* return_promise_reject =
-      factory()->NewReturnStatement(promise_reject, kNoSourcePosition);
-  catch_block->statements()->Add(return_promise_reject, zone());
+    catch_block->statements()->Add(
+        factory()->NewExpressionStatement(set_is_rejection, kNoSourcePosition),
+        zone());
+    catch_block->statements()->Add(factory()->NewExpressionStatement(
+                                       set_async_return_var, kNoSourcePosition),
+                                   zone());
+  }
 
   TryStatement* try_catch_statement =
       factory()->NewTryCatchStatementForAsyncAwait(inner_block, catch_scope,
@@ -3026,10 +3075,28 @@ Block* Parser::BuildRejectPromiseOnException(Block* inner_block) {
       factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
   outer_try_block->statements()->Add(try_catch_statement, zone());
 
-  // finally { %AsyncFunctionPromiseRelease(.promise) }
+  // finally {
+  //   .is_rejection
+  //     ? %RejectPromise(.promise, .catch)

[neis, 2017/02/08 10:28:41]

.catch should be .async_return_value

[gsathya, 2017/02/09 00:55:45]

Done.

+  //     : %ResolvePromise(.promise, .async_return_value);
+  //   %AsyncFunctionPromiseRelease(.promise);
+  //   return .promise;
+  // }
   Block* finally_block =
       factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
   {
+    //  .is_rejection
+    //    ? %RejectPromise(.promise, .catch)
+    //    : %ResolvePromise(.promise, .async_return_value);
+    Expression* resolve_or_reject_promise = factory()->NewConditional(
+        factory()->NewVariableProxy(is_rejection_var), BuildRejectPromise(),
+        BuildResolvePromise(), kNoSourcePosition);
+    finally_block->statements()->Add(
+        factory()->NewExpressionStatement(resolve_or_reject_promise,
+                                          kNoSourcePosition),
+        zone());
+
+    //  %AsyncFunctionPromiseRelease(.promise);
     ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
     args->Add(factory()->NewVariableProxy(PromiseVariable()), zone());
     Expression* call_promise_release = factory()->NewCallRuntime(
@@ -3037,6 +3104,29 @@ Block* Parser::BuildRejectPromiseOnException(Block* inner_block) {
     Statement* promise_release = factory()->NewExpressionStatement(
         call_promise_release, kNoSourcePosition);
     finally_block->statements()->Add(promise_release, zone());
+
+    // return .promise;
+    Statement* return_promise = factory()->NewReturnStatement(
+        factory()->NewVariableProxy(PromiseVariable()), kNoSourcePosition);
+
+    // If we're in a try-catch-finally block of parameters block
+    // desugaring, then we return the promise only on rejection.
+    // The desugaring changes from
+    //     return .promise;
+    // to
+    //     if (.is_rejection) {
+    //       return .promise;
+    //     }

[neis, 2017/02/08 10:28:41]

Please also mention this in the main comment at the top of the function.

[gsathya, 2017/02/09 00:55:44]

Done.

+    if (is_parameter_block) {
+      Expression* condition = factory()->NewCompareOperation(
+          Token::EQ, factory()->NewVariableProxy(is_rejection_var),
+          factory()->NewBooleanLiteral(true, kNoSourcePosition),
+          kNoSourcePosition);

[neis, 2017/02/08 10:28:41]

No need for a comparison here.

[gsathya, 2017/02/09 00:55:44]

Why not? We need to return only in the case of rejection, is there a better way
to do this?

Note -- this isn't required any more with the change caitp@ mentioned.

[neis, 2017/02/09 08:22:07]

I meant you can just do "if (is_rejection_var) ..." instead of "if
(is_rejection_var == true) ..."

+      return_promise = factory()->NewIfStatement(
+          condition, return_promise,
+          factory()->NewEmptyStatement(kNoSourcePosition), kNoSourcePosition);
+    }
+    finally_block->statements()->Add(return_promise, zone());
   }
 
   Statement* try_finally_statement = factory()->NewTryFinallyStatement(
@@ -3062,31 +3152,29 @@ Assignment* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) {
                                   kNoSourcePosition);
 }
 
-Expression* Parser::BuildResolvePromise(Expression* value, int pos) {
-  // %ResolvePromise(.promise, value), .promise
+Expression* Parser::BuildResolvePromise() {
+  // %ResolvePromise(.promise, .async_return_variable), .promise
+  int pos = kNoSourcePosition;

[adamk, 2017/02/07 21:42:01]

No need for this variable, just pass kNoSourcePosition to NewCallRuntime

[gsathya, 2017/02/09 00:55:45]

Done.

   ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
   args->Add(factory()->NewVariableProxy(PromiseVariable()), zone());
-  args->Add(value, zone());
+  args->Add(factory()->NewVariableProxy(AsyncReturnVariable()), zone());
   Expression* call_runtime =
       factory()->NewCallRuntime(Context::PROMISE_RESOLVE_INDEX, args, pos);
-  return factory()->NewBinaryOperation(
-      Token::COMMA, call_runtime,
-      factory()->NewVariableProxy(PromiseVariable()), pos);
+  return call_runtime;
 }
 
-Expression* Parser::BuildRejectPromise(Expression* value, int pos) {
-  // %promise_internal_reject(.promise, value, false), .promise
+Expression* Parser::BuildRejectPromise() {
+  // %promise_internal_reject(.promise, .async_return_variable, false)
   // Disables the additional debug event for the rejection since a debug event
   // already happened for the exception that got us here.
+  int pos = kNoSourcePosition;

[adamk, 2017/02/07 21:42:01]

Same here as above.

[gsathya, 2017/02/09 00:55:45]

Done.

   ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone());
   args->Add(factory()->NewVariableProxy(PromiseVariable()), zone());
-  args->Add(value, zone());
+  args->Add(factory()->NewVariableProxy(AsyncReturnVariable()), zone());
   args->Add(factory()->NewBooleanLiteral(false, pos), zone());
   Expression* call_runtime = factory()->NewCallRuntime(
       Context::PROMISE_INTERNAL_REJECT_INDEX, args, pos);
-  return factory()->NewBinaryOperation(
-      Token::COMMA, call_runtime,
-      factory()->NewVariableProxy(PromiseVariable()), pos);
+  return call_runtime;
 }
 
 Variable* Parser::PromiseVariable() {
@@ -3101,6 +3189,18 @@ Variable* Parser::PromiseVariable() {
   return promise;
 }
 
+Variable* Parser::AsyncReturnVariable() {
+  // Based on the various compilation paths, there are many different code
+  // paths which may be the first to access the Promise temporary. Whichever

[neis, 2017/02/08 10:28:41]

s/Promise/something else/

[gsathya, 2017/02/09 00:55:44]

Done.

+  // comes first should create it and stash it in the FunctionState.
+  Variable* async_return = function_state_->async_return_variable();
+  if (async_return == nullptr) {
+    async_return = scope()->NewTemporary(ast_value_factory()->empty_string());
+    function_state_->set_async_return_variable(async_return);
+  }
+  return async_return;
+}
+
 Expression* Parser::BuildInitialYield(int pos, FunctionKind kind) {
   Assignment* assignment = BuildCreateJSGeneratorObject(pos, kind);
   VariableProxy* generator =
@@ -3782,14 +3882,16 @@ void Parser::RewriteAsyncFunctionBody(ZoneList<Statement*>* body, Block* block,
   //   .generator_object = %CreateJSGeneratorObject();
   //   BuildRejectPromiseOnException({
   //     ... block ...
-  //     return %ResolvePromise(.promise, expr), .promise;
+  //     .async_return_var = expr;
   //   })
   // }
 
-  return_value = BuildResolvePromise(return_value, return_value->position());
-  block->statements()->Add(
-      factory()->NewReturnStatement(return_value, return_value->position()),
-      zone());
+  Assignment* set_async_return_var = factory()->NewAssignment(
+      Token::INIT, factory()->NewVariableProxy(AsyncReturnVariable()),
+      return_value, kNoSourcePosition);

[neis, 2017/02/08 10:28:41]

This must be ASSIGN, not INIT.

[gsathya, 2017/02/09 00:55:45]

Done.

+  block->statements()->Add(factory()->NewExpressionStatement(
+                               set_async_return_var, kNoSourcePosition),
+                           zone());
   block = BuildRejectPromiseOnException(block);
   body->Add(block, zone());
 }