Remove duplicated ForOfStatement init code code from RewriteSpreads

Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).

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

[adamk, 2016-02-27 00:14:40]

Description was changed from

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads
==========

to

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForEachStatement instead, which already has
all the necessary logic.
==========

[adamk, 2016-02-27 00:15:33]

adamk@chromium.org changed reviewers:
+ nikolaos@chomium.org, rossberg@chromium.org

[adamk, 2016-02-27 00:15:33]

I couldn't find a reason why this wouldn't work, please let me know if I'm
missing something.

[adamk, 2016-02-27 00:16:12]

adamk@chromium.org changed reviewers:
+ nikolaos@chromium.org
- nikolaos@chomium.org

[adamk, 2016-02-27 00:16:28]

Fixed nikolaos' email...

[nickie, 2016-02-29 08:43:38]

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

https://codereview.chromium.org/1740293002/diff/1/src/parsing/parser.cc#oldco...
src/parsing/parser.cc:5677: GetIterator(subject, factory(),
spread->expression_position()),
The only reason I can see that this may not be equivalent is the positions for
error messages.  InitializeForEachStatement uses a "hacky" (as documented there)
subject->position()-2 here, instead of spread->expression_position().  I will
take a better look and come back.

https://codereview.chromium.org/1740293002/diff/1/src/parsing/parser.cc#oldco...
src/parsing/parser.cc:5686: spread->expression_position());
Same here, InitializeForEachStatement uses a "hacky" subject->position()-1.

[nickie, 2016-02-29 09:08:32]

On 2016/02/29 08:43:38, nickie wrote:
> The only reason I can see that this may not be equivalent is the positions for
> error messages.  (snip)  I will take a better look and come back.

OK, here it is.

------------------------------------------------------------------------
Mismatch in test: [...42];
------------------------------------------------------------------------

Output of patch is:

(d8):1: TypeError: 42[Symbol.iterator] is not a function
[...42];
  ^
TypeError: 42[Symbol.iterator] is not a function
    at (d8):1:3

Output of vanilla is:

(d8):1: TypeError: 42[Symbol.iterator] is not a function
[...42];
    ^
TypeError: 42[Symbol.iterator] is not a function
    at (d8):1:5

The difference as you can see is between (d8):1:3 and (d8):1:5, in the last line
of each output, which also affects where the carret pointing to the error is
shown.


The same appears more subtly in a more involved test case.  Suppose we have:

function* g(s, e) {
  for (var i=s; i<=e; i++)
    if (i==42)
      i(undefined);  // this is intentionally wrong
    else
      yield i;
};


------------------------------------------------------------------------
Mismatch in test: [...g(41,50)];
------------------------------------------------------------------------

Output of patch is:

(d8):1: TypeError: i is not a function
function* g(s, e) { for (var i=s; i<=e; i++) if (i==42) i(undefined); else yield
i; };
                                                        ^
TypeError: i is not a function
    at g ((d8):1:57)
    at next (native)
    at (d8):1:4

Output of vanilla is:

(d8):1: TypeError: i is not a function
function* g(s, e) { for (var i=s; i<=e; i++) if (i==42) i(undefined); else yield
i; };
                                                        ^
TypeError: i is not a function
    at g ((d8):1:57)
    at next (native)
    at (d8):1:5


Here the difference is between (d8):1:4 and (d8):1:5; both are positions in the
line that called g: "[...g(41,50)];".  At the time I was writing RewriteSpreads,
the error was reported there and not in the function that actually caused it. 
What we have now is better, but still I suppose that if one uses the debugger to
see where this came from, it will end up in a slightly different position.


If we cannot live with this, I suggest adding an extra parameter to
InitializeForEachStatement that would hold the position of the iterator.  If not
initialized, the default behaviour would be the "hacky" one.

[nickie, 2016-03-01 16:27:48]

On 2016/02/29 09:08:32, nickie wrote:
> If we cannot live with this, I suggest adding an extra parameter to
> InitializeForEachStatement that would hold the position of the iterator.  If
not
> initialized, the default behaviour would be the "hacky" one.

Adam, I can do this if you want to proceed with it.

[adamk, 2016-03-01 18:50:28]

On 2016/03/01 16:27:48, nickie wrote:
> On 2016/02/29 09:08:32, nickie wrote:
> > If we cannot live with this, I suggest adding an extra parameter to
> > InitializeForEachStatement that would hold the position of the iterator.  If
> not
> > initialized, the default behaviour would be the "hacky" one.
> 
> Adam, I can do this if you want to proceed with it.

I'm fine with adding that, just didn't get a chance to work on it yesterday.

[adamk, 2016-03-01 20:14:06]

On 2016/03/01 18:50:28, adamk wrote:
> On 2016/03/01 16:27:48, nickie wrote:
> > On 2016/02/29 09:08:32, nickie wrote:
> > > If we cannot live with this, I suggest adding an extra parameter to
> > > InitializeForEachStatement that would hold the position of the iterator. 
If
> > not
> > > initialized, the default behaviour would be the "hacky" one.
> > 
> > Adam, I can do this if you want to proceed with it.
> 
> I'm fine with adding that, just didn't get a chance to work on it yesterday.

Hmm, I'm not actually sure what the "right" fix is here, given how hacky the
hack is. Right now, here's the output for another case;

$ d8 -e 'Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]'
unnamed:1: TypeError: Cannot read property 'next' of undefined
Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]
                                                     ^
TypeError: Cannot read property 'next' of undefined
    at unnamed:1:54

[nickie, 2016-03-02 08:49:18]

