Await always waits

runtime/vm/ast_transformer.cc

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/ast_transformer.h"
 
+#include "vm/object_store.h"
 #include "vm/parser.h"
 
 namespace dart {
 
 // Quick access to the locally defined isolate() method.
 #define I (isolate())
 
 // Nodes that are unreachable from already parsed expressions.
 #define FOR_EACH_UNREACHABLE_NODE(V)                                           \
   V(AwaitMarker)                                                               \
@@ skipping 19 matching lines @@
 
 #define DEFINE_UNREACHABLE(BaseName)                                           \
 void AwaitTransformer::Visit##BaseName##Node(BaseName##Node* node) {           \
   UNREACHABLE();                                                               \
 }
 
 FOR_EACH_UNREACHABLE_NODE(DEFINE_UNREACHABLE)
 #undef DEFINE_UNREACHABLE
 
 AwaitTransformer::AwaitTransformer(SequenceNode* preamble,
-                                   const Library& library,
                                    ParsedFunction* const parsed_function,
                                    LocalScope* function_top)
     : preamble_(preamble),
       temp_cnt_(0),
-      library_(library),
       parsed_function_(parsed_function),
       function_top_(function_top),
       isolate_(Isolate::Current()) {
   ASSERT(function_top_ != NULL);
 }
 
 
 AstNode* AwaitTransformer::Transform(AstNode* expr) {
   expr->Visit(this);
   return result_;
@@ skipping 45 matching lines @@
 void AwaitTransformer::VisitTypeNode(TypeNode* node) {
   result_ = new(I) TypeNode(node->token_pos(), node->type());
 }
 
 
 void AwaitTransformer::VisitAwaitNode(AwaitNode* node) {
   // Await transformation:
   //
   //   :await_temp_var_X = <expr>;
   //   :result_param = :await_temp_var_X;
-  //   if (:result_param is Future) {
-  //     AwaitMarker(kNewContinuationState);
-  //     :result_param = :result_param.then(:async_op);
-  //     _asyncCatchHelper(:result_param.catchError, :async_op);
-  //     return;  // (return_type() == kContinuation)
+  //   if (:result_param is !Future) {
+  //     :result_param = Future.value(:result_param);
   //   }
-  //   AwaitMarker(kTargetForContinuation);  // Join happens here.
+  //   AwaitMarker(kNewContinuationState);
+  //   :result_param = :result_param.then(:async_op);
+  //   _asyncCatchHelper(:result_param.catchError, :async_op);
+  //   return;  // (return_type() == kContinuationTarget)
+  //
   //   :saved_try_ctx_var = :await_saved_try_ctx_var_y;
   //   :await_temp_var_(X+1) = :result_param;
 
   LocalVariable* async_op = GetVariableInScope(
       preamble_->scope(), Symbols::AsyncOperation());
   LocalVariable* result_param = GetVariableInScope(
       preamble_->scope(), Symbols::AsyncOperationParam());
   LocalVariable* error_param = GetVariableInScope(
       preamble_->scope(), Symbols::AsyncOperationErrorParam());
 
-  node->expr()->Visit(this);
+  AstNode* transformed_expr = Transform(node->expr());
   preamble_->Add(new(I) StoreLocalNode(
-      Scanner::kNoSourcePos, result_param, result_));
+      Scanner::kNoSourcePos, result_param, transformed_expr));
+
   LoadLocalNode* load_result_param = new(I) LoadLocalNode(
       Scanner::kNoSourcePos, result_param);
-  LocalScope* is_future_scope = ChainNewScope(preamble_->scope());
-  SequenceNode* is_future_branch = new (I) SequenceNode(
-      Scanner::kNoSourcePos, is_future_scope);
-  AwaitMarkerNode* await_marker = new (I) AwaitMarkerNode(
-      AwaitMarkerNode::kNewContinuationState);
-  await_marker->set_scope(is_future_scope);
-  GetVariableInScope(is_future_scope, Symbols::AwaitJumpVar());
-  GetVariableInScope(is_future_scope, Symbols::AwaitContextVar());
-  is_future_branch->Add(await_marker);
+
+  const Class& future_cls =
+      Class::ZoneHandle(I, I->object_store()->future_class());
+  ASSERT(!future_cls.IsNull());
+  const AbstractType& future_type =
+      AbstractType::ZoneHandle(I, future_cls.RareType());
+  ASSERT(!future_type.IsNull());
+
+  LocalScope* is_not_future_scope = ChainNewScope(preamble_->scope());
+  SequenceNode* is_not_future_branch =
+      new (I) SequenceNode(Scanner::kNoSourcePos, is_not_future_scope);
+
+  // if (:result_param is !Future) {
+  //   :result_param = Future.value(:result_param);
+  // }
+  const Function& value_ctor = Function::ZoneHandle(
+      I, future_cls.LookupFunction(Symbols::FutureValue()));
+  ASSERT(!value_ctor.IsNull());
+  ArgumentListNode* ctor_args = new (I) ArgumentListNode(Scanner::kNoSourcePos);
+  ctor_args->Add(new (I) LoadLocalNode(Scanner::kNoSourcePos, result_param));
+  ConstructorCallNode* ctor_call =
+      new (I) ConstructorCallNode(Scanner::kNoSourcePos,
+                                  TypeArguments::ZoneHandle(I),
+                                  value_ctor,
+                                  ctor_args);
+  is_not_future_branch->Add(new (I) StoreLocalNode(
+      Scanner::kNoSourcePos, result_param, ctor_call));
+  AstNode* is_not_future_test = new (I) ComparisonNode(
+      Scanner::kNoSourcePos,
+      Token::kISNOT,
+      load_result_param,
+      new (I) TypeNode(Scanner::kNoSourcePos, future_type));
+  preamble_->Add(new(I) IfNode(Scanner::kNoSourcePos,
+                               is_not_future_test,
+                               is_not_future_branch,
+                               NULL));
+
+  AwaitMarkerNode* await_marker = new (I) AwaitMarkerNode();
+  await_marker->set_scope(preamble_->scope());
+  GetVariableInScope(preamble_->scope(), Symbols::AwaitJumpVar());

