Implement iterator finalization in array destructuring.

Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless an iterator operation (eg. next) threw.  We do this by wrapping the iterator use in a try-catch-finally similar to the desugaring of for-of.

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y

Committed: https://crrev.com/3062af70eb8e7dc49501981237c658ff5a861780
Cr-Commit-Position: refs/heads/master@{#34654}

[neis, 2016-03-07 17:42:00]

Hi Adam,

there are a few cosmetic changes that I want to do tomorrow.  Besides that, I
haven't looked at all at destructuring in function parameters.  Maybe that is
already covered by my changes, maybe not.  Will look into that tomorrow.

[adamk, 2016-03-07 20:28:05]

First round of comments (as noted in several places, this patch is pretty big so
my review may have missed things).

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6901: Expression* condition;
Can you give this a better name? Maybe "iterator_close_condition"?

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:712: enum CompletionKind {
Can you make this an enum class? Otherwise you've effectively added a bunch of
bare names as valid identifiers in all 7000 lines of parser.cc.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:718: ABRUPT = THROW | BREAK | CONTINUE | RETURN
Besides this line, do you actually use this enum as a bit pattern? I didn't see
anyplace on first run through, which suggests to me that it shouldn't be one.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:448: factory()->NewBlock(nullptr, 2, true,
nopos);
Can you leave stylistic changes out of this patch? The diff is already pretty
big, and this just makes the change harder to review (for me, anyway).

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:521: // while (!done) {
This change is already giant (and thus tricky to review); could you leave the
call out to %concat_iterable_to_array() in this one?

We'll always exhaust the iterator when we hit a spread, so I don't think there's
any interaction between this stuff and the new code you're adding for iterator
close.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:525: // <.completion = ABRUPT>
This doesn't seem right to me. The recursion is over a whole different iterator,
which will be exhausted by the time we get here. Won't that be handled by a
generated try-finally inside the recurision?

https://codereview.chromium.org/1772793002/diff/1/test/mjsunit/harmony/iterat...
File test/mjsunit/harmony/iterator-close.js (right):

https://codereview.chromium.org/1772793002/diff/1/test/mjsunit/harmony/iterat...
test/mjsunit/harmony/iterator-close.js:5: // Flags: --harmony-iterator-close
There are a ton of new tests in here, but I don't on first reading see any that
have errors on the LHS. For example, this case:

  log = []
  x = { set attr(_) { throw 1234; } };
  assertThrowsEquals(() => {
    [x.attr] = g();
  }, 1234);
  assertEquals([[]], log);

I think your patch probably does the right thing, but it would be good to
exercise such cases.

[adamk, 2016-03-08 01:57:07]

A few more comments.

Also, re: function parameters, I think this patch should be sufficient to handle
them.

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6576: // following code:
It's somewhat confusing that this code contains a single try/catch/finally but
that the desugaring below creates a TryCatch and a TryFinally. I realize that
has to do with how those AST nodes work, but I think it might make reading the
desugaring easier if the block below matched up precisely with the code
generated as part of the desugaring.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6592: auto factory = parser_->factory();
If you put this function in Parser instead of ParserTraits, you wouldn't need
this or the other shorthands below (though I guess you might run into trouble
since BuildIteratorCloseForCompletion is itself in ParserTraits).

Maybe something to keep in mind for a cleanup.

[neis, 2016-03-08 13:40:10]

Thanks for the comments.  I uploaded a new patch set.

Two points to note:

- There was a 'done = true' missing in order to not call return when next or
such throws.

- The existing GetIterator function in parser.cc does not check if the obtained
result is an object.  The four or five call-sites do that manually afterwards,
except for the pattern-rewriter code.  In my CL I didn't include such a check
because it accidentally does the right thing in that case (a TypeError is
thrown).  I suggest that I add the check to GetIterator in a followup CL.

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6576: // following code:
On 2016/03/08 01:57:07, adamk wrote:
> It's somewhat confusing that this code contains a single try/catch/finally but
> that the desugaring below creates a TryCatch and a TryFinally. I realize that
> has to do with how those AST nodes work, but I think it might make reading the
> desugaring easier if the block below matched up precisely with the code
> generated as part of the desugaring.

Done.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6901: Expression* condition;
On 2016/03/07 20:28:05, adamk wrote:
> Can you give this a better name? Maybe "iterator_close_condition"?

Done.

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:712: enum CompletionKind {
On 2016/03/07 20:28:05, adamk wrote:
> Can you make this an enum class? Otherwise you've effectively added a bunch of
> bare names as valid identifiers in all 7000 lines of parser.cc.

If I make it an enum class, then instead of eg NORMAL etc. I will have to write
static_cast<int>(CompletionKind::NORMAL).  So I prefer keeping it as is.  I made
it private, though (there was no reason for it to be public).

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:718: ABRUPT = THROW | BREAK | CONTINUE | RETURN
On 2016/03/07 20:28:05, adamk wrote:
> Besides this line, do you actually use this enum as a bit pattern? I didn't
see
> anyplace on first run through, which suggests to me that it shouldn't be one.

Alright, changed it back to three values.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:448: factory()->NewBlock(nullptr, 2, true,
nopos);
On 2016/03/07 20:28:05, adamk wrote:
> Can you leave stylistic changes out of this patch? The diff is already pretty
> big, and this just makes the change harder to review (for me, anyway).

Ok, I removed them.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:521: // while (!done) {
On 2016/03/07 20:28:05, adamk wrote:
> This change is already giant (and thus tricky to review); could you leave the
> call out to %concat_iterable_to_array() in this one?
> 
> We'll always exhaust the iterator when we hit a spread, so I don't think
there's
> any interaction between this stuff and the new code you're adding for iterator
> close.

Except that 'done' must be set to true at the end.  I changed the code back to
using concat_iterable_to_array and added an extra assignment.

https://codereview.chromium.org/1772793002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:525: // <.completion = ABRUPT>
On 2016/03/07 20:28:05, adamk wrote:
> This doesn't seem right to me. The recursion is over a whole different
iterator,
> which will be exhausted by the time we get here. Won't that be handled by a
> generated try-finally inside the recurision?

Oops, these two assignments were wrong here indeed.

https://codereview.chromium.org/1772793002/diff/1/test/mjsunit/harmony/iterat...
File test/mjsunit/harmony/iterator-close.js (right):

https://codereview.chromium.org/1772793002/diff/1/test/mjsunit/harmony/iterat...
test/mjsunit/harmony/iterator-close.js:5: // Flags: --harmony-iterator-close
On 2016/03/07 20:28:05, adamk wrote:
> There are a ton of new tests in here, but I don't on first reading see any
that
> have errors on the LHS. For example, this case:
> 
>   log = []
>   x = { set attr(_) { throw 1234; } };
>   assertThrowsEquals(() => {
>     [x.attr] = g();
>   }, 1234);
>   assertEquals([[]], log);
> 
> I think your patch probably does the right thing, but it would be good to
> exercise such cases.

Yes, definitely.  I added more.

[adamk, 2016-03-08 18:33:08]

Description was changed from

==========
Implement iterator finalization in destructuring.

R=adamk@chromium.org
BUG=
==========

to

==========
Implement iterator finalization in destructuring.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

[adamk, 2016-03-08 18:41:13]

Please add more to the CL description explaining the approach taken here. With
that and the two other notes below, this patch lgtm.

I am curious whether it would be cleaner to replace most of the iterator
interaction in PatternRewriter with a ForOf loop, similar to what Nikolaos did
for array spreads. Did you give that a try and run into problems, or just decide
to take a different path?

The good news is with all the tests added, it should be easy to refactor this
later.

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

https://codereview.chromium.org/1772793002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:712: enum CompletionKind {
On 2016/03/08 13:40:10, neis wrote:
> On 2016/03/07 20:28:05, adamk wrote:
> > Can you make this an enum class? Otherwise you've effectively added a bunch
of
> > bare names as valid identifiers in all 7000 lines of parser.cc.
> 
> If I make it an enum class, then instead of eg NORMAL etc. I will have to
write
> static_cast<int>(CompletionKind::NORMAL).  So I prefer keeping it as is.  I
made
> it private, though (there was no reason for it to be public).

Oh, I forgot about that bit. The thing that bothers me is actually the
shortness, not the typing; "NORMAL" is an especially strange identifier to add
if it means something as specific as non-abrupt completion. How about:

kNormalCompletion
kThrowCompletion
kAbruptCompletion

"kThrowCompletion" is a little bit funny, but the others seem reasonable. WDYT?

https://codereview.chromium.org/1772793002/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1772793002/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:450: factory()->NewBlock(nullptr, 2, true,
RelocInfo::kNoPosition);
This block now contains 3 statements.

[neis, 2016-03-10 08:32:05]

Description was changed from

==========
Implement iterator finalization in destructuring.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

to

==========
Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless
an iterator operation (eg. next) threw.  We do this by wrapping the iterator use
in a try-catch-finally similar to the desugaring of for-of.  

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

[neis, 2016-03-10 08:32:22]

The CQ bit was checked by neis@chromium.org

[neis, 2016-03-10 08:32:22]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1772793002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-10 08:32:30]

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

