Add the implicit return in the correct sequence node

Add the implicit return in the correct sequence node

Make sure we add the implicit return node into the sequence
node that contains the function’s code. Previously, we added
the implicit return in the outermost sequence. If the outermost
sequence is at a different context level than the sequence that
contains the function’s code, the debugger cannot access the
captured variables when stopped at the return statement.

R=regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44826

[hausner, 2015-03-31 18:09:41]

hausner@google.com changed reviewers:
+ regis@google.com

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

Sorry, but I do not like that the return statement is inserted as a side effect
of closing a block. If a future change requires blocks to be opened and closed
slightly differently, return statements will start popping up here and there.

Isn't it possible to check for and add the implicit return statement when done
parsing a function body? It would be much clearer and safer to do it in an
explicit manner rather than implicitly when closing any block.

https://codereview.chromium.org/1037183003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1037183003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6820: // with a return statement. S may be a child sequence
of seq.
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;
}

https://codereview.chromium.org/1037183003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6860: EnsureHasReturnNode(statements);
This does not feel right to me. Closing a block should not add a return
statement behind the scene.

[hausner, 2015-03-31 19:45:59]

I moved the code that adds the return node out of the static function
Parser::ParseFunction() because I wanted it to be in an instance method that has
access to the function object being compiled. Rather than putting it into each
of the specialized ParseXXXX() methods, I moved it to CloseBlock(). It's maybe
not ideal.

The reason why EnsureHasReturnNode() sometimes has to check the child sequence
of the outermost block is that some ParseXXX functions put an implicit,
synthesized sequence around the whole function, enclosing the block that
contains the code of the function. In that case, we want the return statement in
the child, or else the return node could be at the wrong context level.

I'll try to make this all a bit more robust and explicit.

[regis, 2015-03-31 20:38:00]

On 2015/03/31 19:45:59, hausner wrote:
> I moved the code that adds the return node out of the static function
> Parser::ParseFunction() because I wanted it to be in an instance method that
has
> access to the function object being compiled. Rather than putting it into each
> of the specialized ParseXXXX() methods, I moved it to CloseBlock(). It's maybe
> not ideal.
> 
> The reason why EnsureHasReturnNode() sometimes has to check the child sequence
> of the outermost block is that some ParseXXX functions put an implicit,
> synthesized sequence around the whole function, enclosing the block that
> contains the code of the function. In that case, we want the return statement
in
> the child, or else the return node could be at the wrong context level.
> 
> I'll try to make this all a bit more robust and explicit.

Thanks for listening :-)

So, the ParseXXXX() methods know exactly in what node sequence to add a return,
since they are the ones wrapping the function body in a synthesized node
sequence. I think it would be much better to let them call
EnsureHasReturnNode(seq) with the correct seq passed as argument, even if the
call needs to be repeated in all ParseXXXX() methods.

[hausner, 2015-03-31 21:36:24]

PTAL

I think I addressed your comments and suggestions. I like this version better,
too.

[regis, 2015-03-31 21:49:03]

LGTM

Much better, indeed.

Thanks!

[hausner, 2015-03-31 22:11:12]

Committed patchset #2 (id:20001) manually as r44826 (presubmit successful).