[es6] Implement for-of iterator finalization

[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got completely out of hand after this change (the new bytecode has 150+ lines). See the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2 relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y

Committed: https://crrev.com/cb1bf4af3c961093e9a75a22089f78daf7959304
Cr-Commit-Position: refs/heads/master@{#34111}

[rossberg, 2016-02-16 16:23:16]

Description was changed from

==========
[WIP] for-of iterator finalization

BUG=

patch from issue 1695583003 at patchset 1 (http://crrev.com/1695583003#ps1)
==========

to

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

[rossberg, 2016-02-16 16:23:16]

rossberg@chromium.org changed reviewers:
+ dehrenberg@chromium.org, rmcilroy@chromium.org

[rossberg, 2016-02-16 16:27:40]

Description was changed from

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

to

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

[rossberg, 2016-02-16 16:29:19]

PTAL

[rossberg, 2016-02-16 16:59:39]

Georg, FYI.

[Dan Ehrenberg, 2016-02-16 21:18:29]

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

[Dan Ehrenberg, 2016-02-16 21:18:30]

https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.cc
File src/ast/prettyprinter.cc (right):

https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.c...
src/ast/prettyprinter.cc:1568: break;
Minor: Did git cl format add these indents? It's a little surprising.

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

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3779: : loop;
I'm not sure if any of this is risky (the nopos part?); if so, we may want to do
it only behind a flag.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6318: // Here, |...| denotes optional parts, depending on
the presence of the
Given the below comment (iteratorReturn shouldn't be passed input per spec), the
caller could set input to undefined when it's not provided, rather than making
this a Maybe.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6362: // output = %_Call(iteratorReturn, iterator|,
input|);
Not sure why you're passing input to return. This doesn't seem to be there in
the spec: IteratorClose always calls the "return" method with no arguments:
http://tc39.github.io/ecma262/#sec-iteratorclose

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6414: enum ForOfLoopCompletion { THROW, RETURN_OR_BREAK,
NORMAL };
This seems to correspond to JSGeneratorObject::ResumeMode. Can you reuse that
somehow (maybe putting it elsewhere if the names feel funny)?

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6475: Token::EQ_STRICT, type_of, function_literal, nopos);
FYI mvstanton@: Here's how you can do IsCallable in a desugaring in an
optimizable way.

Seems like multiple desugarings may need to do this IsCallable/throw type error
thing (also @@hasInstance). Should we factor this out?

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6577: // if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) { ...
}
Nit: All your other comments fully say what statements are included, except this
one. It's so pretty in general! Keep it up!

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6586: block->statements()->Add(validate_output, zone);  //
GAGA
GAGA? I don't really see anything funny about this code.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6622: const int nopos = RelocInfo::kNoPosition;
This desugaring uses nopos for everything. Is that really OK? Do we maybe want
to ascribe things to the beginning or end of the block, position-wise?

Style nit: Usually, I like putting things in variables, but here, I think it's
nice to see explicit references to RelocInfo::kNoPosition; it makes it more
clear what's going on, and you can grep for it to find where we're skimping on
positions. It's just a single long name, not a method call or anything, so I
think it makes sense to repeat around.

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:1067: assertThrows("'use strict'; let x =
{}; for (let [y, x] in [x]);", ReferenceError);
Why delete the old tests? I also don't understand what in this patch changes
that has to do with throwing this ReferenceError.

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/it...
File test/mjsunit/harmony/iterator-close.js (right):

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/it...
test/mjsunit/harmony/iterator-close.js:226: }
Can you write some tests to ensure that at least some of these cases work when
you have nested for loops, to ensure that scoping for various variables (the
anonymous var_completion, .catch, etc) works out in a properly nested way? In
particular, testing that IteratorClose gets called for the inner or outer one as
appropriate, and the right exception gets propagated.

[adamk, 2016-02-16 22:00:53]

adamk@chromium.org changed reviewers:
- dehrenberg@chromium.org

[mvstanton, 2016-02-16 22:55:46]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2016-02-16 22:55:46]

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

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6475: Token::EQ_STRICT, type_of, function_literal, nopos);
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> FYI mvstanton@: Here's how you can do IsCallable in a desugaring in an
> optimizable way.
> 
> Seems like multiple desugarings may need to do this IsCallable/throw type
error
> thing (also @@hasInstance). Should we factor this out?

Thanks Dan! Over in my CL I'll put this into a method in parser.cc for handy
reuse...

[rossberg, 2016-02-17 12:54:45]

Addressed comments. Also fixed behaviour wrt exceptions produced by the .next
method, which should _not_ trigger IteratorClose.

https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.cc
File src/ast/prettyprinter.cc (right):

https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.c...
src/ast/prettyprinter.cc:1568: break;
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Minor: Did git cl format add these indents? It's a little surprising.

No, nothing changed with that, see diff. What do you find surprising about them?

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

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3779: : loop;
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> I'm not sure if any of this is risky (the nopos part?); if so, we may want to
do
> it only behind a flag.

Good point, added flag.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6318: // Here, |...| denotes optional parts, depending on
the presence of the
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Given the below comment (iteratorReturn shouldn't be passed input per spec),
the
> caller could set input to undefined when it's not provided, rather than making
> this a Maybe.

Done.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6362: // output = %_Call(iteratorReturn, iterator|,
input|);
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Not sure why you're passing input to return. This doesn't seem to be there in
> the spec: IteratorClose always calls the "return" method with no arguments:
> http://tc39.github.io/ecma262/#sec-iteratorclose

