[es6] Self-assignment in a default parameter initializer should throw

[es6] Self-assignment in a default parameter initializer should throw

The first bug was that there are two different "initialization positions"
passed into PatternRewriter::DeclareAndInitializeVariables, and we weren't
setting them all properly for this case.

After further code review, it became clear that we weren't even recording
the correct position (the end of the initializer expression).

The combination of those two bugs caused the hole check elimination code
in full-codegen to skip emitting a hole check.

This patch takes care of both of those things. A follow-up will try
to reduce the number of "initializer positions" we track in the
variable declaration code.

R=littledan@chromium.org
BUG=v8:4568
LOG=n

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

[Dan Ehrenberg, 2015-11-24 00:37:35]

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

https://codereview.chromium.org/1468143004/diff/1/src/parser.cc#newcode4663
src/parser.cc:4663: parameter.pattern, descriptor.initialization_pos,
initial_value);
I think you want the position to be the end of the expression, not the
beginning. This is what let declarations do. What if the self-reference isn't
the first token in the default expression--then Fullcodegen could still
eliminate your hole check.

[adamk, 2015-11-24 00:57:02]

Description was changed from

==========
[es6] Self-assignment in a default parameter initializer should throw

There are two different "initialization positions" passed into
PatternRewriter::DeclareAndInitializeVariables, and we weren't
setting them all properly for this case.

In a followup I aim to clean this code up, but I'd like to make this
small in case we want to merge this into the 4.8 branch.

R=littledan@chromium.org
BUG=v8:4568
LOG=n
==========

to

==========
[es6] Self-assignment in a default parameter initializer should throw

The first bug was that there are two different "initialization positions"
passed into PatternRewriter::DeclareAndInitializeVariables, and we weren't
setting them all properly for this case.

After further code review, it became clear that we weren't even recording
the correct position (the end of the initializer expression).

The combination of those two bugs caused the hole check elimination code
in full-codegen to skip emitting a hole check.

This patch takes care of both of those things. A follow-up will try
to reduce the number of "initializer positions" we track in the
variable declaration code.

R=littledan@chromium.org
BUG=v8:4568
LOG=n
==========

[adamk, 2015-11-24 01:08:51]

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

https://codereview.chromium.org/1468143004/diff/1/src/parser.cc#newcode4663
src/parser.cc:4663: parameter.pattern, descriptor.initialization_pos,
initial_value);
On 2015/11/24 00:37:35, Dan Ehrenberg wrote:
> I think you want the position to be the end of the expression, not the
> beginning. This is what let declarations do. What if the self-reference isn't
> the first token in the default expression--then Fullcodegen could still
> eliminate your hole check.

Great catch. I made the test more complex and added a test for arrow functions.
This then required me to store an initializer position for each parameter (for
arrows, we don't easily have that information available, see the TODO). I'm
going to see if I can do something cleaner for arrows.

[adamk, 2015-11-24 01:18:29]

Hmm, it looks like I misunderstood the two concepts of "initialization
position". One is for the var, and is apparently only used in hole checks and in
strong mode. The other is just what the position of the Assignment expression
should be. So I probably need to further fix up this code.

[adamk, 2015-11-24 20:01:37]

Okay, this is ready for re-review

[Dan Ehrenberg, 2015-11-25 01:10:18]

lgtm

[adamk, 2015-11-25 01:13:30]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-11-25 01:13:40]

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

[commit-bot: I haz the power, 2015-11-25 01:30:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-25 01:30:45]

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