[parser] report SyntaxError if rest parameter used in Setter MethodDefinition

[parser] report SyntaxError if rest parameter used in Setter MethodDefinition

BUG=v8:4107, v8:2159
LOG=N
R=arv@chromium.org

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

[caitp (gmail), 2015-05-15 13:36:01]

PTAL

[arv (Not doing code reviews), 2015-05-15 13:55:56]

https://codereview.chromium.org/1141223002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
src/messages.js:144: setter_rest_param:             ["Setter function argument
must not be a rest parameter"],
Can you use messages.h instead since Yang is in the process of removing this
table from js.

https://codereview.chromium.org/1141223002/diff/1/test/message/rest-param-cla...
File test/message/rest-param-class-setter-strict.out (right):

https://codereview.chromium.org/1141223002/diff/1/test/message/rest-param-cla...
test/message/rest-param-class-setter-strict.out:3: ^^^^^^^^^
The location here is a bit strange. Can it be improved to point at `...args`
instead of `(...args)`?

[caitp (gmail), 2015-05-15 13:57:59]

On 2015/05/15 13:55:56, arv wrote:
> https://codereview.chromium.org/1141223002/diff/1/src/messages.js
> File src/messages.js (right):
> 
> https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
> src/messages.js:144: setter_rest_param:             ["Setter function argument
> must not be a rest parameter"],
> Can you use messages.h instead since Yang is in the process of removing this
> table from js.
> 
>
https://codereview.chromium.org/1141223002/diff/1/test/message/rest-param-cla...
> File test/message/rest-param-class-setter-strict.out (right):
> 
>
https://codereview.chromium.org/1141223002/diff/1/test/message/rest-param-cla...
> test/message/rest-param-class-setter-strict.out:3: ^^^^^^^^^
> The location here is a bit strange. Can it be improved to point at `...args`
> instead of `(...args)`?

it seemed harder to do that, but it would be nice :| I think the simplest thing
was to just report this in ChecArityRestrictions, and it's maybe simpler to just
reuse the existing setter arity error, the way SpiderMonkey does

[caitp (gmail), 2015-05-15 14:01:27]

caitpotter88@gmail.com changed reviewers:
+ yangguo@chromium.org

[caitp (gmail), 2015-05-15 14:01:28]

https://codereview.chromium.org/1141223002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
src/messages.js:144: setter_rest_param:             ["Setter function argument
must not be a rest parameter"],
On 2015/05/15 13:55:56, arv wrote:
> Can you use messages.h instead since Yang is in the process of removing this
> table from js.

I'd like to, but it's not obvious how to make the parser's ReportMessage
infrastructure use the new message stuff, which I think is why most of those
messages haven't been moved to that yet. The one SyntaxError which does use this
is part of a runtime function, which doesn't really apply to normal parsing.
Maybe yangguo could shed some light on this, I'll add them as a reviewer

[caitp (gmail), 2015-05-16 18:53:20]

On 2015/05/15 14:01:28, caitp wrote:
> https://codereview.chromium.org/1141223002/diff/1/src/messages.js
> File src/messages.js (right):
> 
> https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
> src/messages.js:144: setter_rest_param:             ["Setter function argument
> must not be a rest parameter"],
> On 2015/05/15 13:55:56, arv wrote:
> > Can you use messages.h instead since Yang is in the process of removing this
> > table from js.
> 
> I'd like to, but it's not obvious how to make the parser's ReportMessage
> infrastructure use the new message stuff, which I think is why most of those
> messages haven't been moved to that yet. The one SyntaxError which does use
this
> is part of a runtime function, which doesn't really apply to normal parsing.
> Maybe yangguo could shed some light on this, I'll add them as a reviewer

I think the only thing that can make this simpler is just getting rid of the new
error, but I like that it's specific.

[Yang, 2015-05-16 22:15:37]

On 2015/05/16 18:53:20, caitp wrote:
> On 2015/05/15 14:01:28, caitp wrote:
> > https://codereview.chromium.org/1141223002/diff/1/src/messages.js
> > File src/messages.js (right):
> > 
> > https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
> > src/messages.js:144: setter_rest_param:             ["Setter function
argument
> > must not be a rest parameter"],
> > On 2015/05/15 13:55:56, arv wrote:
> > > Can you use messages.h instead since Yang is in the process of removing
this
> > > table from js.
> > 
> > I'd like to, but it's not obvious how to make the parser's ReportMessage
> > infrastructure use the new message stuff, which I think is why most of those
> > messages haven't been moved to that yet. The one SyntaxError which does use
> this
> > is part of a runtime function, which doesn't really apply to normal parsing.
> > Maybe yangguo could shed some light on this, I'll add them as a reviewer
> 
> I think the only thing that can make this simpler is just getting rid of the
new
> error, but I like that it's specific.

I have a CL pending that migrates the parser related error messages.

[Yang, 2015-05-18 07:03:46]

On 2015/05/16 22:15:37, Yang wrote:
> On 2015/05/16 18:53:20, caitp wrote:
> > On 2015/05/15 14:01:28, caitp wrote:
> > > https://codereview.chromium.org/1141223002/diff/1/src/messages.js
> > > File src/messages.js (right):
> > > 
> > >
https://codereview.chromium.org/1141223002/diff/1/src/messages.js#newcode144
> > > src/messages.js:144: setter_rest_param:             ["Setter function
> argument
> > > must not be a rest parameter"],
> > > On 2015/05/15 13:55:56, arv wrote:
> > > > Can you use messages.h instead since Yang is in the process of removing
> this
> > > > table from js.
> > > 
> > > I'd like to, but it's not obvious how to make the parser's ReportMessage
> > > infrastructure use the new message stuff, which I think is why most of
those
> > > messages haven't been moved to that yet. The one SyntaxError which does
use
> > this
> > > is part of a runtime function, which doesn't really apply to normal
parsing.
> > > Maybe yangguo could shed some light on this, I'll add them as a reviewer
> > 
> > I think the only thing that can make this simpler is just getting rid of the
> new
> > error, but I like that it's specific.
> 
> I have a CL pending that migrates the parser related error messages.

See https://codereview.chromium.org/1130133003/

[caitp (gmail), 2015-05-18 11:57:00]

On 2015/05/18 07:03:46, Yang wrote:
> See https://codereview.chromium.org/1130133003/

Thanks, updated to match.

[arv (Not doing code reviews), 2015-05-18 14:59:50]

Acually, can you also add something to test-parsing.cc so that we can ensure
that both preparser and parser works the same?

[caitp (gmail), 2015-05-18 15:10:40]

On 2015/05/18 14:59:50, arv wrote:
> Acually, can you also add something to test-parsing.cc so that we can ensure
> that both preparser and parser works the same?

Done

[arv (Not doing code reviews), 2015-05-18 15:18:13]

LGTM

[caitp (gmail), 2015-05-18 15:24:05]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-05-18 15:24:17]

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

[commit-bot: I haz the power, 2015-05-18 15:53:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-18 15:53:19]

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