[Florian Schneider, 2014/10/14 11:47:30]

It looks a little strange that the result of this call is not used here. Why is
the side-effect needed here? and could it be moved to the place where the
variable is actually used? (e.g. EffectGraphVisitor::VisitAwaitMarkerNode)

[Florian Schneider, 2014/10/14 15:04:37]

I tried fixing this in my CL (https://codereview.chromium.org/655773003/) and
mark those as captured variables in the parser instead.

[hausner, 2014/10/14 19:56:39]

Let's move these with your change list, as a separate change from this one.

As i commented in your CL, I am not sure why these statements are here, but
there may be a reason for it. Looking up a variable for a particular scope has a
side effect on that scope. We may depend on this side effect here.

+  GetVariableInScope(preamble_->scope(), Symbols::AwaitContextVar());

[Florian Schneider, 2014/10/14 11:47:30]

Same question for this variable.

+  preamble_->Add(await_marker);
   ArgumentListNode* args = new(I) ArgumentListNode(Scanner::kNoSourcePos);
+
   args->Add(new(I) LoadLocalNode(Scanner::kNoSourcePos, async_op));
-  is_future_branch->Add(new (I) StoreLocalNode(
+  preamble_->Add(new (I) StoreLocalNode(
       Scanner::kNoSourcePos,
       result_param,
-      new(I) InstanceCallNode(
-          Scanner::kNoSourcePos,
-          load_result_param,
-          Symbols::FutureThen(),
-          args)));
+      new(I) InstanceCallNode(Scanner::kNoSourcePos,
+                              load_result_param,
+                              Symbols::FutureThen(),
+                              args)));
   const Library& core_lib = Library::Handle(Library::CoreLibrary());
   const Function& async_catch_helper = Function::ZoneHandle(
       I, core_lib.LookupFunctionAllowPrivate(Symbols::AsyncCatchHelper()));
   ASSERT(!async_catch_helper.IsNull());
   ArgumentListNode* catch_helper_args = new (I) ArgumentListNode(
       Scanner::kNoSourcePos);
   InstanceGetterNode* catch_error_getter = new (I) InstanceGetterNode(
       Scanner::kNoSourcePos,
       load_result_param,
       Symbols::FutureCatchError());
   catch_helper_args->Add(catch_error_getter);
   catch_helper_args->Add(new (I) LoadLocalNode(
       Scanner::kNoSourcePos, async_op));
-  is_future_branch->Add(new (I) StaticCallNode(
+  preamble_->Add(new (I) StaticCallNode(
       Scanner::kNoSourcePos,
       async_catch_helper,
       catch_helper_args));
   ReturnNode* continuation_return = new(I) ReturnNode(Scanner::kNoSourcePos);
-  continuation_return->set_return_type(ReturnNode::kContinuation);
-  is_future_branch->Add(continuation_return);
-
-  const Class& cls = Class::ZoneHandle(
-      I, library_.LookupClass(Symbols::Future()));
-  const AbstractType& future_type = AbstractType::ZoneHandle(I, cls.RareType());
-  ASSERT(!future_type.IsNull());
-  preamble_->Add(new(I) IfNode(
-      Scanner::kNoSourcePos,
-      new (I) ComparisonNode(
-          Scanner::kNoSourcePos,
-          Token::kIS,
-          load_result_param,
-          new (I) TypeNode(Scanner::kNoSourcePos, future_type)),
-      is_future_branch,
-      NULL));
-  preamble_->Add(new (I) AwaitMarkerNode(
-      AwaitMarkerNode::kTargetForContinuation));
+  continuation_return->set_return_type(ReturnNode::kContinuationTarget);
+  preamble_->Add(continuation_return);
 
   // If this expression is part of a try block, also append the code for
   // restoring the saved try context that lives on the stack.
   const String& async_saved_try_ctx_name =
       String::Handle(I, parsed_function_->async_saved_try_ctx_name());
   if (!async_saved_try_ctx_name.IsNull()) {
     LocalVariable* async_saved_try_ctx =
         GetVariableInScope(preamble_->scope(), async_saved_try_ctx_name);
     preamble_->Add(new (I) StoreLocalNode(
         Scanner::kNoSourcePos,
@@ skipping 380 matching lines @@
 void AwaitTransformer::VisitThrowNode(ThrowNode* node) {
   // TODO(mlippautz): Check if relevant.
   AstNode* new_exception = Transform(node->exception());
   AstNode* new_stacktrace = Transform(node->stacktrace());
   result_ = new(I) ThrowNode(node->token_pos(),
                              new_exception,
                              new_stacktrace);
 }
 
 }  // namespace dart