[generators] Desugar yield*.

[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=

Committed: https://crrev.com/5269944a18adf88577aa4787586cd8ca92510686
Cr-Commit-Position: refs/heads/master@{#33735}

[neis, 2016-01-28 10:28:16]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 10:28:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1643903003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1643903003/1

[commit-bot: I haz the power, 2016-01-28 11:05:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 11:05:38]

Dry run: This issue passed the CQ dry run.

[neis, 2016-01-28 11:15:13]

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6320: // crashing the rewriter.
When just using one normal block, the rewriter crashes because the try-finally
statement's scope is a null pointer (if I remember correctly).

Maybe it's actually a good idea to have this init block here because having the
rewriter go through it would just be a waste of time anyway.

[neis, 2016-01-28 11:15:29]

Description was changed from

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=
==========

to

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=
==========

[neis, 2016-01-28 11:15:29]

neis@chromium.org changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org, rossberg@chromium.org

[neis, 2016-01-28 11:15:45]

PTAL

[Michael Starzinger, 2016-01-29 10:15:40]

Approach looking good. But I am not a parser expert, please wait for sign-off
from a parser owner.

[rossberg, 2016-01-29 18:12:28]

Mostly looking good.

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5804: // loop containing an "raw" yield (a yield that
doesn't wrap its argument).
Nit: s/an/a/

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5835: //           if
(IS_NULL_OR_UNDEFINED(iteratorThrow)) {
Hm, this logic is so random. Why close the iterator when interator.throw is
null, but not when it is, say, 0? Oh JavaScript....

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5850: //           RawYield(output);
What's RawYield?

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6006: Variable* var_return =
scope->NewTemporary(avfactory->empty_string());
Nit: I suppose you could reuse some of the temporaries.

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6320: // crashing the rewriter.
On 2016/01/28 11:15:13, neis wrote:
> When just using one normal block, the rewriter crashes because the try-finally
> statement's scope is a null pointer (if I remember correctly).

Is there a bug for that?

> Maybe it's actually a good idea to have this init block here because having
the
> rewriter go through it would just be a waste of time anyway.

https://codereview.chromium.org/1643903003/diff/1/test/mjsunit/es6/generators...
File test/mjsunit/es6/generators-iteration.js (left):

https://codereview.chromium.org/1643903003/diff/1/test/mjsunit/es6/generators...
test/mjsunit/es6/generators-iteration.js:400: // Test that yield* re-yields
received results without re-boxing.
Why did this test go away?

[neis, 2016-01-31 19:21:50]

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5850: //           RawYield(output);
On 2016/01/29 18:12:27, rossberg wrote:
> What's RawYield?

It's explained in the comment above.

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6006: Variable* var_return =
scope->NewTemporary(avfactory->empty_string());
On 2016/01/29 18:12:27, rossberg wrote:
> Nit: I suppose you could reuse some of the temporaries.

You mean e.g. merge var_return, var_throw, var_next?  Sure, will do.

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6320: // crashing the rewriter.
On 2016/01/29 18:12:27, rossberg wrote:
> On 2016/01/28 11:15:13, neis wrote:
> > When just using one normal block, the rewriter crashes because the
try-finally
> > statement's scope is a null pointer (if I remember correctly).
> 
> Is there a bug for that?

I didn't check.  I wasn't sure if it's the rewriter's fault or if I'm doing
something wrong when creating the nodes.

https://codereview.chromium.org/1643903003/diff/1/test/mjsunit/es6/generators...
File test/mjsunit/es6/generators-iteration.js (left):

https://codereview.chromium.org/1643903003/diff/1/test/mjsunit/es6/generators...
test/mjsunit/es6/generators-iteration.js:400: // Test that yield* re-yields
received results without re-boxing.
On 2016/01/29 18:12:27, rossberg wrote:
> Why did this test go away?

I replaced it with one that checks the correct behavior.

[Jarin, 2016-02-01 07:44:36]

It is looking good, but I know nearly nothing about parser.

[adamk, 2016-02-01 21:38:42]

Drive-by high-level comment/question...

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6339: }
This function is almost 500 lines long. Should we consider adding some
infrastructure to make writing (and reading) these desugarings easier?

[neis, 2016-02-03 08:33:09]

Patchset #2 (id:20001) has been deleted

[neis, 2016-02-03 10:10:34]

>
https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
> src/parsing/parser.cc:6320: // crashing the rewriter.
> On 2016/01/29 18:12:27, rossberg wrote:
> > On 2016/01/28 11:15:13, neis wrote:
> > > When just using one normal block, the rewriter crashes because the
> try-finally
> > > statement's scope is a null pointer (if I remember correctly).
> > 
> > Is there a bug for that?
> 
> I didn't check.  I wasn't sure if it's the rewriter's fault or if I'm doing
> something wrong when creating the nodes.

Looked into this again.  The rewriter assumes it was created with a valid scope.
 But it was created with the scope of the do-block and the default scope of a
Block is nullptr.  So the rewriter crashes.

I can now either set the block's scope to something valid and then let the
rewriter walk through it, or I can keep it the way it is.  I prefer the second
because running the rewriter on the whole block is unnecessary---it only needs
to rewrite the last expression.

[neis, 2016-02-04 09:36:37]

neis@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2016-02-04 11:40:32]

lgtm

Really clean code; I wish the whole parser had this level of comments. Just a
couple nits.

https://codereview.chromium.org/1643903003/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1643903003/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:5885: enum { kNext, kReturn, kThrow };
This looks redundant with JSGeneratorObject::ResumeMode and must be kept in sync
with it. Make a reference to that instead?

https://codereview.chromium.org/1643903003/diff/100001/test/mjsunit/es6/gener...
File test/mjsunit/es6/generators-iteration.js (left):

https://codereview.chromium.org/1643903003/diff/100001/test/mjsunit/es6/gener...
test/mjsunit/es6/generators-iteration.js:430: assertEquals(expected,
collect_results(yield_results(expected)));
Rather than just deleting the old test and replacing it with the new error
checks, would it make sense to somehow run it but assert that the intended thing
happens (an error is thrown at the right point) in addition to the new tests?

[rossberg, 2016-02-04 13:21:41]

lgtm

[neis, 2016-02-04 13:44:08]

The CQ bit was checked by neis@chromium.org

[neis, 2016-02-04 13:44:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1643903003/#ps140001
(title: "Refer to ResumeMode")

[commit-bot: I haz the power, 2016-02-04 13:44:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1643903003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1643903003/140001

[commit-bot: I haz the power, 2016-02-04 14:12:41]

Description was changed from

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=
==========

to

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=
==========

[commit-bot: I haz the power, 2016-02-04 14:12:41]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-02-04 14:13:10]

Description was changed from

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=
==========

to

==========
[generators] Desugar yield*.

This CL deals with yield* by desugaring it in the parser.  Hence the
full-codegen implementation of it becomes obsolete and can be removed in a
future CL.

The only change in semantics should be that the results of the iterator's next
and throw methods are checked to be objects, which didn't happen before but is
required by the spec.

BUG=

Committed: https://crrev.com/5269944a18adf88577aa4787586cd8ca92510686
Cr-Commit-Position: refs/heads/master@{#33735}
==========

[commit-bot: I haz the power, 2016-02-04 14:13:11]

Patchset 7 (id:??) landed as
https://crrev.com/5269944a18adf88577aa4787586cd8ca92510686
Cr-Commit-Position: refs/heads/master@{#33735}

[neis, 2016-02-05 08:33:19]

On 2016/02/01 21:38:42, adamk (ooo until feb 10) wrote:
> Drive-by high-level comment/question...
> 
> https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
> File src/parsing/parser.cc (right):
> 
>
https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
> src/parsing/parser.cc:6339: }
> This function is almost 500 lines long. Should we consider adding some
> infrastructure to make writing (and reading) these desugarings easier?

Do you have concrete ideas for that?  If not, maybe a good time to do this kind
of thing is when the parser rewrite is on the agenda again.

[adamk, 2016-02-10 18:36:20]

On 2016/02/05 08:33:19, neis wrote:
> On 2016/02/01 21:38:42, adamk (ooo until feb 10) wrote:
> > Drive-by high-level comment/question...
> > 
> > https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc
> > File src/parsing/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/1643903003/diff/1/src/parsing/parser.cc#newco...
> > src/parsing/parser.cc:6339: }
> > This function is almost 500 lines long. Should we consider adding some
> > infrastructure to make writing (and reading) these desugarings easier?
> 
> Do you have concrete ideas for that?  If not, maybe a good time to do this
kind
> of thing is when the parser rewrite is on the agenda again.

Not super concrete, no; I imagine we'd build some helper classes that would do
the AST generation but have an interface that's less verbose than the
AstNodeFactory (and the nodes themselves). Putting it on the "parser rewrite"
agenda seems reasonable.