[async await] fix async await desugaring

src/parsing/parser.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/parsing/parser.h"
 
 #include <memory>
 
 #include "src/api.h"
 #include "src/ast/ast-expression-rewriter.h"
@@ skipping 1598 matching lines @@
         Token::EQ_STRICT, assign,
         factory()->NewUndefinedLiteral(kNoSourcePosition), pos);
 
     // is_undefined ? this : is_object_conditional
     return_value = factory()->NewConditional(is_undefined, ThisExpression(pos),
                                              is_object_conditional, 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;
 }
 
 Expression* Parser::RewriteDoExpression(Block* body, int pos, bool* ok) {
   Variable* result = NewTemporary(ast_value_factory()->dot_result_string());
   DoExpression* expr = factory()->NewDoExpression(body, result, pos);
   if (!Rewriter::Rewrite(this, GetClosureScope(), expr, ast_value_factory())) {
     *ok = false;
     return nullptr;
@@ skipping 1340 matching lines @@
       if (param_scope != nullptr) {
         CheckConflictingVarDeclarations(param_scope, CHECK_OK);
       }
       init_block->statements()->Add(param_block, zone());
     }
     ++index;
   }
   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);
+  //   .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;
-  // } finally {
-  //   %AsyncFunctionPromiseRelease(.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);
     Assignment* assign_promise = factory()->NewAssignment(
         Token::INIT, factory()->NewVariableProxy(PromiseVariable()),
         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.

 
-  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());
+    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.

+    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,
                                                    catch_variable, catch_block,
                                                    kNoSourcePosition);
 
   // There is no TryCatchFinally node, so wrap it in an outer try/finally
   Block* outer_try_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(
         Context::ASYNC_FUNCTION_PROMISE_RELEASE_INDEX, args, kNoSourcePosition);
     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(
       outer_try_block, finally_block, kNoSourcePosition);
 
   result->statements()->Add(try_finally_statement, zone());
   return result;
 }
 
 Assignment* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) {
   // .generator = %CreateJSGeneratorObject(...);
   DCHECK_NOT_NULL(function_state_->generator_object_variable());
   ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
   args->Add(factory()->NewThisFunction(pos), zone());
   args->Add(IsArrowFunction(kind) ? GetLiteralUndefined(pos)
                                   : ThisExpression(kNoSourcePosition),
             zone());
   Expression* allocation =
       factory()->NewCallRuntime(Runtime::kCreateJSGeneratorObject, args, pos);
   VariableProxy* proxy =
       factory()->NewVariableProxy(function_state_->generator_object_variable());
   return factory()->NewAssignment(Token::INIT, proxy, allocation,
                                   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() {
   // Based on the various compilation paths, there are many different code
   // paths which may be the first to access the Promise temporary. Whichever
   // comes first should create it and stash it in the FunctionState.
   Variable* promise = function_state_->promise_variable();
   if (function_state_->promise_variable() == nullptr) {
     promise = scope()->NewTemporary(ast_value_factory()->empty_string());
     function_state_->set_promise_variable(promise);
   }
   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 =
       factory()->NewVariableProxy(function_state_->generator_object_variable());
   // The position of the yield is important for reporting the exception
   // caused by calling the .throw method on a generator suspended at the
   // initial yield (i.e. right after generator instantiation).
   return factory()->NewYield(generator, assignment, scope()->start_position(),
                              Yield::kOnExceptionThrow);
 }
@@ skipping 661 matching lines @@
             zone());
 }
 
 // This method completes the desugaring of the body of async_function.
 void Parser::RewriteAsyncFunctionBody(ZoneList<Statement*>* body, Block* block,
                                       Expression* return_value, bool* ok) {
   // function async_function() {
   //   .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());
 }
 
 Expression* Parser::RewriteAwaitExpression(Expression* value, int await_pos) {
   // yield do {
   //   tmp = <operand>;
   //   %AsyncFunctionAwait(.generator_object, tmp, .promise);
   //   .promise
   // }
@@ skipping 1223 matching lines @@
 
   return final_loop;
 }
 
 #undef CHECK_OK
 #undef CHECK_OK_VOID
 #undef CHECK_FAILED
 
 }  // namespace internal
 }  // namespace v8