Add parser support for generators.

Add parser support for generators.

This patchset begins by adding support for "yield", which is unlike other tokens
in JS. In a generator, whether strict or classic, it is a syntactic keyword.
In classic mode it is an identifier. In strict mode it is reserved.

This patch adds YIELD as a token to the scanner, and adapts the preparser and
parser appropriately. It also parses "function*", indicating that a function is
actually a generator, for both eagerly and lazily parsed functions.

Currently "yield" just compiles as "return".

BUG=v8:2355
TEST=mjsunit/harmony/generators-parsing

Committed: http://code.google.com/p/v8/source/detail?r=14116

[wingo, 2013-03-11 12:10:53]

https://codereview.chromium.org/12646003/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/12646003/diff/1/src/preparser.cc#newcode1123
src/preparser.cc:1123: case i::Token::YIELD:
This FIXME is to mark that this case and the FUTURE_RESERVED_WORD case and the
FUTURE_STRICT_RESERVED_WORD case can all be elided, it seems, as ParseIdentifier
(called later) will DTRT.  Am preparing a patch for that.  Apologies for letting
a FIXME through :)

[Michael Starzinger, 2013-03-11 15:42:10]

This CL definitely requires parsing tests. I would put them into a separate
mjsunit/harmony/generator-parsing.js file.

Also the parsing of generators should be put behind a separate
--harmony-generators flag in the flag-definitions.h file.

IIUC the changes in this CL are never executed because the "in_generator"
predicate is never set. If you wanna keep it split up as two CLs, it might make
more sense to land the part first that set's the predicate and then land this
one, so that stuff is testable. Otherwise this CL is dead code essentially from
a test coverage point of view, and we try to avoid this situation.

[wingo, 2013-03-12 09:43:37]

Thanks for the notes.  I was trying to show that parsing yield in this way would
not cause any change before going forward with things, but I suppose the tests
will catch that anyway.

I'll update this patch with the rest of things: the Yield AST node, the
is_generator() flag in FunctionLiteral and SharedFunctionInfo, and the parts of
the parser that actually build yield expressions.

[Michael Starzinger, 2013-03-14 22:29:24]

This is already looking pretty good. First round of comments. I didn't look at
the pre-parser yet, but I covered everything else.

https://codereview.chromium.org/12646003/diff/13001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/12646003/diff/13001/src/full-codegen.cc#newco...
src/full-codegen.cc:1552: // Unfortunately, it seems that the full-codegen
doesn't maintain a stack
Turn this comment into a TODO, it should vanish once yield expressions are
implemented.

https://codereview.chromium.org/12646003/diff/13001/src/full-codegen.cc#newco...
src/full-codegen.cc:1556: // What follows is as in VisitReturnStatement.
Also turn this comment into a TODO, as this is only a temporary implementation.

