Baseline for-of implementation

Baseline for-of implementation

Add full-codegen support for the ES6 for-of iteration statement.

R=mstarzinger@chromium.org, rossberg@chromium.org
TEST=mjsunit/harmony/iteration-semantics
BUG=v8:2214
Committed: http://code.google.com/p/v8/source/detail?r=15002

[wingo, 2013-05-21 16:20:33]

I am pretty sure that the LoadIC comments for ARM are out of date, and have made
this code under the assumption that they are, and updated the comments.  Please
take more scrutiny than usual on the patch to ic-arm.cc.

[wingo, 2013-05-22 08:23:08]

The updated patch makes ForOf have subexpressions for the following
sub-operations:

  var iterator = iterable; // can be replaced by @@iterator call
  var result = iterator.next();
  result.done // used in a conditional
  each = result.value

This should make it easier to crankshaft for-of.

This patch also removes the change to ic-arm.cc, which I should submit in a
separate patch.

[Michael Starzinger, 2013-06-06 11:09:08]

The implementation looks good already, just a nit. But can we try to increase
the test coverage? I left a list of suggestions in the test file.

https://codereview.chromium.org/15288011/diff/2001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/15288011/diff/2001/src/parser.h#newcode662
src/parser.h:662: void InitializeForEachStatement(ForEachStatement* stmt,
Let's move this declaration out of the block of parse methods (e.g. down to
where BuildObjectLiteralConstantProperties is declared).

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:33: 
Can we increase the test coverage for iterations. I was thinking about the
following cases which are not yet covered:

- Nested for-of statements, to verify that none of the internal ".result" and
".iterator" variables are destroyed.
- Using for-of on arbitrary iterator-like objects that have a next() method as
opposed to just generators.
- Using for-of on iterators that return non-standard result objects (e.g.
missing "done", missing "value", getters on these properties that count number
of accesses)
- Corner-cases where the the next() method is missing or disappears after a
certain number of iterations (to verify that the "iterator.next" lookup is
performed on every iteration).

[rossberg, 2013-06-06 11:35:32]

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:33: 
On 2013/06/06 11:09:09, Michael Starzinger wrote:
> Can we increase the test coverage for iterations. I was thinking about the
> following cases which are not yet covered:
> 
> - Nested for-of statements, to verify that none of the internal ".result" and
> ".iterator" variables are destroyed.
> - Using for-of on arbitrary iterator-like objects that have a next() method as
> opposed to just generators.
> - Using for-of on iterators that return non-standard result objects (e.g.
> missing "done", missing "value", getters on these properties that count number
> of accesses)
> - Corner-cases where the the next() method is missing or disappears after a
> certain number of iterations (to verify that the "iterator.next" lookup is
> performed on every iteration).

Also:

- interaction with exceptions
- ...and other non-local control flow (return, break/continue to outer loops)
- return objects with getters that throw
- iterators and/or return objects that are proxies

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:90: assertEquals(0, fold(sum, 0,
unreachable(null)));
Sigh...

[wingo, 2013-06-06 17:10:01]

Updated patchset rebases onto master and adds tests.

https://codereview.chromium.org/15288011/diff/2001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/15288011/diff/2001/src/parser.h#newcode662
src/parser.h:662: void InitializeForEachStatement(ForEachStatement* stmt,
On 2013/06/06 11:09:09, Michael Starzinger wrote:
> Let's move this declaration out of the block of parse methods (e.g. down to
> where BuildObjectLiteralConstantProperties is declared).

Done.

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:33: 
On 2013/06/06 11:09:09, Michael Starzinger wrote:
> Can we increase the test coverage for iterations. I was thinking about the
> following cases which are not yet covered:
> 
> - Nested for-of statements, to verify that none of the internal ".result" and
> ".iterator" variables are destroyed.
> - Using for-of on arbitrary iterator-like objects that have a next() method as
> opposed to just generators.
> - Using for-of on iterators that return non-standard result objects (e.g.
> missing "done", missing "value", getters on these properties that count number
> of accesses)
> - Corner-cases where the the next() method is missing or disappears after a
> certain number of iterations (to verify that the "iterator.next" lookup is
> performed on every iteration).

