[es6] strict eval/arguments and strong undefined in AssignmentPattern

[es6] strict eval/arguments and strong undefined in AssignmentPattern

BUG=v8:4613
LOG=N
R=adamk@chromium.org

Committed: https://crrev.com/8f63710a3df00c3f093d1fbdad363849bc0dd640
Cr-Commit-Position: refs/heads/master@{#32888}

[caitp (gmail), 2015-12-12 00:46:41]

quick hack --- this one goes out of its way to match the error messages reported
for other cases. If we don't care which message is used, it can be done simpler

[adamk, 2015-12-12 01:03:35]

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

https://codereview.chromium.org/1522693002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1531: CheckDestructuringElement(argument, classifier,
start_pos,
I think CheckDestructuringElement is doing the work here already (testing
locally, "[...eval] = []" already gives me an error without your patch). It
calls IsAssignableIdentifier(), which does the proper checks
eval/arguments/undefined checks.

Maybe you just need to make sure to call CheckDestructuringElement in more
places?

https://codereview.chromium.org/1522693002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3255: void
ParserBase<Traits>::ClassifyReferenceExpression(
Indeed, this code is very close to the existing CheckDestructuringElement...

[caitp (gmail), 2015-12-12 01:08:14]

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

https://codereview.chromium.org/1522693002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1531: CheckDestructuringElement(argument, classifier,
start_pos,
On 2015/12/12 01:03:35, adamk wrote:
> I think CheckDestructuringElement is doing the work here already (testing
> locally, "[...eval] = []" already gives me an error without your patch). It
> calls IsAssignableIdentifier(), which does the proper checks
> eval/arguments/undefined checks.
> 
> Maybe you just need to make sure to call CheckDestructuringElement in more
> places?

Yes, there is some redundant stuff here --- as mentioned, this change matches
the messages used in other situations where assignment to illegal variables is
performed, but it can be cheapened a bit if we don't care which message is used

[adamk, 2015-12-12 01:10:44]

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

https://codereview.chromium.org/1522693002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1531: CheckDestructuringElement(argument, classifier,
start_pos,
On 2015/12/12 01:08:14, caitp wrote:
> On 2015/12/12 01:03:35, adamk wrote:
> > I think CheckDestructuringElement is doing the work here already (testing
> > locally, "[...eval] = []" already gives me an error without your patch). It
> > calls IsAssignableIdentifier(), which does the proper checks
> > eval/arguments/undefined checks.
> > 
> > Maybe you just need to make sure to call CheckDestructuringElement in more
> > places?
> 
> Yes, there is some redundant stuff here --- as mentioned, this change matches
> the messages used in other situations where assignment to illegal variables is
> performed, but it can be cheapened a bit if we don't care which message is
used

Would much prefer less duplication to message clarity (at least as a first cut).

[adamk, 2015-12-16 09:45:42]

lgtm % style nit

Sorry for the delay.

https://codereview.chromium.org/1522693002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1522693002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3283: return;
Nit: wrap this in a block (required by style since if expression is on multiple
lines)

[adamk, 2015-12-16 09:52:34]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-12-16 09:52:39]

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

[commit-bot: I haz the power, 2015-12-16 10:47:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-16 10:48:04]

Description was changed from

==========
[es6] strict eval/arguments and strong undefined in AssignmentPattern

BUG=v8:4613
LOG=N
R=adamk@chromium.org
==========

to

==========
[es6] strict eval/arguments and strong undefined in AssignmentPattern

BUG=v8:4613
LOG=N
R=adamk@chromium.org

Committed: https://crrev.com/8f63710a3df00c3f093d1fbdad363849bc0dd640
Cr-Commit-Position: refs/heads/master@{#32888}
==========

[commit-bot: I haz the power, 2015-12-16 10:48:05]

Patchset 2 (id:??) landed as
https://crrev.com/8f63710a3df00c3f093d1fbdad363849bc0dd640
Cr-Commit-Position: refs/heads/master@{#32888}