https://codereview.chromium.org/12646003/diff/13001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12646003/diff/13001/src/hydrogen.cc#newcode4817
src/hydrogen.cc:4817: void HOptimizedGraphBuilder::VisitYield(Yield* expr) {
Move this method away from the statement visitors to the expression visitors,
e.g. in front of VisitThrow for consistency.

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc#newcode1901
src/parser.cc:1901: //   'function' Identifier '(' FormalParameterListopt ')'
'{' FunctionBody '}'
Could we add a comment like the following here ...

// GeneratorDeclaration ::
//   'function' '*' Identifier '(' FormalParameter ')' '{' FunctionBody '}'

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc#newcode4389
src/parser.cc:4389: //    '(' (Identifier)*[','] ')'
We need test cases for the following generator constructs. I think we are
getting it right, but we should have test coverage for it.

function* g(yield) { yield 0; }

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc#newcode4790
src/parser.cc:4790: (next == Token::YIELD && !inside_generator())))) {
I think it would be clearer here if we would call the is_generator() predicate
directly on top_scope_ instead of using a helper. That makes it explicit that
this information is carried along with the scope.

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc#newcode4808
src/parser.cc:4808: (next == Token::YIELD && !inside_generator())) {
Likewise.

https://codereview.chromium.org/12646003/diff/13001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/12646003/diff/13001/src/parser.h#newcode713
src/parser.h:713: bool inside_generator() const { return
top_scope_->inside_generator(); }
See comment in ParseIdentifier(), I think we should drop this helper completely.
It might lead to confusion.

https://codereview.chromium.org/12646003/diff/13001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/12646003/diff/13001/src/scopes.cc#newcode254
src/scopes.cc:254: // Find the deepest scope that's still in this function.
Instead of searching for the next function scope down the chain, we could just
keep track of said function scope in a next_function_scope variable. Note that I
only jotted this down without actually trying it, so take it with a grain of
salt.

At top of function:
  Scope* next_function_scope = NULL;

Here:
  // All the inner scopes up to the next function scope are inside this
  // generator if we are deserializing a generator scope.
  if (context->closure()->shared()->is_generator()) {
    current_scope->scope_inside_generator_ = true;
    Scope* inner_scope = (next_function_scope != NULL)
        ? next_function_scope->outer_scope() : innermost_scope;
    for (Scope* s = inner_scope; s != NULL; s = s->outer_scope()) {
      s->scope_inside_generator_ = true;
    }
  }
  next_function_scope = current_scope;

https://codereview.chromium.org/12646003/diff/13001/src/scopes.cc#newcode343
src/scopes.cc:343: scope_inside_generator_ = outer_scope_->inside_generator();
We should only inherit this property from the outer scope if we are not
initializing a function scope. Otherwise this property is first set here and
later overridden by the parser again, that is confusing (it took me a while to
figure out why it works at all). How about the following instead ...

scope_inside_generator_ = outer_scope_->inside_generator() &&
!is_function_scope();

https://codereview.chromium.org/12646003/diff/13001/src/scopes.cc#newcode908
src/scopes.cc:908: if (scope_contains_with_) Indent(n1, "// scope contains
'with'\n");
We should add the following debug output here for --print-scopes to be usable
with generators ...

if (scope_inside_generator_) Indent(n1, "// scope inside 'function*'\n");

https://codereview.chromium.org/12646003/diff/13001/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-parsing.js (right):

https://codereview.chromium.org/12646003/diff/13001/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-parsing.js:1: // Copyright 2012 the V8 project
authors. All rights reserved.
Should say 2013 here.

[Michael Starzinger, 2013-03-14 22:48:23]

The more I think about it ...

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/12646003/diff/13001/src/parser.cc#newcode4368
src/parser.cc:4368: scope->set_inside_generator(is_generator);
Actually, the more I think about it, is it really necessary that this
information is carried along with the scope? Wouldn't it be sufficient to store
this predicate as part of the FunctionState?

That way we wouldn't need to deserialize that information and scope
initialization would be unaffected. The predicate would be available via
current_function_state_ and I don't see the need to carry it along to other
units of compilation through the context chain. WDYT?

[wingo, 2013-03-15 10:03:33]

Thanks for the review.  I believe I addressed all of your comments in the new
patchset.  It also includes a preparser bug fix that was not caught by the
tests.

I kept the in_generator() helper, as current_function_state_->in_generator() was
very long.  I can remove the helper if you still think that's a good idea.

[wingo, 2013-03-28 15:28:59]

On 2013/03/15 10:03:33, wingo wrote:
> Thanks for the review.  I believe I addressed all of your comments in the new
> patchset.  It also includes a preparser bug fix that was not caught by the
> tests.
> 
> I kept the in_generator() helper, as current_function_state_->in_generator()
was
> very long.  I can remove the helper if you still think that's a good idea.

See https://codereview.chromium.org/13179002/ for a rebased version.  I
published as a new CL so as not to disrupt the delta view.

[Michael Starzinger, 2013-04-02 11:08:54]

LGTM with two final comments. I will land this for you as soon as those two
comments are addressed.

The patch seems to apply cleanly, so no need to rebase, I can do this while
landing. But editing the description of this CL to contain the updated
description might be a good idea.

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc#newcode304
src/preparser.cc:304: bool is_generator = Check(i::Token::MUL);
This also needs to go behind the harmony-generators flag.

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc#newcode1064
src/preparser.cc:1064: bool is_generator = Check(i::Token::MUL);
This also needs to go behind the harmony-generators flag.

[wingo, 2013-04-02 15:56:01]

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc#newcode304
src/preparser.cc:304: bool is_generator = Check(i::Token::MUL);
On 2013/04/02 11:08:55, Michael Starzinger wrote:
> This also needs to go behind the harmony-generators flag.
Done.  Unfortunately the preparser has no direct access to flags, so there was
some more plumbing to do.

https://codereview.chromium.org/12646003/diff/25001/src/preparser.cc#newcode1064
src/preparser.cc:1064: bool is_generator = Check(i::Token::MUL);
On 2013/04/02 11:08:55, Michael Starzinger wrote:
> This also needs to go behind the harmony-generators flag.

Done.

[Michael Starzinger, 2013-04-02 17:35:26]

Committed patchset #7 manually as r14116 (presubmit successful).

[arv (Not doing code reviews), 2013-04-04 20:56:11]

Drive by...

https://codereview.chromium.org/12646003/diff/45003/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-parsing.js (right):

https://codereview.chromium.org/12646003/diff/45003/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-parsing.js:77: assertThrows("function f*() {
yield: 1 }", SyntaxError)
This should be function* f()

[Michael Starzinger, 2013-04-05 08:36:04]

https://codereview.chromium.org/12646003/diff/45003/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-parsing.js (right):

https://codereview.chromium.org/12646003/diff/45003/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-parsing.js:77: assertThrows("function f*() {
yield: 1 }", SyntaxError)
On 2013/04/04 20:56:11, arv wrote:
> This should be function* f()

See https://codereview.chromium.org/13575010/ for the fix.