Done.

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:33: 
On 2013/06/06 11:35:33, rossberg wrote:
> 
> - interaction with exceptions

Not sure what you mean here.  Errors do seem to be thrown correctly and the
program continues as it should.  Do you mean a catch in a loop?

> - ...and other non-local control flow (return, break/continue to outer loops)

Done.

> - return objects with getters that throw

Done.

> - iterators and/or return objects that are proxies

Surely if getters work, then proxies will work, right?

[rossberg, 2013-06-06 18:14:10]

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/2001/test/mjsunit/harmony/itera...
test/mjsunit/harmony/iteration-semantics.js:33: 
On 2013/06/06 17:10:06, wingo wrote:
> > - interaction with exceptions
> 
> Not sure what you mean here.  Errors do seem to be thrown correctly and the
> program continues as it should.  Do you mean a catch in a loop?

Yes, some combinations of cases where an exception is thrown from different
places inside the loop or the iterator/generator, and caught/not caught
inside/outside the loop.

> Surely if getters work, then proxies will work, right?

Oh, if only that was true... ;) Unfortunately, they could not be any more
different, and proxies tend to need annoying special casing all over the place.

Since iterators make non-trivial use of objects it would be great if you could
add tests for some proxy cases (preferably in a separate file). But I'm fine
with deferring that to a separate CL.

[wingo, 2013-06-07 09:01:32]

Updated patch adds some try/catch and proxy tests.  PTAL :)

[rossberg, 2013-06-07 09:29:52]

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
test/mjsunit/harmony/iteration-semantics.js:297: // Proxies and getters,
together at last.
Thanks. :) Sorry to pester, but can we also have a test where the iterator is a
proxy?

Another (unrelated) thing that just occurred to me is testing the behaviour of
'done' values that are not Booleans.

[wingo, 2013-06-07 10:07:15]

Added more tests, thanks for the feedback.  See inline question about proxy
receivers for generators.

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
test/mjsunit/harmony/iteration-semantics.js:297: // Proxies and getters,
together at last.
On 2013/06/07 09:29:52, rossberg wrote:
> Thanks. :) Sorry to pester, but can we also have a test where the iterator is
a
> proxy?

Done.  However it doesn't work for generators, as iter.next() ends up being
called with a proxy receiver, which fails as GeneratorNext() wants a true
generator receiver.

This is orthogonal to for-of, however.  What should happen for proxy receivers? 
Should GeneratorNext() unpack them?

> Another (unrelated) thing that just occurred to me is testing the behaviour of
> 'done' values that are not Booleans.

Added a couple tests.  Not present == undefined was already being checked FWIW.

[rossberg, 2013-06-07 10:43:52]

LGTM from my side

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/19001/test/mjsunit/harmony/iter...
test/mjsunit/harmony/iteration-semantics.js:297: // Proxies and getters,
together at last.
On 2013/06/07 10:07:15, wingo wrote:
> On 2013/06/07 09:29:52, rossberg wrote:
> > Thanks. :) Sorry to pester, but can we also have a test where the iterator
> > is a proxy?
> 
> Done.  However it doesn't work for generators, as iter.next() ends up being
> called with a proxy receiver, which fails as GeneratorNext() wants a true
> generator receiver.
> 
> This is orthogonal to for-of, however.  What should happen for proxy
receivers? 
> Should GeneratorNext() unpack them?

Yeah, you just hit the core of the problem re proxies vs private state. There
won't be a real solution in ES6. Or ever. (This particular instance might be
addressed by the proposed 'invoke' trap, though.)

[Michael Starzinger, 2013-06-07 10:52:11]

LGTM from my end as well.

https://codereview.chromium.org/15288011/diff/25001/test/mjsunit/harmony/iter...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/15288011/diff/25001/test/mjsunit/harmony/iter...
test/mjsunit/harmony/iteration-semantics.js:30: // Test for-of syntax.
s/syntax/semantics/

[wingo, 2013-06-07 11:12:33]

Committed patchset #6 manually as r15002 (presubmit successful).