Move generator-close on exception from the generator function to the GeneratorResume builtin.

The change also moves creation of the iterator result from the parser to the bytecode generator.

Unfortunately, async generators will stay on the old scheme (try-finally around generator body) because I am not exactly sure how they work.

Review-Url: https://codereview.chromium.org/2917263002
Cr-Commit-Position: refs/heads/master@{#45713}

src/ast/ast.h

@@ -2541,6 +2541,10 @@ class Suspend final : public Expression {
     return suspend_id() > 0 && (flags() & SuspendFlags::kAsyncGeneratorAwait) ==
                                    SuspendFlags::kAsyncGenerator;
   }
+  inline bool IsNonInitialGeneratorYield() const {
+    // Return true if is_generator() && !is_await() && yield_id() > 0
+    return suspend_id() > 0 && (flags() == SuspendFlags::kGeneratorYield);
+  }
 
   void set_expression(Expression* e) { expression_ = e; }
   void set_suspend_id(int id) { suspend_id_ = id; }

src/builtins/builtins-definitions.h

@@ -1049,7 +1049,11 @@ namespace internal {
 
 #define BUILTIN_EXCEPTION_CAUGHT_PREDICTION_LIST(V) V(PromiseHandleReject)
 
-#define BUILTIN_EXCEPTION_UNCAUGHT_PREDICTION_LIST(V) V(MapConstructor)
+#define BUILTIN_EXCEPTION_UNCAUGHT_PREDICTION_LIST(V) \
+  V(MapConstructor)                                   \
+  V(GeneratorPrototypeNext)                           \
+  V(GeneratorPrototypeReturn)                         \
+  V(GeneratorPrototypeThrow)
 
 #define IGNORE_BUILTIN(...)
 

src/builtins/builtins-generator-gen.cc

@@ -47,12 +47,32 @@ void GeneratorBuiltinsAssembler::GeneratorPrototypeResume(
   GotoIf(SmiLessThan(receiver_continuation, closed), &if_receiverisrunning);
 
   // Resume the {receiver} using our trampoline.
+  VARIABLE(var_exception, MachineRepresentation::kTagged, UndefinedConstant());
+  Label if_exception(this, Label::kDeferred), if_final_return(this);
   Node* result =
       CallStub(CodeFactory::ResumeGenerator(isolate()), context, value,
                receiver, SmiConstant(resume_mode),
                SmiConstant(static_cast<int>(SuspendFlags::kGeneratorYield)));
+  // Make sure we close the generator if there was an exception.
+  GotoIfException(result, &if_exception, &var_exception);
+
+  // If the generator is not suspended (i.e., it's state is 'closed'),
+  // wrap the return value in IteratorResult.
+  Node* result_continuation =
+      LoadObjectField(receiver, JSGeneratorObject::kContinuationOffset);
+  GotoIf(SmiEqual(result_continuation, closed), &if_final_return);
   Return(result);
 
+  Callable create_iter_result_object =
+      CodeFactory::CreateIterResultObject(isolate());
+
+  BIND(&if_final_return);
+  {
+    // Return the wrapped result.
+    Return(
+        CallStub(create_iter_result_object, context, result, TrueConstant()));
+  }
+
   BIND(&if_receiverisincompatible);
   {
     // The {receiver} is not a valid JSGeneratorObject.
@@ -65,9 +85,6 @@ void GeneratorBuiltinsAssembler::GeneratorPrototypeResume(
 
   BIND(&if_receiverisclosed);
   {
-    Callable create_iter_result_object =
-        CodeFactory::CreateIterResultObject(isolate());
-
     // The {receiver} is closed already.
     Node* result = nullptr;
     switch (resume_mode) {
@@ -91,6 +108,14 @@ void GeneratorBuiltinsAssembler::GeneratorPrototypeResume(
     CallRuntime(Runtime::kThrowGeneratorRunning, context);
     Unreachable();
   }
+
+  BIND(&if_exception);
+  {
+    StoreObjectFieldNoWriteBarrier(
+        receiver, JSGeneratorObject::kContinuationOffset, closed);
+    CallRuntime(Runtime::kReThrow, context, var_exception.value());
+    Unreachable();
+  }
 }
 
 // ES6 #sec-generator.prototype.next

src/interpreter/bytecode-generator.cc

@@ -981,13 +981,16 @@ void BytecodeGenerator::BuildGeneratorPrologue() {
 
   // This is a resume call. Restore the current context and the registers,
   // then perform state dispatch.
-  Register generator_context = register_allocator()->NewRegister();
-  builder()
-      ->CallRuntime(Runtime::kInlineGeneratorGetContext, generator_object_)
-      .PushContext(generator_context)
-      .RestoreGeneratorState(generator_object_)
-      .StoreAccumulatorInRegister(generator_state_)
-      .SwitchOnSmiNoFeedback(generator_jump_table_);
+  {
+    RegisterAllocationScope register_scope(this);
+    Register generator_context = register_allocator()->NewRegister();
+    builder()
+        ->CallRuntime(Runtime::kInlineGeneratorGetContext, generator_object_)
+        .PushContext(generator_context)
+        .RestoreGeneratorState(generator_object_)
+        .StoreAccumulatorInRegister(generator_state_)
+        .SwitchOnSmiNoFeedback(generator_jump_table_);
+  }
   // We fall through when the generator state is not in the jump table.
   // TODO(leszeks): Only generate this for debug builds.
   BuildAbort(BailoutReason::kInvalidJumpTableIndex);
@@ -2183,6 +2186,15 @@ void BytecodeGenerator::BuildReturn() {
   if (info()->literal()->feedback_vector_spec()->HasTypeProfileSlot()) {
     builder()->CollectTypeProfile(info()->literal()->return_position());
   }
+  if (IsGeneratorFunction(info()->literal()->kind())) {
+    // Mark the generator as closed if returning from a generator function.
+    RegisterAllocationScope register_scope(this);
+    Register result = register_allocator()->NewRegister();
+    builder()
+        ->StoreAccumulatorInRegister(result)
+        .CallRuntime(Runtime::kInlineGeneratorClose, generator_object_)
+        .LoadAccumulatorWithRegister(result);
+  }
   builder()->Return();
 }
 
@@ -2533,7 +2545,15 @@ void BytecodeGenerator::BuildGeneratorSuspend(Suspend* expr,
       ->LoadLiteral(Smi::FromInt(expr->suspend_id()))
       .SuspendGenerator(generator_object_, registers_to_save, expr->flags());
 
-  if (expr->IsNonInitialAsyncGeneratorYield()) {
+  if (expr->IsNonInitialGeneratorYield()) {
+    // GeneratorYield: Wrap the value into IteratorResult.
+    RegisterList args = register_allocator()->NewRegisterList(2);
+    builder()
+        ->MoveRegister(value, args[0])
+        .LoadFalse()
+        .StoreAccumulatorInRegister(args[1])
+        .CallRuntime(Runtime::kInlineCreateIterResultObject, args);
+  } else if (expr->IsNonInitialAsyncGeneratorYield()) {
     // AsyncGenerator yields (with the exception of the initial yield) delegate
     // to AsyncGeneratorResolve(), implemented via the runtime call below.
     RegisterList args = register_allocator()->NewRegisterList(3);
@@ -2606,17 +2626,11 @@ void BytecodeGenerator::BuildGeneratorResume(
     {
       // Resume with return.
       builder()->Bind(jump_table, JSGeneratorObject::kReturn);
+      builder()->LoadAccumulatorWithRegister(input);
       if (expr->is_async_generator()) {
         // Async generator methods will produce the iter result object.
-        builder()->LoadAccumulatorWithRegister(input);
         execution_control()->AsyncReturnAccumulator();
       } else {
-        RegisterList args = register_allocator()->NewRegisterList(2);
-        builder()
-            ->MoveRegister(input, args[0])
-            .LoadTrue()
-            .StoreAccumulatorInRegister(args[1])
-            .CallRuntime(Runtime::kInlineCreateIterResultObject, args);
         execution_control()->ReturnAccumulator();
       }
     }

src/parsing/parser-base.h

@@ -1388,10 +1388,6 @@ class ParserBase {
   // Convenience method which determines the type of return statement to emit
   // depending on the current function type.
   inline StatementT BuildReturnStatement(ExpressionT expr, int pos) {
-    if (is_generator() && !is_async_generator()) {
-      expr = impl()->BuildIteratorResult(expr, true);
-    }
-
     if (is_async_function()) {
       return factory()->NewAsyncReturnStatement(expr, pos);
     }
@@ -2999,12 +2995,6 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseYieldExpression(
     return impl()->RewriteYieldStar(expression, pos);
   }
 
-  if (!is_async_generator()) {
-    // Async generator yield is rewritten in Ignition, and doesn't require
-    // producing an Iterator Result.
-    expression = impl()->BuildIteratorResult(expression, false);
-  }
-
   // Hackily disambiguate o from o.next and o [Symbol.iterator]().
   // TODO(verwaest): Come up with a better solution.
   ExpressionT yield = BuildSuspend(expression, pos, Suspend::kOnExceptionThrow,
@@ -4091,7 +4081,9 @@ void ParserBase<Impl>::ParseFunctionBody(
   {
     BlockState block_state(&scope_, inner_scope);
 
-    if (IsGeneratorFunction(kind)) {
+    if (IsAsyncGeneratorFunction(kind)) {
+      impl()->ParseAndRewriteAsyncGeneratorFunctionBody(pos, kind, body, ok);
+    } else if (IsGeneratorFunction(kind)) {
       impl()->ParseAndRewriteGeneratorFunctionBody(pos, kind, body, ok);
     } else if (IsAsyncFunction(kind)) {
       const bool accept_IN = true;

src/parsing/parser.cc

@@ -352,19 +352,6 @@ Expression* Parser::BuildUnaryExpression(Expression* expression,
   return factory()->NewUnaryOperation(op, expression, pos);
 }
 
-Expression* Parser::BuildIteratorResult(Expression* value, bool done) {
-  int pos = kNoSourcePosition;
-
-  if (value == nullptr) value = factory()->NewUndefinedLiteral(pos);
-
-  auto args = new (zone()) ZoneList<Expression*>(2, zone());
-  args->Add(value, zone());
-  args->Add(factory()->NewBooleanLiteral(done, pos), zone());
-
-  return factory()->NewCallRuntime(Runtime::kInlineCreateIterResultObject, args,
-                                   pos);
-}
-
 Expression* Parser::NewThrowError(Runtime::FunctionId id,
                                   MessageTemplate::Template message,
                                   const AstRawString* arg, int pos) {
@@ -1778,11 +1765,15 @@ Statement* Parser::RewriteTryStatement(Block* try_block, Block* catch_block,
 void Parser::ParseAndRewriteGeneratorFunctionBody(int pos, FunctionKind kind,
                                                   ZoneList<Statement*>* body,
                                                   bool* ok) {
-  // For ES6 Generators, we produce:
-  //
-  // try { InitialYield; ...body...; return {value: undefined, done: true} }
-  // finally { %_GeneratorClose(generator) }
-  //
+  // For ES6 Generators, we just prepend the initial yield.
+  Expression* initial_yield = BuildInitialYield(pos, kind);
+  body->Add(factory()->NewExpressionStatement(initial_yield, kNoSourcePosition),
+            zone());
+  ParseStatementList(body, Token::RBRACE, ok);
+}
+
+void Parser::ParseAndRewriteAsyncGeneratorFunctionBody(
+    int pos, FunctionKind kind, ZoneList<Statement*>* body, bool* ok) {
   // For ES2017 Async Generators, we produce:
   //
   // try {
@@ -1803,6 +1794,7 @@ void Parser::ParseAndRewriteGeneratorFunctionBody(int pos, FunctionKind kind,
   // - BytecodeGenerator performs special handling for ReturnStatements in
   //   async generator functions, resolving the appropriate Promise with an
   //   "done" iterator result object containing a Promise-unwrapped value.
+  DCHECK(IsAsyncGeneratorFunction(kind));
 
   Block* try_block = factory()->NewBlock(nullptr, 3, false, kNoSourcePosition);
   Expression* initial_yield = BuildInitialYield(pos, kind);
@@ -1812,49 +1804,45 @@ void Parser::ParseAndRewriteGeneratorFunctionBody(int pos, FunctionKind kind,
   ParseStatementList(try_block->statements(), Token::RBRACE, ok);
   if (!*ok) return;
 
-  if (IsAsyncGeneratorFunction(kind)) {
-    // Don't create iterator result for async generators, as the resume methods
-    // will create it.
-    Statement* final_return = BuildReturnStatement(
-        factory()->NewUndefinedLiteral(kNoSourcePosition), kNoSourcePosition);
-    try_block->statements()->Add(final_return, zone());
+  // Don't create iterator result for async generators, as the resume methods
+  // will create it.
+  Statement* final_return = BuildReturnStatement(
+      factory()->NewUndefinedLiteral(kNoSourcePosition), kNoSourcePosition);
+  try_block->statements()->Add(final_return, zone());
 
-    // For AsyncGenerators, a top-level catch block will reject the Promise.
-    Scope* catch_scope = NewHiddenCatchScopeWithParent(scope());
+  // For AsyncGenerators, a top-level catch block will reject the Promise.
+  Scope* catch_scope = NewHiddenCatchScopeWithParent(scope());
 
-    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
-    args->Add(factory()->NewVariableProxy(
-                  function_state_->generator_object_variable()),
-              zone());
-    args->Add(factory()->NewVariableProxy(catch_scope->catch_variable()),
-              zone());
+  ZoneList<Expression*>* reject_args =
+      new (zone()) ZoneList<Expression*>(2, zone());
+  reject_args->Add(
+      factory()->NewVariableProxy(function_state_->generator_object_variable()),
+      zone());
+  reject_args->Add(factory()->NewVariableProxy(catch_scope->catch_variable()),
+                   zone());
 
-    Expression* call = factory()->NewCallRuntime(
-        Runtime::kInlineAsyncGeneratorReject, args, kNoSourcePosition);
-    Block* catch_block = IgnoreCompletion(
-        factory()->NewReturnStatement(call, kNoSourcePosition));
+  Expression* reject_call = factory()->NewCallRuntime(
+      Runtime::kInlineAsyncGeneratorReject, reject_args, kNoSourcePosition);
+  Block* catch_block = IgnoreCompletion(
+      factory()->NewReturnStatement(reject_call, kNoSourcePosition));
 
-    TryStatement* try_catch = factory()->NewTryCatchStatementForAsyncAwait(
-        try_block, catch_scope, catch_block, kNoSourcePosition);
+  TryStatement* try_catch = factory()->NewTryCatchStatementForAsyncAwait(
+      try_block, catch_scope, catch_block, kNoSourcePosition);
 
-    try_block = factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
-    try_block->statements()->Add(try_catch, zone());
-  } else {
-    Statement* final_return = factory()->NewReturnStatement(
-        BuildIteratorResult(nullptr, true), kNoSourcePosition);
-    try_block->statements()->Add(final_return, zone());
-  }
+  try_block = factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
+  try_block->statements()->Add(try_catch, zone());
 
   Block* finally_block =
       factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
-  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
+  ZoneList<Expression*>* close_args =
+      new (zone()) ZoneList<Expression*>(1, zone());
   VariableProxy* call_proxy =
       factory()->NewVariableProxy(function_state_->generator_object_variable());
-  args->Add(call_proxy, zone());
-  Expression* call = factory()->NewCallRuntime(Runtime::kInlineGeneratorClose,
-                                               args, kNoSourcePosition);
+  close_args->Add(call_proxy, zone());
+  Expression* close_call = factory()->NewCallRuntime(
+      Runtime::kInlineGeneratorClose, close_args, kNoSourcePosition);
   finally_block->statements()->Add(
-      factory()->NewExpressionStatement(call, kNoSourcePosition), zone());
+      factory()->NewExpressionStatement(close_call, kNoSourcePosition), zone());
 
   body->Add(factory()->NewTryFinallyStatement(try_block, finally_block,
                                               kNoSourcePosition),

src/parsing/parser.h

@@ -352,6 +352,9 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   void ParseAndRewriteGeneratorFunctionBody(int pos, FunctionKind kind,
                                             ZoneList<Statement*>* body,
                                             bool* ok);
+  void ParseAndRewriteAsyncGeneratorFunctionBody(int pos, FunctionKind kind,
+                                                 ZoneList<Statement*>* body,
+                                                 bool* ok);
   void CreateFunctionNameAssignment(const AstRawString* function_name, int pos,
                                     FunctionLiteral::FunctionType function_type,
                                     DeclarationScope* function_scope,
@@ -874,8 +877,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   Expression* BuildUnaryExpression(Expression* expression, Token::Value op,
                                    int pos);
 
-  Expression* BuildIteratorResult(Expression* value, bool done);
-
   // Generate AST node that throws a ReferenceError with the given type.
   V8_INLINE Expression* NewThrowReferenceError(
       MessageTemplate::Template message, int pos) {

src/parsing/preparser.h

@@ -1076,6 +1076,10 @@ class PreParser : public ParserBase<PreParser> {
       int pos, FunctionKind kind, PreParserStatementList body, bool* ok) {
     ParseStatementList(body, Token::RBRACE, ok);
   }
+  V8_INLINE void ParseAndRewriteAsyncGeneratorFunctionBody(
+      int pos, FunctionKind kind, PreParserStatementList body, bool* ok) {
+    ParseStatementList(body, Token::RBRACE, ok);
+  }
   V8_INLINE void CreateFunctionNameAssignment(
       PreParserIdentifier function_name, int pos,
       FunctionLiteral::FunctionType function_type,
@@ -1289,11 +1293,6 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::Default();
   }
 
-  V8_INLINE PreParserExpression BuildIteratorResult(PreParserExpression value,
-                                                    bool done) {
-    return PreParserExpression::Default();
-  }
-
   V8_INLINE PreParserStatement
   BuildInitializationBlock(DeclarationParsingResult* parsing_result,
                            ZoneList<const AstRawString*>* names, bool* ok) {