[commit-bot: I haz the power, 2016-03-10 09:33:38]

Description was changed from

==========
Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless
an iterator operation (eg. next) threw.  We do this by wrapping the iterator use
in a try-catch-finally similar to the desugaring of for-of.  

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

to

==========
Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless
an iterator operation (eg. next) threw.  We do this by wrapping the iterator use
in a try-catch-finally similar to the desugaring of for-of.  

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

[commit-bot: I haz the power, 2016-03-10 09:33:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-10 09:34:46]

Description was changed from

==========
Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless
an iterator operation (eg. next) threw.  We do this by wrapping the iterator use
in a try-catch-finally similar to the desugaring of for-of.  

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y
==========

to

==========
Implement iterator finalization in array destructuring.

We must close the iterator whenever the destructuring didn't exhaust it, unless
an iterator operation (eg. next) threw.  We do this by wrapping the iterator use
in a try-catch-finally similar to the desugaring of for-of.

This is behind --harmony-iterator-close.

R=adamk@chromium.org
BUG=v8:3566
LOG=Y

Committed: https://crrev.com/3062af70eb8e7dc49501981237c658ff5a861780
Cr-Commit-Position: refs/heads/master@{#34654}
==========

[commit-bot: I haz the power, 2016-03-10 09:34:47]

Patchset 5 (id:??) landed as
https://crrev.com/3062af70eb8e7dc49501981237c658ff5a861780
Cr-Commit-Position: refs/heads/master@{#34654}

[neis, 2016-03-10 11:44:43]

On 2016/03/08 18:41:13, adamk wrote:
> Please add more to the CL description explaining the approach taken here. With
> that and the two other notes below, this patch lgtm.

Done.
 
> I am curious whether it would be cleaner to replace most of the iterator
> interaction in PatternRewriter with a ForOf loop, similar to what Nikolaos did
> for array spreads. Did you give that a try and run into problems, or just
decide
> to take a different path?

You mean along the lines of what you wrote in your old CL?  When I saw that, I
was already in the middle of what I submitted now.  And I thought there was a
problem with rest-patterns in that approach, but now I'm thinking maybe there
isn't.  My feeling is that if it works, then your approach is cleaner but
requires more changes.

[adamk, 2016-03-10 18:49:04]

On 2016/03/10 11:44:43, neis wrote:
> On 2016/03/08 18:41:13, adamk wrote:
> > Please add more to the CL description explaining the approach taken here.
With
> > that and the two other notes below, this patch lgtm.
> 
> Done.
>  
> > I am curious whether it would be cleaner to replace most of the iterator
> > interaction in PatternRewriter with a ForOf loop, similar to what Nikolaos
did
> > for array spreads. Did you give that a try and run into problems, or just
> decide
> > to take a different path?
> 
> You mean along the lines of what you wrote in your old CL?  When I saw that, I
> was already in the middle of what I submitted now.  And I thought there was a
> problem with rest-patterns in that approach, but now I'm thinking maybe there
> isn't.  My feeling is that if it works, then your approach is cleaner but
> requires more changes.

Okay. If I get some spare time and you don't mind I may go back and re-implement
this with for-of just to avoid the duplicated iterator logic.