On 2016/03/01 20:14:06, adamk wrote:
> Hmm, I'm not actually sure what the "right" fix is here, given how hacky the
> hack is. Right now, here's the output for another case;
> 
> $ d8 -e 'Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]'
> unnamed:1: TypeError: Cannot read property 'next' of undefined
> Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]
>                                                      ^
> TypeError: Cannot read property 'next' of undefined
>     at unnamed:1:54

This looks OK to me.  The problem is that (after one messes up with the array
prototype), the array [1, 2, 3] where the arrow points is not an iterable any
more.  What is it that you don't like here?

[adamk, 2016-03-02 22:14:22]

On 2016/03/02 08:49:18, nickie wrote:
> On 2016/03/01 20:14:06, adamk wrote:
> > Hmm, I'm not actually sure what the "right" fix is here, given how hacky the
> > hack is. Right now, here's the output for another case;
> > 
> > $ d8 -e 'Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]'
> > unnamed:1: TypeError: Cannot read property 'next' of undefined
> > Array.prototype[Symbol.iterator] = function(){}; [...[1, 2, 3]]
> >                                                      ^
> > TypeError: Cannot read property 'next' of undefined
> >     at unnamed:1:54
> 
> This looks OK to me.  The problem is that (after one messes up with the array
> prototype), the array [1, 2, 3] where the arrow points is not an iterable any
> more.  What is it that you don't like here?

So I'd thought that for for-of we gave a better error message here, but I don't
think we do.

[adamk, 2016-03-03 01:38:25]

adamk@chromium.org changed reviewers:
+ verwaest@chromium.org

[adamk, 2016-03-03 01:38:25]

+verwaest, who wrote this position "hack" in the first place. Toon, do you have
any thoughts on a way we could get rid of the position hacks for for-of errors
while doing this cleanup?

[adamk, 2016-03-03 22:38:04]

Description was changed from

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForEachStatement instead, which already has
all the necessary logic.
==========

to

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for another
(use_position_hack).
==========

[adamk, 2016-03-03 22:39:03]

Okay, this now avoids changing any existing behavior, PTAL.

[nickie, 2016-03-04 12:04:11]

On 2016/03/03 22:39:03, adamk wrote:
> Okay, this now avoids changing any existing behavior, PTAL.

Here's another side-effect, though.  The symptom here is not the position where
the error is reported, but the error message.  I think the right message to
report is the one in the vanilla version.  I'll check why this happens now and
I'll come back.

------------------------------------------------------------------------
Mismatch in test: [17, ...42(undefined)];
------------------------------------------------------------------------

Output of patch is:

(d8):1: TypeError: 42(...)[Symbol.iterator] is not a function
[17, ...42(undefined)];
          ^
TypeError: 42(...)[Symbol.iterator] is not a function
    at (d8):1:11

Output of vanilla is:

(d8):1: TypeError: 42 is not a function
[17, ...42(undefined)];
          ^
TypeError: 42 is not a function
    at (d8):1:11

[nickie, 2016-03-04 14:15:13]

Got it.

https://codereview.chromium.org/1740293002/diff/60001/src/parsing/parser.cc
File src/parsing/parser.cc (left):

https://codereview.chromium.org/1740293002/diff/60001/src/parsing/parser.cc#o...
src/parsing/parser.cc:5677: GetIterator(subject, factory(),
spread->expression_position()),
There was another tricky point but it was a bit hidden and I missed it in my
first comment.  In the line above, it's spread->expression_position() and not
spread->expression->position().  There's an extra field for spreads keeping the
starting position of the expression after the dots; this is the one used here.

So, for "...42(undefined)", spread->expression->position() would have been the
opening parenthesis.  Instead, the original code was taking
spread->expression_position() which was the digit 4.  As far as I understand,
this is what guides the error messaging system to report the right expression
that's guilty.

https://codereview.chromium.org/1740293002/diff/60001/src/parsing/parser.cc#o...
src/parsing/parser.cc:5686: spread->expression_position());
Here too...

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

https://codereview.chromium.org/1740293002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3291: bool use_position_hack) {
As a fix, I suggest that you replace "bool use_position_hack" with "int
iterable_position".  The default value would be something illegal as a position
(e.g., -1) and this would trigger the "position hack".

https://codereview.chromium.org/1740293002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3305: int next_result_pos = subject->position() -
(use_position_hack * 1);
These two would become:

int get_iterator_pos =
  iterable_position < 0 ? subject->position() - 2 : iterable_position;
int next_result_pos =
  iterable_position < 0 ? subject->position() - 1 : iterable_position;

[adamk, 2016-03-04 18:50:09]

Thanks for the details, I could never remember what
Spread::expression_position() was all about.

Switched to a position arg.

[adamk, 2016-03-04 18:50:28]

Description was changed from

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for another
(use_position_hack).
==========

to

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).
==========

[nickie, 2016-03-07 09:21:43]

On 2016/03/04 18:50:09, adamk wrote:
> Thanks for the details, I could never remember what
> Spread::expression_position() was all about.
> 
> Switched to a position arg.

LGTM.

I tested it and found no difference in the error messages this time.
It still needs rebasing, though.

[adamk, 2016-03-07 19:12:05]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-03-07 19:12:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
nikolaos@chromium.org
Link to the patchset: https://codereview.chromium.org/1740293002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-07 19:12:14]

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

[commit-bot: I haz the power, 2016-03-07 19:52:15]

Description was changed from

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).
==========

to

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).
==========

[commit-bot: I haz the power, 2016-03-07 19:52:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-07 19:53:35]

Description was changed from

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).
==========

to

==========
Remove duplicated ForOfStatement init code code from RewriteSpreads

Simply call InitializeForOfStatement (split out from InitializeForEachStatement)
instead, which already has all the necessary logic.

As part of this, trade one bool arg (is_destructuring) for an int
(iterable_pos).

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

[commit-bot: I haz the power, 2016-03-07 19:53:36]

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