[es6] Make assignment to new.target an early ReferenceError

[es6] Make assignment to new.target an early ReferenceError

In doing so, fix calls CheckAndRewriteReferenceExpression to take proper
start and end positions (instead of just pointing at the first token in
the LHS expression).

BUG=v8:4370
LOG=n

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

[adamk, 2015-08-12 23:36:48]

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

[adamk, 2015-08-12 23:39:29]

Note that the preparser already (accidentally) seems to throw an error here,
because its implementation of IsValidReferenceExpression() is:

  bool IsValidReferenceExpression() const {
    return IsIdentifier() || IsProperty();
  }

and NewTargetExpression is a PreParserExpression::Default().

[caitp (gmail), 2015-08-12 23:51:42]

On 2015/08/12 23:39:29, adamk wrote:
> Note that the preparser already (accidentally) seems to throw an error here,
> because its implementation of IsValidReferenceExpression() is:
> 
>   bool IsValidReferenceExpression() const {
>     return IsIdentifier() || IsProperty();
>   }
> 
> and NewTargetExpression is a PreParserExpression::Default().

Just for the benefit of people touching the parser in the future, i wonder if it
might be worth making that clearer. Or if not, maybe a comment would be good

[adamk, 2015-08-12 23:58:04]

On 2015/08/12 23:51:42, caitp wrote:
> On 2015/08/12 23:39:29, adamk wrote:
> > Note that the preparser already (accidentally) seems to throw an error here,
> > because its implementation of IsValidReferenceExpression() is:
> > 
> >   bool IsValidReferenceExpression() const {
> >     return IsIdentifier() || IsProperty();
> >   }
> > 
> > and NewTargetExpression is a PreParserExpression::Default().
> 
> Just for the benefit of people touching the parser in the future, i wonder if
it
> might be worth making that clearer. Or if not, maybe a comment would be good

Yeah, agreed that this should be somewhere other than just this code review.
I'll see about just adding it here. Would be great if the messages tests ran
with --min-preparse-length=0 to force the preparser.

[adamk, 2015-08-13 00:29:51]

On 2015/08/12 23:58:04, adamk wrote:
> On 2015/08/12 23:51:42, caitp wrote:
> > On 2015/08/12 23:39:29, adamk wrote:
> > > Note that the preparser already (accidentally) seems to throw an error
here,
> > > because its implementation of IsValidReferenceExpression() is:
> > > 
> > >   bool IsValidReferenceExpression() const {
> > >     return IsIdentifier() || IsProperty();
> > >   }
> > > 
> > > and NewTargetExpression is a PreParserExpression::Default().
> > 
> > Just for the benefit of people touching the parser in the future, i wonder
if
> it
> > might be worth making that clearer. Or if not, maybe a comment would be good
> 
> Yeah, agreed that this should be somewhere other than just this code review.
> I'll see about just adding it here. Would be great if the messages tests ran
> with --min-preparse-length=0 to force the preparser.

Not sure where this comment should go, actually. Any thoughts?

[caitp (gmail), 2015-08-13 00:35:33]

On 2015/08/13 00:29:51, adamk wrote:
> On 2015/08/12 23:58:04, adamk wrote:
> > On 2015/08/12 23:51:42, caitp wrote:
> > > On 2015/08/12 23:39:29, adamk wrote:
> > > > Note that the preparser already (accidentally) seems to throw an error
> here,
> > > > because its implementation of IsValidReferenceExpression() is:
> > > > 
> > > >   bool IsValidReferenceExpression() const {
> > > >     return IsIdentifier() || IsProperty();
> > > >   }
> > > > 
> > > > and NewTargetExpression is a PreParserExpression::Default().
> > > 
> > > Just for the benefit of people touching the parser in the future, i wonder
> if
> > it
> > > might be worth making that clearer. Or if not, maybe a comment would be
good
> > 
> > Yeah, agreed that this should be somewhere other than just this code review.
> > I'll see about just adding it here. Would be great if the messages tests ran
> > with --min-preparse-length=0 to force the preparser.
> 
> Not sure where this comment should go, actually. Any thoughts?

On phone atm, hard to see. But i had the point where the default expression is
created.i think it would be nice to encode clearly that it's a special thing
though, do a new enum value and factory. BUt yeah, wherever new.target is
preparsed/parsef, just leave a note there

[adamk, 2015-08-13 00:45:56]

