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

src/ast/ast.h

Index: src/ast/ast.h
diff --git a/src/ast/ast.h b/src/ast/ast.h
index 5e8255051f8bb556761ff0efe86a2c6b81a50152..cbba803983bc267a6287d0da907bd98d7f3d28c4 100644
--- a/src/ast/ast.h
+++ b/src/ast/ast.h
@@ -896,17 +896,22 @@ class BreakStatement final : public JumpStatement {
 
 class ReturnStatement final : public JumpStatement {
  public:
+  enum Type { kNormal, kHardReturn };

[Dan Ehrenberg, 2017/02/08 21:12:56]

Bikeshedding suggestion: Instead, have normal returns and async returns.
RewriteReturn would turn return statements into async return statements when in
an async function. RewriteReturn only runs for syntactic return statements, not
for ones that come out of desugaring, so this should end up doing the same
thing, right?

[caitp, 2017/02/08 23:22:15]

As explained on IRC:

The key fix for the try/finally issue is to remove the Parser::RewriteReturn()
stuff for async functions.

In place of calling `ResolvePromise(<function_promise>, return_value);` in the
Parser/AST, it's done in BCG::BuildReturn().

But, because this version doesn't move the implicit control flow (top-level
try/catch blocks) into BCG, and the parser still generates the `return
RejectPromise(). <function_promise>;`, or effectively `return
<function_promise>;` at the end of those implicit try/catch blocks. As noted
above, BCG would turn this into `ResolvePromise(<function_promise>,
<function_promise>)`, which is a circular promise resolution.

This "hard return" bit tells the BCG not to transform the return value into a
ResolvePromise() call, so it can avoid the circular promise resolution.

Please let me know if I'm not explaining this clearly.

[Dan Ehrenberg, 2017/02/09 06:46:58]

I see how you need to differentiate between returns that need the promise
behavior and returns that don't in order to avoid introducing a bug, and the
logic seems valid here. Above, I was making a style suggestion about phrasing
that logic--I though the logic I described would end up generating the same
bytecode.

I am still missing something about how this fixes the issue. You end up
desugaring (in the BCG now) to the same thing in some cases. Which cases differ?
I can't find them. Is this all about the implicit 'return undefined' generated
at the end?

   Expression* expression() const { return expression_; }
 
   void set_expression(Expression* e) { expression_ = e; }
+  bool is_hard_return() const { return type_ == kHardReturn; }
 
  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),
+        type_(type) {}
 
   Expression* expression_;
+  Type type_ = kNormal;
 };
 
 
@@ -3209,7 +3214,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* NewHardReturnStatement(Expression* expression, int pos) {
+    return new (zone_)
+        ReturnStatement(expression, ReturnStatement::kHardReturn, pos);
   }
 
   WithStatement* NewWithStatement(Scope* scope,