[async-await] (simpler) fix for Return in try/finally in async functions

Alternative approach to https://codereview.chromium.org/2667983004/, which
does not depend on implicit control flow changes from
https://codereview.chromium.org/2664083002

- Remove handling for `async function` from Parser::RewriteReturn(). This functionality
is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution
is deferred until all finally blocks are evaluated fully.

- Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to
generate return code using BuildAsyncReturn rather than BuildReturn.

- Parser has a new `NewReturnStatement()` helper which determines what type of return
statement to generate based on the type of function.

BUG=v8:5896, v8:4483
R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org

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

src/ast/ast-numbering.cc

@@ -234,6 +234,9 @@ void AstNumberingVisitor::VisitExpressionStatement(ExpressionStatement* node) {
 void AstNumberingVisitor::VisitReturnStatement(ReturnStatement* node) {
   IncrementNodeCount();
   Visit(node->expression());
+
+  DCHECK(!node->is_async_return() ||
+         properties_.flags() & AstProperties::kMustUseIgnitionTurbo);
 }
 
 

src/ast/ast.h

@@ -897,17 +897,25 @@ class BreakStatement final : public JumpStatement {
 
 class ReturnStatement final : public JumpStatement {
  public:
+  enum Type { kNormal, kAsyncReturn };
   Expression* expression() const { return expression_; }
 
   void set_expression(Expression* e) { expression_ = e; }
+  Type type() const { return TypeField::decode(bit_field_); }
+  bool is_async_return() const { return type() == kAsyncReturn; }
 
  private:
   friend class AstNodeFactory;
 
-  ReturnStatement(Expression* expression, int pos)
-      : JumpStatement(pos, kReturnStatement), expression_(expression) {}
+  ReturnStatement(Expression* expression, Type type, int pos)
+      : JumpStatement(pos, kReturnStatement), expression_(expression) {
+    bit_field_ |= TypeField::encode(type);
+  }
 
   Expression* expression_;
+
+  class TypeField
+      : public BitField<Type, JumpStatement::kNextBitFieldIndex, 1> {};
 };
 
 
@@ -3210,7 +3218,13 @@ class AstNodeFactory final BASE_EMBEDDED {
   }
 
   ReturnStatement* NewReturnStatement(Expression* expression, int pos) {
-    return new (zone_) ReturnStatement(expression, pos);
+    return new (zone_)
+        ReturnStatement(expression, ReturnStatement::kNormal, pos);
+  }
+
+  ReturnStatement* NewAsyncReturnStatement(Expression* expression, int pos) {
+    return new (zone_)
+        ReturnStatement(expression, ReturnStatement::kAsyncReturn, pos);
   }
 
   WithStatement* NewWithStatement(Scope* scope,

src/ast/scopes.cc

@@ -297,7 +297,7 @@ void DeclarationScope::SetDefaults() {
   new_target_ = nullptr;
   function_ = nullptr;
   arguments_ = nullptr;
-  this_function_ = nullptr;
+  rare_data_ = nullptr;
   should_eager_compile_ = false;
   was_lazily_parsed_ = false;
 #ifdef DEBUG
@@ -682,7 +682,7 @@ void DeclarationScope::DeclareDefaultFunctionVariables(
 
   if (IsConciseMethod(function_kind_) || IsClassConstructor(function_kind_) ||
       IsAccessorFunction(function_kind_)) {
-    this_function_ =
+    EnsureRareData()->this_function =
         Declare(zone(), ast_value_factory->this_function_string(), CONST);
   }
 }
@@ -703,6 +703,24 @@ Variable* DeclarationScope::DeclareFunctionVar(const AstRawString* name) {
   return function_;
 }
 
+Variable* DeclarationScope::DeclareGeneratorObjectVar(
+    const AstRawString* name) {
+  DCHECK(is_function_scope() || is_module_scope());
+  DCHECK_NULL(generator_object_var());
+
+  Variable* result = EnsureRareData()->generator_object = NewTemporary(name);
+  result->set_is_used();
+  return result;
+}
+
+Variable* DeclarationScope::DeclarePromiseVar(const AstRawString* name) {
+  DCHECK(is_function_scope());
+  DCHECK_NULL(promise_var());
+  Variable* result = EnsureRareData()->promise = NewTemporary(name);
+  result->set_is_used();
+  return result;
+}
+
 bool Scope::HasBeenRemoved() const {
   if (sibling() == this) {
     DCHECK_NULL(inner_scope_);
@@ -2143,9 +2161,8 @@ void DeclarationScope::AllocateLocals() {
     new_target_ = nullptr;
   }
 
-  if (this_function_ != nullptr && !MustAllocate(this_function_)) {
-    this_function_ = nullptr;
-  }
+  NullifyRareVariableIf(RareVariable::kThisFunction,
+                        [=](Variable* var) { return !MustAllocate(var); });
 }
 
 void ModuleScope::AllocateModuleVariables() {

src/ast/scopes.h

@@ -680,6 +680,11 @@ class DeclarationScope : public Scope {
   // calls sloppy eval.
   Variable* DeclareFunctionVar(const AstRawString* name);
 
+  // Declare some special internal variables which must be accessible to
+  // Ignition without ScopeInfo.
+  Variable* DeclareGeneratorObjectVar(const AstRawString* name);
+  Variable* DeclarePromiseVar(const AstRawString* name);
+
   // Declare a parameter in this scope.  When there are duplicated
   // parameters the rightmost one 'wins'.  However, the implementation
   // expects all parameters to be declared and from left to right.
@@ -723,6 +728,17 @@ class DeclarationScope : public Scope {
     return function_;
   }
 
+  Variable* generator_object_var() const {
+    DCHECK(is_function_scope() || is_module_scope());
+    return GetRareVariable(RareVariable::kGeneratorObject);
+  }
+
+  Variable* promise_var() const {
+    DCHECK(is_function_scope());
+    DCHECK(IsAsyncFunction(function_kind_));
+    return GetRareVariable(RareVariable::kPromise);
+  }
+
   // Parameters. The left-most parameter has index 0.
   // Only valid for function and module scopes.
   Variable* parameter(int index) const {
@@ -763,12 +779,14 @@ class DeclarationScope : public Scope {
   }
 
   Variable* this_function_var() const {
+    Variable* this_function = GetRareVariable(RareVariable::kThisFunction);
+
     // This is only used in derived constructors atm.
-    DCHECK(this_function_ == nullptr ||
+    DCHECK(this_function == nullptr ||
            (is_function_scope() && (IsClassConstructor(function_kind()) ||
                                     IsConciseMethod(function_kind()) ||
                                     IsAccessorFunction(function_kind()))));
-    return this_function_;
+    return this_function;
   }
 
   // Adds a local variable in this scope's locals list. This is for adjusting
@@ -877,8 +895,50 @@ class DeclarationScope : public Scope {
   Variable* new_target_;
   // Convenience variable; function scopes only.
   Variable* arguments_;
-  // Convenience variable; Subclass constructor only
-  Variable* this_function_;
+
+  struct RareData : public ZoneObject {
+    void* operator new(size_t size, Zone* zone) { return zone->New(size); }
+
+    // Convenience variable; Subclass constructor only
+    Variable* this_function = nullptr;
+
+    // Generator object, if any; generator function scopes and module scopes
+    // only.
+    Variable* generator_object = nullptr;
+    // Promise, if any; async function scopes only.
+    Variable* promise = nullptr;
+  };
+
+  enum class RareVariable {
+    kThisFunction = offsetof(RareData, this_function),
+    kGeneratorObject = offsetof(RareData, generator_object),
+    kPromise = offsetof(RareData, promise)
+  };
+
+  V8_INLINE RareData* EnsureRareData() {
+    if (rare_data_ == nullptr) {
+      rare_data_ = new (zone_) RareData;
+    }
+    return rare_data_;
+  }
+
+  V8_INLINE Variable* GetRareVariable(RareVariable id) const {
+    if (rare_data_ == nullptr) return nullptr;
+    return *reinterpret_cast<Variable**>(
+        reinterpret_cast<uint8_t*>(rare_data_) + static_cast<ptrdiff_t>(id));
+  }
+
+  // Set `var` to null if it's non-null and Predicate (Variable*) -> bool
+  // returns true.
+  template <typename Predicate>
+  V8_INLINE void NullifyRareVariableIf(RareVariable id, Predicate predicate) {
+    if (V8_LIKELY(rare_data_ == nullptr)) return;
+    Variable** var = reinterpret_cast<Variable**>(
+        reinterpret_cast<uint8_t*>(rare_data_) + static_cast<ptrdiff_t>(id));
+    if (*var && predicate(*var)) *var = nullptr;
+  }
+
+  RareData* rare_data_ = nullptr;
 };
 
 class ModuleScope final : public DeclarationScope {

src/interpreter/bytecode-generator.cc

@@ -106,12 +106,19 @@ class BytecodeGenerator::ControlScope BASE_EMBEDDED {
   void Break(Statement* stmt) { PerformCommand(CMD_BREAK, stmt); }
   void Continue(Statement* stmt) { PerformCommand(CMD_CONTINUE, stmt); }
   void ReturnAccumulator() { PerformCommand(CMD_RETURN, nullptr); }
+  void AsyncReturnAccumulator() { PerformCommand(CMD_ASYNC_RETURN, nullptr); }
   void ReThrowAccumulator() { PerformCommand(CMD_RETHROW, nullptr); }
 
   class DeferredCommands;
 
  protected:
-  enum Command { CMD_BREAK, CMD_CONTINUE, CMD_RETURN, CMD_RETHROW };
+  enum Command {
+    CMD_BREAK,
+    CMD_CONTINUE,
+    CMD_RETURN,
+    CMD_ASYNC_RETURN,
+    CMD_RETHROW
+  };
   void PerformCommand(Command command, Statement* statement);
   virtual bool Execute(Command command, Statement* statement) = 0;
 
@@ -221,6 +228,9 @@ class BytecodeGenerator::ControlScopeForTopLevel final
       case CMD_RETURN:
         generator()->BuildReturn();
         return true;
+      case CMD_ASYNC_RETURN:
+        generator()->BuildAsyncReturn();
+        return true;
       case CMD_RETHROW:
         generator()->BuildReThrow();
         return true;
@@ -249,6 +259,7 @@ class BytecodeGenerator::ControlScopeForBreakable final
         return true;
       case CMD_CONTINUE:
       case CMD_RETURN:
+      case CMD_ASYNC_RETURN:
       case CMD_RETHROW:
         break;
     }
@@ -286,6 +297,7 @@ class BytecodeGenerator::ControlScopeForIteration final
         loop_builder_->Continue();
         return true;
       case CMD_RETURN:
+      case CMD_ASYNC_RETURN:
       case CMD_RETHROW:
         break;
     }
@@ -311,6 +323,7 @@ class BytecodeGenerator::ControlScopeForTryCatch final
       case CMD_BREAK:
       case CMD_CONTINUE:
       case CMD_RETURN:
+      case CMD_ASYNC_RETURN:
         break;
       case CMD_RETHROW:
         generator()->BuildReThrow();
@@ -337,6 +350,7 @@ class BytecodeGenerator::ControlScopeForTryFinally final
       case CMD_BREAK:
       case CMD_CONTINUE:
       case CMD_RETURN:
+      case CMD_ASYNC_RETURN:
       case CMD_RETHROW:
         commands_->RecordCommand(command, statement);
         try_finally_builder_->LeaveTry();
@@ -1058,7 +1072,12 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForAccumulatorValue(stmt->expression());
-  execution_control()->ReturnAccumulator();
+
+  if (stmt->is_async_return()) {
+    execution_control()->AsyncReturnAccumulator();
+  } else {
+    execution_control()->ReturnAccumulator();
+  }
 }
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
@@ -2001,6 +2020,28 @@ void BytecodeGenerator::BuildReturn() {
   builder()->Return();
 }
 
+void BytecodeGenerator::BuildAsyncReturn() {
+  DCHECK(IsAsyncFunction(info()->literal()->kind()));
+  RegisterAllocationScope register_scope(this);
+  RegisterList args = register_allocator()->NewRegisterList(3);
+  Register receiver = args[0];
+  Register promise = args[1];
+  Register return_value = args[2];
+  builder()->StoreAccumulatorInRegister(return_value);
+
+  Variable* var_promise = scope()->promise_var();
+  DCHECK_NOT_NULL(var_promise);
+  BuildVariableLoad(var_promise, FeedbackSlot::Invalid(),
+                    HoleCheckMode::kElided);
+  builder()
+      ->StoreAccumulatorInRegister(promise)
+      .LoadUndefined()
+      .StoreAccumulatorInRegister(receiver)
+      .CallJSRuntime(Context::PROMISE_RESOLVE_INDEX, args)
+      .LoadAccumulatorWithRegister(promise);
+  BuildReturn();
+}
+
 void BytecodeGenerator::BuildReThrow() { builder()->ReThrow(); }
 
 void BytecodeGenerator::BuildAbort(BailoutReason bailout_reason) {

src/interpreter/bytecode-generator.h

@@ -105,6 +105,7 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
                                HoleCheckMode hole_check_mode);
 
   void BuildReturn();
+  void BuildAsyncReturn();
   void BuildReThrow();
   void BuildAbort(BailoutReason bailout_reason);
   void BuildThrowIfHole(Handle<String> name);

src/parsing/parser-base.h

@@ -427,23 +427,12 @@ class ParserBase {
     FunctionKind kind() const { return scope()->function_kind(); }
     FunctionState* outer() const { return outer_function_state_; }
 
-    void set_generator_object_variable(typename Types::Variable* variable) {
-      DCHECK_NOT_NULL(variable);
-      DCHECK(IsResumableFunction(kind()));
-      DCHECK(scope()->has_forced_context_allocation());
-      generator_object_variable_ = variable;
-    }
     typename Types::Variable* generator_object_variable() const {
-      return generator_object_variable_;
+      return scope()->generator_object_var();
     }
 
-    void set_promise_variable(typename Types::Variable* variable) {
-      DCHECK(variable != NULL);
-      DCHECK(IsAsyncFunction(kind()));
-      promise_variable_ = variable;
-    }
     typename Types::Variable* promise_variable() const {
-      return promise_variable_;
+      return scope()->promise_var();
     }
 
     const ZoneList<DestructuringAssignment>&
@@ -1383,6 +1372,15 @@ class ParserBase {
     return Call::NOT_EVAL;
   }
 
+  // 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 (V8_UNLIKELY(is_async_function())) {
+      return factory()->NewAsyncReturnStatement(expr, pos);
+    }
+    return factory()->NewReturnStatement(expr, pos);
+  }
+
   // Validation per ES6 object literals.
   class ObjectLiteralChecker {
    public:
@@ -4276,9 +4274,8 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
       } else {
         ExpressionT expression = ParseAssignmentExpression(accept_IN, CHECK_OK);
         impl()->RewriteNonPattern(CHECK_OK);
-        body->Add(
-            factory()->NewReturnStatement(expression, expression->position()),
-            zone());
+        body->Add(BuildReturnStatement(expression, expression->position()),
+                  zone());
         if (allow_tailcalls() && !is_sloppy(language_mode())) {
           // ES6 14.6.1 Static Semantics: IsInTailPosition
           impl()->MarkTailPosition(expression);
@@ -5183,7 +5180,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseReturnStatement(
   }
   ExpectSemicolon(CHECK_OK);
   return_value = impl()->RewriteReturn(return_value, loc.beg_pos);
-  return factory()->NewReturnStatement(return_value, loc.beg_pos);
+  return BuildReturnStatement(return_value, loc.beg_pos);
 }
 
 template <typename Impl>

src/parsing/parser.cc

@@ -1613,8 +1613,6 @@ 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());
   }
   return return_value;
 }
@@ -2477,9 +2475,8 @@ void Parser::PrepareGeneratorVariables() {
   // Calling a generator returns a generator object.  That object is stored
   // in a temporary variable, a definition that is used by "yield"
   // expressions.
-  Variable* temp =
-      NewTemporary(ast_value_factory()->dot_generator_object_string());
-  function_state_->set_generator_object_variable(temp);
+  function_state_->scope()->DeclareGeneratorObjectVar(
+      ast_value_factory()->dot_generator_object_string());
 }
 
 FunctionLiteral* Parser::ParseFunctionLiteral(
@@ -3093,8 +3090,8 @@ Variable* Parser::PromiseVariable() {
   // 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);
+    promise = function_state_->scope()->DeclarePromiseVar(
+        ast_value_factory()->empty_string());
   }
   return promise;
 }

src/parsing/preparser.h

@@ -670,6 +670,10 @@ class PreParserFactory {
                                         int pos) {
     return PreParserStatement::Jump();
   }
+  PreParserStatement NewAsyncReturnStatement(PreParserExpression expression,
+                                             int pos) {
+    return PreParserStatement::Jump();
+  }
   PreParserExpression NewFunctionLiteral(
       PreParserIdentifier name, Scope* scope, PreParserStatementList body,
       int materialized_literal_count, int expected_property_count,

test/mjsunit/es8/async-function-try-finally.js

+// Copyright 2017 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.
+
+// Flags: --allow-natives-syntax
+
+function assertThrowsAsync(run, errorType, message) {
+  var actual;
+  var hadValue = false;
+  var hadError = false;
+  var promise = run();
+
+  if (typeof promise !== "object" || typeof promise.then !== "function") {
+    throw new MjsUnitAssertionError(
+        "Expected " + run.toString() +
+        " to return a Promise, but it returned " + PrettyPrint(promise));
+  }
+
+  promise.then(function(value) { hadValue = true; actual = value; },
+               function(error) { hadError = true; actual = error; });
+
+  assertFalse(hadValue || hadError);
+
+  %RunMicrotasks();
+
+  if (!hadError) {
+    throw new MjsUnitAssertionError(
+        "Expected " + run + "() to throw " + errorType.name +
+        ", but did not throw.");
+  }
+  if (!(actual instanceof errorType))
+    throw new MjsUnitAssertionError(
+        "Expected " + run + "() to throw " + errorType.name +
+        ", but threw '" + actual + "'");
+  if (message !== void 0 && actual.message !== message)
+    throw new MjsUnitAssertionError(
+        "Expected " + run + "() to throw '" + message + "', but threw '" +
+        actual.message + "'");
+};
+
+function assertEqualsAsync(expected, run, msg) {
+  var actual;
+  var hadValue = false;
+  var hadError = false;
+  var promise = run();
+
+  if (typeof promise !== "object" || typeof promise.then !== "function") {
+    throw new MjsUnitAssertionError(
+        "Expected " + run.toString() +
+        " to return a Promise, but it returned " + PrettyPrint(promise));
+  }
+
+  promise.then(function(value) { hadValue = true; actual = value; },
+               function(error) { hadError = true; actual = error; });
+
+  assertFalse(hadValue || hadError);
+
+  %RunMicrotasks();
+
+  if (hadError) throw actual;
+
+  assertTrue(
+      hadValue, "Expected '" + run.toString() + "' to produce a value");
+
+  assertEquals(expected, actual, msg);
+};
+
+function resolveLater(value) {
+  return new Promise(function(resolve) {
+    resolve(value);
+  });
+}
+
+function rejectLater(reason) {
+  return new Promise(function(resolve, reject) {
+    reject(reason);
+  });
+}
+
+class MyError extends Error {};
+var AsyncFunction = async function() {}.constructor;
+
+assertEqualsAsync("finally-return (func-expr)", async function() {
+  try {
+    return "early-return (func-expr)";
+  } finally {
+    return "finally-return (func-expr)";
+  }
+});
+
+assertEqualsAsync("finally-return (arrow)", async() => {
+  try {
+    return "early-return (arrow)";
+  } finally {
+    return "finally-return (arrow)";
+  }
+});
+
+assertEqualsAsync("finally-return (eval)", AsyncFunction(`
+  try {
+    return "early-return (eval)";
+  } finally {
+    return "finally-return (eval)";
+  }
+`));
+
+assertEqualsAsync("promise-finally-return (func-expr)", async function() {
+  try {
+    return new Promise(function() {});
+  } finally {
+    return "promise-finally-return (func-expr)";
+  }
+});
+
+assertEqualsAsync("promise-finally-return (arrow)", async() => {
+  try {
+    return new Promise(function() {});
+  } finally {
+    return "promise-finally-return (arrow)";
+  }
+});
+
+assertEqualsAsync("promise-finally-return (eval)", AsyncFunction(`
+  try {
+    return new Promise(function() {});
+  } finally {
+    return "promise-finally-return (eval)";
+  }
+`));
+
+assertEqualsAsync("await-finally-return (func-expr)", async function() {
+  try {
+    return "early-return (func-expr)";
+  } finally {
+    return await resolveLater("await-finally-return (func-expr)");
+  }
+});
+
+assertEqualsAsync("await-finally-return (arrow)", async() => {
+  try {
+    return "early-return (arrow)";
+  } finally {
+    return await resolveLater("await-finally-return (arrow)");
+  }
+});
+
+assertEqualsAsync("await-finally-return (eval)", AsyncFunction(`
+  try {
+    return "early-return (eval)";
+  } finally {
+    return await resolveLater("await-finally-return (eval)");
+  }
+`));
+
+assertThrowsAsync(async function() {
+  try {
+    return "early-return (func-expr)";
+  } finally {
+    throw new MyError("finally-throw (func-expr)");
+  }
+}, MyError, "finally-throw (func-expr)");
+
+assertThrowsAsync(async() => {
+  try {
+    return "early-return (arrow)";
+  } finally {
+    throw new MyError("finally-throw (arrow)");
+  }
+}, MyError, "finally-throw (arrow)");
+
+assertThrowsAsync(AsyncFunction(`
+  try {
+    return "early-return (eval)";
+  } finally {
+    throw new MyError("finally-throw (eval)");
+  }
+`), MyError, "finally-throw (eval)");
+
+assertThrowsAsync(async function() {
+  try {
+    return "early-return (func-expr)";
+  } finally {
+    await rejectLater(new MyError("await-finally-throw (func-expr)"));
+  }
+}, MyError, "await-finally-throw (func-expr)");
+
+assertThrowsAsync(async() => {
+  try {
+    return "early-return (arrow)";
+  } finally {
+    await rejectLater(new MyError("await-finally-throw (arrow)"));
+  }
+}, MyError, "await-finally-throw (arrow)");
+
+assertThrowsAsync(AsyncFunction(`
+  try {
+    return "early-return (eval)";
+  } finally {
+    await rejectLater(new MyError("await-finally-throw (eval)"));
+  }
+`), MyError, "await-finally-throw (eval)");