On 2015/08/13 00:35:33, caitp wrote:
> On 2015/08/13 00:29:51, adamk wrote:
> > On 2015/08/12 23:58:04, adamk wrote:
> > > On 2015/08/12 23:51:42, caitp wrote:
> > > > On 2015/08/12 23:39:29, adamk wrote:
> > > > > Note that the preparser already (accidentally) seems to throw an error
> > here,
> > > > > because its implementation of IsValidReferenceExpression() is:
> > > > > 
> > > > >   bool IsValidReferenceExpression() const {
> > > > >     return IsIdentifier() || IsProperty();
> > > > >   }
> > > > > 
> > > > > and NewTargetExpression is a PreParserExpression::Default().
> > > > 
> > > > Just for the benefit of people touching the parser in the future, i
wonder
> > if
> > > it
> > > > might be worth making that clearer. Or if not, maybe a comment would be
> good
> > > 
> > > Yeah, agreed that this should be somewhere other than just this code
review.
> > > I'll see about just adding it here. Would be great if the messages tests
ran
> > > with --min-preparse-length=0 to force the preparser.
> > 
> > Not sure where this comment should go, actually. Any thoughts?
> 
> On phone atm, hard to see. But i had the point where the default expression is
> created.i think it would be nice to encode clearly that it's a special thing
> though, do a new enum value and factory. BUt yeah, wherever new.target is
> preparsed/parsef, just leave a note there

My issue is that there are lots of PreParserExpressions for which
IsValidReferenceExpression() returns false, so it seems odd to put a comment
just for NewTarget. When you're at a computer let me know a particular place you
think it'd make sense.

[caitp (gmail), 2015-08-13 01:59:49]

On 2015/08/13 00:45:56, adamk wrote:
> On 2015/08/13 00:35:33, caitp wrote:
> > On 2015/08/13 00:29:51, adamk wrote:
> > > On 2015/08/12 23:58:04, adamk wrote:
> > > > On 2015/08/12 23:51:42, caitp wrote:
> > > > > On 2015/08/12 23:39:29, adamk wrote:
> > > > > > Note that the preparser already (accidentally) seems to throw an
error
> > > here,
> > > > > > because its implementation of IsValidReferenceExpression() is:
> > > > > > 
> > > > > >   bool IsValidReferenceExpression() const {
> > > > > >     return IsIdentifier() || IsProperty();
> > > > > >   }
> > > > > > 
> > > > > > and NewTargetExpression is a PreParserExpression::Default().
> > > > > 
> > > > > Just for the benefit of people touching the parser in the future, i
> wonder
> > > if
> > > > it
> > > > > might be worth making that clearer. Or if not, maybe a comment would
be
> > good
> > > > 
> > > > Yeah, agreed that this should be somewhere other than just this code
> review.
> > > > I'll see about just adding it here. Would be great if the messages tests
> ran
> > > > with --min-preparse-length=0 to force the preparser.
> > > 
> > > Not sure where this comment should go, actually. Any thoughts?
> > 
> > On phone atm, hard to see. But i had the point where the default expression
is
> > created.i think it would be nice to encode clearly that it's a special thing
> > though, do a new enum value and factory. BUt yeah, wherever new.target is
> > preparsed/parsef, just leave a note there
> 
> My issue is that there are lots of PreParserExpressions for which
> IsValidReferenceExpression() returns false, so it seems odd to put a comment
> just for NewTarget. When you're at a computer let me know a particular place
you
> think it'd make sense.

I would just write a quick comment in ParseNewTargetExpression(), below the
final return statement. But I guess it doesn't make much sense to specifically
document weirdness there but ignore all the other similar cases, so you can
decide whether you think it's worth it or not.

[rossberg, 2015-08-13 11:13:54]

lgtm

https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/ne...
File test/mjsunit/harmony/new-target.js (right):

https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/ne...
test/mjsunit/harmony/new-target.js:390: assertThrows(function() {
Function("new.target = 42"); }, ReferenceError);
Could add a case for at least one compound assignment op.

[adamk, 2015-08-13 17:05:44]

https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/ne...
File test/mjsunit/harmony/new-target.js (right):

https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/ne...
test/mjsunit/harmony/new-target.js:390: assertThrows(function() {
Function("new.target = 42"); }, ReferenceError);
On 2015/08/13 11:13:54, rossberg wrote:
> Could add a case for at least one compound assignment op.

Done. Added a few more tests here.

[adamk, 2015-08-13 17:05:48]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-08-13 17:05:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1290013002/#ps40001
(title: "Add a few more tests")

[commit-bot: I haz the power, 2015-08-13 17:06:04]

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

[commit-bot: I haz the power, 2015-08-13 18:06:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-13 18:06:26]

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