Fixed.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6414: enum ForOfLoopCompletion { THROW, RETURN_OR_BREAK,
NORMAL };
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> This seems to correspond to JSGeneratorObject::ResumeMode. Can you reuse that
> somehow (maybe putting it elsewhere if the names feel funny)?

Here it only talks about the completion of the loop body. Also, it cannot
distinguish RETURN and BREAK, which would make it a bit misleading to reuse the
other. Changed names to clarify.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6475: Token::EQ_STRICT, type_of, function_literal, nopos);
On 2016/02/16 22:55:46, mvstanton wrote:
> On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> > FYI mvstanton@: Here's how you can do IsCallable in a desugaring in an
> > optimizable way.
> > 
> > Seems like multiple desugarings may need to do this IsCallable/throw type
> error
> > thing (also @@hasInstance). Should we factor this out?
> 
> Thanks Dan! Over in my CL I'll put this into a method in parser.cc for handy
> reuse...

Makes sense. Okay, whoever lands second should remember to change these lines to
use your method.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6577: // if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) { ...
}
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Nit: All your other comments fully say what statements are included, except
this
> one. It's so pretty in general! Keep it up!

Actually, not true. The convention (see also RewriteYieldStar) is to use dots
for the parts that are not actually constructed in this block. I think that
makes sense.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6586: block->statements()->Add(validate_output, zone);  //
GAGA
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> GAGA? I don't really see anything funny about this code.

I missed that one, removed. You'll have to ask Georg what it meant.

https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6622: const int nopos = RelocInfo::kNoPosition;
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> This desugaring uses nopos for everything. Is that really OK? Do we maybe want
> to ascribe things to the beginning or end of the block, position-wise?
> 
> Style nit: Usually, I like putting things in variables, but here, I think it's
> nice to see explicit references to RelocInfo::kNoPosition; it makes it more
> clear what's going on, and you can grep for it to find where we're skimping on
> positions. It's just a single long name, not a method call or anything, so I
> think it makes sense to repeat around.

This is done in various other functions already. I think it makes sense to save
on the verbosity.

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:1067: assertThrows("'use strict'; let x =
{}; for (let [y, x] in [x]);", ReferenceError);
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Why delete the old tests? I also don't understand what in this patch changes
> that has to do with throwing this ReferenceError.

Just a tweak. The old version didn't actually make sense for for-of, because
objects aren't iterable. Reverted for the for-in ones.

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/it...
File test/mjsunit/harmony/iterator-close.js (right):

https://codereview.chromium.org/1695393003/diff/40001/test/mjsunit/harmony/it...
test/mjsunit/harmony/iterator-close.js:226: }
On 2016/02/16 21:18:30, Dan Ehrenberg wrote:
> Can you write some tests to ensure that at least some of these cases work when
> you have nested for loops, to ensure that scoping for various variables (the
> anonymous var_completion, .catch, etc) works out in a properly nested way? In
> particular, testing that IteratorClose gets called for the inner or outer one
as
> appropriate, and the right exception gets propagated.

Done.

[Dan Ehrenberg, 2016-02-17 15:42:01]

Oh yeah, I remember now Georg explaining that he needed a try/catch on every
iteration to make try exceptions propagate. I can't find it in the code--what
did you do to prevent that?

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

https://codereview.chromium.org/1695393003/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6342: 
Nit: no need for a blank line.

https://codereview.chromium.org/1695393003/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6356: 
Here too

[rossberg, 2016-02-17 16:04:07]

I change the completion flag between .next call and loop body.

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

https://codereview.chromium.org/1695393003/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6342: 
On 2016/02/17 15:42:00, Dan Ehrenberg wrote:
> Nit: no need for a blank line.

Removed various double empty lines, here and in other pre-existing functions.

[Dan Ehrenberg, 2016-02-17 16:09:51]

lgtm

[Dan Ehrenberg, 2016-02-17 16:10:01]

lgtm

[rossberg, 2016-02-17 17:14:22]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2016-02-17 17:14:29]

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

[commit-bot: I haz the power, 2016-02-17 17:33:32]

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

[commit-bot: I haz the power, 2016-02-17 17:33:33]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[rossberg, 2016-02-18 10:27:52]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2016-02-18 10:27:53]

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/1695393003/#ps120001
(title: "Disable iterator test for Ignition")

[commit-bot: I haz the power, 2016-02-18 10:28:10]

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

[commit-bot: I haz the power, 2016-02-18 10:49:10]

Description was changed from

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

to

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

[commit-bot: I haz the power, 2016-02-18 10:49:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-18 10:50:02]

Description was changed from

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y
==========

to

==========
[es6] Implement for-of iterator finalization

Implements iterator finalisation by desugaring for-of loops with an additional
try-finally wrapper. See comment in parser.cc for details.

Also improved some AST printing facilities while there.

@Ross, I had to disable the bytecode generation test for for-of, because it got
completely out of hand after this change (the new bytecode has 150+ lines). See
the TODO that I assigned to you.

Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2
relative changes.

@Georg, FYI, I changed the following:

- Moved try-finally out of the loop body, for performance, and in order to be
able to handle `continue` correctly.
- Fixed scope management in ParseForStatement, which was the cause for the
variable allocation failure.
- Fixed pre-existing zone initialisation bug in rewriter, which caused the
crashes.
- Enabled all tests, adjusted a few others, added a couple more.

BUG=v8:2214
LOG=Y

Committed: https://crrev.com/cb1bf4af3c961093e9a75a22089f78daf7959304
Cr-Commit-Position: refs/heads/master@{#34111}
==========

[commit-bot: I haz the power, 2016-02-18 10:50:03]

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