Add the implicit return in the correct sequence node

runtime/vm/parser.cc

 // Copyright (c) 2012, 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/parser.h"
 
 #include "lib/invocation_mirror.h"
 #include "platform/utils.h"
 #include "vm/ast_transformer.h"
 #include "vm/bootstrap.h"
@@ skipping 746 matching lines @@
 struct TopLevel {
   TopLevel() :
       fields(GrowableObjectArray::Handle(GrowableObjectArray::New())),
       functions(GrowableObjectArray::Handle(GrowableObjectArray::New())) { }
 
   GrowableObjectArray& fields;
   GrowableObjectArray& functions;
 };
 
 
-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();
     TimerScope timer(FLAG_compiler_stats, &CompilerStats::parser_timer);
     ASSERT(isolate->long_jump_base()->IsSafeToJump());
     const Script& script = Script::Handle(isolate, cls.script());
     const Library& lib = Library::Handle(isolate, cls.library());
     Parser parser(script, lib, cls.token_pos());
     parser.ParseClassDefinition(cls);
   } else if (cls.is_enum_class()) {
@@ skipping 110 matching lines @@
     case RawFunction::kInvokeFieldDispatcher:
       node_sequence =
           parser.ParseInvokeFieldDispatcher(func, &default_parameter_values);
       break;
     case RawFunction::kIrregexpFunction:
       UNREACHABLE();  // Irregexp functions have their own parser.
     default:
       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());
   }
   node_sequence->scope()->AddVariable(
       parsed_function->current_context_var());
   if (parsed_function->has_finally_return_temp_var()) {
     node_sequence->scope()->AddVariable(
         parsed_function->finally_return_temp_var());
   }
   parsed_function->SetNodeSequence(node_sequence);
@@ skipping 5901 matching lines @@
   new_body->scope()->LookupVariable(Symbols::Controller(), false);
   new_body->scope()->LookupVariable(Symbols::AsyncOperationParam(), false);
   new_body->scope()->LookupVariable(Symbols::AsyncOperationErrorParam(), false);
   new_body->scope()->LookupVariable(
       Symbols::AsyncOperationStackTraceParam(), false);
   new_body->scope()->RecursivelyCaptureAllVariables();
   return new_body;
 }
 
 
+// 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) {
     // Record the begin and end token index of the scope.
     ASSERT(statements != NULL);
     current_block_->scope->set_begin_token_pos(statements->token_pos());
     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;
 }
 
 
 SequenceNode* Parser::CloseAsyncFunction(const Function& closure,
                                          SequenceNode* closure_body) {
   TRACE_PARSER("CloseAsyncFunction");
   ASSERT(!closure.IsNull());
   ASSERT(closure_body != NULL);
 
@@ skipping 6554 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart