Add the implicit return in the correct sequence node

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 44786)
+++ runtime/vm/parser.cc	(working copy)
@@ -764,18 +764,6 @@
 };
 
 
-static bool HasReturnNode(SequenceNode* seq) {
-  if (seq->length() == 0) {
-    return false;
-  } else if ((seq->length()) == 1 &&
-             (seq->NodeAt(seq->length() - 1)->IsSequenceNode())) {
-    return HasReturnNode(seq->NodeAt(seq->length() - 1)->AsSequenceNode());
-  } else {
-    return seq->NodeAt(seq->length() - 1)->IsReturnNode();
-  }
-}
-
-
 void Parser::ParseClass(const Class& cls) {
   if (!cls.is_synthesized_class()) {
     Isolate* isolate = Isolate::Current();
@@ -906,16 +894,6 @@
       UNREACHABLE();
   }
 
-  if (!HasReturnNode(node_sequence)) {
-    // Add implicit return node. The implicit return value of synchronous
-    // generator closures is false, to indicate that there are no more
-    // elements in the iterable. In other cases the implicit return value
-    // is null.
-    AstNode* return_value = func.IsSyncGenClosure()
-        ? new LiteralNode(func.end_token_pos(), Bool::False())
-        : new LiteralNode(func.end_token_pos(), Instance::ZoneHandle());
-    node_sequence->Add(new ReturnNode(func.end_token_pos(), return_value));
-  }
   if (parsed_function->has_expression_temp_var()) {
     node_sequence->scope()->AddVariable(parsed_function->expression_temp_var());
   }
@@ -6837,6 +6815,37 @@
 }
 
 
+// This is called for the compiled function's outermost node sequence.
+// Ensure that the node sequence S containing the function's code ends
+// with a return statement. S may be a child sequence of seq.

[regis, 2015/03/31 18:44:41]

The comment does not explain the logic implemented below. Why do you only
consider the child of a sequence of length 1? Why do you check that the token
positions of the child and parent are equal? What if the scope is null and you
cannot check?

Is this code supposed to handle this case?
if (cond) {
  no_return();
} else {
  return;
}

+void Parser::EnsureHasReturnNode(SequenceNode* seq) {
+  // Find innermost child sequence that ends at the same token position.
+  while ((seq->length() == 1) && (seq->NodeAt(0)->IsSequenceNode())) {
+    SequenceNode* child = seq->NodeAt(0)->AsSequenceNode();
+    if ((seq->scope() != NULL) &&
+        (child->scope() != NULL) &&
+        (seq->scope()->end_token_pos() == child->scope()->end_token_pos())) {
+      seq = child;
+    } else {
+      break;
+    }
+  }
+  // Add a return node if necessary.
+  if ((seq->length() == 0) ||
+      !seq->NodeAt(seq->length() - 1)->IsReturnNode()) {
+    // The implicit return value of synchronous generator closures is false,
+    // to indicate that there are no more elements in the iterable.
+    // In other cases the implicit return value is null.
+    ASSERT(current_function().raw() == innermost_function().raw());
+    const Function& func = current_function();
+    AstNode* return_value = func.IsSyncGenClosure()
+        ? new LiteralNode(func.end_token_pos(), Bool::False())
+        : new LiteralNode(func.end_token_pos(), Instance::ZoneHandle());
+    seq->Add(new ReturnNode(func.end_token_pos(), return_value));
+  }
+}
+
+
 SequenceNode* Parser::CloseBlock() {
   SequenceNode* statements = current_block_->statements;
   if (current_block_->scope != NULL) {
@@ -6846,6 +6855,10 @@
     current_block_->scope->set_end_token_pos(TokenPos());
   }
   current_block_ = current_block_->parent;
+  if (current_block_ == NULL) {
+    ASSERT(statements != NULL);
+    EnsureHasReturnNode(statements);

[regis, 2015/03/31 18:44:41]

This does not feel right to me. Closing a block should not add a return
statement behind the scene.

+  }
   return statements;
 }