Desugar async/await to create the resulting Promise upfront

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index a47a0b48d27243352c9a7a189735614f8674755e..54066f4b447d0b1c4e97ebb5e93fc299be819d03 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -2635,7 +2635,7 @@ Statement* Parser::ParseReturnStatement(bool* ok) {
   if (is_generator()) {
     return_value = BuildIteratorResult(return_value, true);
   } else if (is_async_function()) {
-    return_value = BuildPromiseResolve(return_value, return_value->position());
+    return_value = BuildResolvePromise(return_value, return_value->position());
   }
 
   result = factory()->NewReturnStatement(return_value, loc.beg_pos);
@@ -3965,12 +3965,11 @@ void Parser::DesugarAsyncFunctionBody(const AstRawString* function_name,
                                       FunctionBodyType body_type,
                                       bool accept_IN, int pos, bool* ok) {
   // function async_function() {
-  //   try {
+  //   BuildRejectPromiseOnException({
   //     .generator_object = %CreateGeneratorObject();

[neis, 2016/08/24 09:48:51]

The .generator_object assignment is actually outside the try block. Can you fix
the comment?

[Dan Ehrenberg, 2016/08/24 18:03:22]

Fixed

   //     ... function body ...
-  //   } catch (e) {
-  //     return Promise.reject(e);
-  //   }
+  //     return %ResolvePromise(.promise, expr), .promise;
+  //   })
   // }
   scope->ForceContextAllocation();
   Variable* temp =
@@ -3998,11 +3997,12 @@ void Parser::DesugarAsyncFunctionBody(const AstRawString* function_name,
     RewriteNonPattern(classifier, CHECK_OK_VOID);
   }
 
-  return_value = BuildPromiseResolve(return_value, return_value->position());
+  return_value = BuildResolvePromise(return_value, return_value->position());
   inner_body->Add(
       factory()->NewReturnStatement(return_value, return_value->position()),
       zone());
-  body->Add(BuildRejectPromiseOnException(try_block), zone());
+  Statement* block = BuildRejectPromiseOnException(try_block, CHECK_OK_VOID);
+  body->Add(block, zone());
   scope->set_end_position(scanner()->location().end_pos);
 }
 
@@ -4562,9 +4562,73 @@ Block* Parser::BuildParameterInitializationBlock(
   return init_block;
 }
 
-Block* Parser::BuildRejectPromiseOnException(Block* block) {
-  // try { <block> } catch (error) { return Promise.reject(error); }
-  Block* try_block = block;
+Block* Parser::BuildRejectPromiseOnException(Block* inner_block, bool* ok) {

[neis, 2016/08/24 09:48:51]

s/inner_block/block/ to match the comment.

[Dan Ehrenberg, 2016/08/24 18:03:22]

Changed the name in the other direction

+  // var .promise = %CreatePromise();
+  // var .debug_is_active = %_DebugIsActive() !== 0;

[neis, 2016/08/24 09:48:51]

I think we don't need the !== 0 comparison.  (But why doesn't DebugIsActive
return a boolean in the first place?)

[Dan Ehrenberg, 2016/08/24 18:03:22]

Right, I was considering this but left it out as the existing Promise code did
the comparison. But I don't see the point either, so the new patch removes the
comparison.

+  // try {
+  //   if (.debug_is_active) %DebugPushPromise(.promise);

[neis, 2016/08/24 09:48:51]

Can't we move this out of the try-block?  Seems cleaner and avoids creating
another block.

[Dan Ehrenberg, 2016/08/24 18:03:22]

Done.

+  //   <block>
+  // } catch (.catch) {
+  //   %RejectPromise(.promise, .catch);
+  //   return .promise;
+  // } finally {
+  //   if (.debug_is_active) %DebugPopPromise();
+  // }
+  Block* block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);

[jgruber, 2016/08/24 09:16:36]

We could set the initial size to 3 to avoid having to grow the ZoneList.

[neis, 2016/08/24 09:48:51]

s/block/result/ for clarity (and to avoid conflict)

[Dan Ehrenberg, 2016/08/24 18:03:22]

Done, and fixed the initial size to 4.

+
+  // var .promise = %CreatePromise();
+  Statement* set_promise;
+  {
+    DeclareVariable(ast_value_factory()->dot_promise_string(), VAR,

[jgruber, 2016/08/24 09:16:36]

Since I'm not familiar with the parser, a question: Whenever I see dot_* names
used for variables in the parser, NewTemporary() is used instead of
DeclareVariable(). Is that not applicable here?

[Dan Ehrenberg, 2016/08/24 18:03:22]

I could do that, but then I'd have to store the temporary in FunctionState like
.generator_object is. I don't really see the point in going through that
bookkeeping in this case--scope analysis does just fine here.

+                    kNoSourcePosition, CHECK_OK);
+    Expression* create_promise = factory()->NewCallRuntime(
+        Context::PROMISE_CREATE_INDEX,
+        new (zone()) ZoneList<Expression*>(0, zone()), kNoSourcePosition);
+    Assignment* assign_promise = factory()->NewAssignment(
+        Token::INIT, BuildDotPromise(), create_promise, kNoSourcePosition);
+    set_promise =
+        factory()->NewExpressionStatement(assign_promise, kNoSourcePosition);
+  }
+  block->statements()->Add(set_promise, zone());
+
+  // var .debug_is_active = %_DebugIsActive() != 0;

[jgruber, 2016/08/24 09:16:36]

Nit: This differs from !== above.

[Dan Ehrenberg, 2016/08/24 18:03:22]

Removed the comparison, as it is unnecessary.

+  Statement* set_debug_is_active;
+  {
+    DeclareVariable(ast_value_factory()->dot_debug_is_active_string(), VAR,
+                    kNoSourcePosition, CHECK_OK);
+    Expression* debug_is_active_num = factory()->NewCallRuntime(
+        Runtime::kInlineDebugIsActive,
+        new (zone()) ZoneList<Expression*>(0, zone()), kNoSourcePosition);
+    Expression* debug_is_active = factory()->NewCompareOperation(
+        Token::EQ_STRICT, debug_is_active_num,
+        factory()->NewSmiLiteral(0, kNoSourcePosition), kNoSourcePosition);
+    debug_is_active = factory()->NewUnaryOperation(Token::NOT, debug_is_active,
+                                                   kNoSourcePosition);
+    Assignment* assign_debug_is_active = factory()->NewAssignment(
+        Token::INIT, BuildDebugIsActive(), debug_is_active, kNoSourcePosition);
+    set_debug_is_active = factory()->NewExpressionStatement(
+        assign_debug_is_active, kNoSourcePosition);
+  }
+  block->statements()->Add(set_debug_is_active, zone());
+
+  Block* try_block = factory()->NewBlock(nullptr, 2, true, kNoSourcePosition);
+  //   if (.debug_is_active) %DebugPushPromise(.promise);
+  Statement* conditionally_debug_push_promise;
+  {
+    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
+    args->Add(BuildDotPromise(), zone());
+    Expression* call_push_promise = factory()->NewCallRuntime(
+        Runtime::kDebugPushPromise, args, kNoSourcePosition);
+    Statement* debug_push_promise =
+        factory()->NewExpressionStatement(call_push_promise, kNoSourcePosition);
+    conditionally_debug_push_promise = factory()->NewIfStatement(
+        BuildDebugIsActive(), debug_push_promise,
+        factory()->NewEmptyStatement(kNoSourcePosition), kNoSourcePosition);
+  }
+  try_block->statements()->Add(conditionally_debug_push_promise, zone());
+  try_block->statements()->Add(inner_block, zone());
+
+  // catch (.catch) { return %RejectPromise(.promise, .catch), .promise }
   Scope* catch_scope = NewScope(CATCH_SCOPE);
   catch_scope->set_is_hidden();
   Variable* catch_variable =
@@ -4572,17 +4636,39 @@ Block* Parser::BuildRejectPromiseOnException(Block* block) {
                                 kCreatedInitialized, Variable::NORMAL);
   Block* catch_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
 
-  Expression* promise_reject = BuildPromiseReject(
+  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());
+
   TryStatement* try_catch_statement = factory()->NewTryCatchStatement(
       try_block, catch_scope, catch_variable, catch_block, kNoSourcePosition);
 
-  block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
-  block->statements()->Add(try_catch_statement, zone());
+  // 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 { if (.debug_is_active) %DebugPopPromise(); }
+  Block* finally_block =
+      factory()->NewBlock(nullptr, 1, true, kNoSourcePosition);
+  {
+    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(0, zone());
+    Expression* call_pop_promise = factory()->NewCallRuntime(
+        Runtime::kDebugPopPromise, args, kNoSourcePosition);
+    Statement* debug_pop_promise =
+        factory()->NewExpressionStatement(call_pop_promise, kNoSourcePosition);
+    Statement* conditionally_debug_pop_promise = factory()->NewIfStatement(
+        BuildDebugIsActive(), debug_pop_promise,
+        factory()->NewEmptyStatement(kNoSourcePosition), kNoSourcePosition);
+    finally_block->statements()->Add(conditionally_debug_pop_promise, zone());
+  }
+
+  Statement* try_finally_statement = factory()->NewTryFinallyStatement(
+      outer_try_block, finally_block, kNoSourcePosition);
+
+  block->statements()->Add(try_finally_statement, zone());
   return block;
 }
 
@@ -4597,18 +4683,36 @@ Expression* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) {
                                    pos);
 }
 
-Expression* Parser::BuildPromiseResolve(Expression* value, int pos) {
-  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
+Expression* Parser::BuildResolvePromise(Expression* value, int pos) {
+  // %ResolvePromise(.promise, value), .promise
+  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
+  args->Add(BuildDotPromise(), zone());
   args->Add(value, zone());
-  return factory()->NewCallRuntime(Context::PROMISE_CREATE_RESOLVED_INDEX, args,
-                                   pos);
+  Expression* call_runtime =
+      factory()->NewCallRuntime(Context::PROMISE_RESOLVE_INDEX, args, pos);
+  return factory()->NewBinaryOperation(Token::COMMA, call_runtime,
+                                       BuildDotPromise(), pos);
+}
+
+Expression* Parser::BuildRejectPromise(Expression* value, int pos) {
+  // %RejectPromise(.promise, value, true), .promise
+  // The 'true' flag disables the additional debug event for the rejection
+  // since a debug event already happened for the exception that got us here.

[neis, 2016/08/24 09:48:51]

I think you mean 'false' here, not 'true', but since it doesn't appear in the
AST anymore you don't need to mention the argument.  I'm wondering, though, if
it's really worth having RejectPromiseNoDebugEvent as a seperate definition.

[Dan Ehrenberg, 2016/08/24 18:03:22]

I made it a separate function because RejectPromise is exported to a couple
other places and I didn't want to mess with it, especially once the calling
convention switched to using 'true' to trigger an event (which pre-existing
callers did). Fixed the comment.

+  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone());

[jgruber, 2016/08/24 09:16:36]

Should this be 2? There's only two args added.

[Dan Ehrenberg, 2016/08/24 18:03:22]

Fixed

+  args->Add(BuildDotPromise(), zone());
+  args->Add(value, zone());
+  Expression* call_runtime = factory()->NewCallRuntime(
+      Context::PROMISE_REJECT_NO_DEBUG_EVENT_INDEX, args, pos);
+  return factory()->NewBinaryOperation(Token::COMMA, call_runtime,
+                                       BuildDotPromise(), pos);
 }
 
-Expression* Parser::BuildPromiseReject(Expression* value, int pos) {
-  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
-  args->Add(value, zone());
-  return factory()->NewCallRuntime(Context::PROMISE_CREATE_REJECTED_INDEX, args,
-                                   pos);
+VariableProxy* Parser::BuildDotPromise() {
+  return NewUnresolved(ast_value_factory()->dot_promise_string(), VAR);
+}
+
+VariableProxy* Parser::BuildDebugIsActive() {

[jgruber, 2016/08/24 09:16:36]

This should be BuildDotDebugIsActive for consistency.

[Dan Ehrenberg, 2016/08/24 18:03:22]

Done

+  return NewUnresolved(ast_value_factory()->dot_debug_is_active_string(), VAR);
 }
 
 ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
@@ -4734,8 +4838,9 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
                                            CHECK_OK);
     }
 
+    // TODO(littledan): Merge the two rejection blocks into one
     if (IsAsyncFunction(kind)) {
-      init_block = BuildRejectPromiseOnException(init_block);
+      init_block = BuildRejectPromiseOnException(init_block, CHECK_OK);
     }
 
     DCHECK_NOT_NULL(init_block);