Factor formal argument parsing into ParserBase

Factor formal argument parsing into ParserBase

This commit is a precursor to making lazy arrow function parsing use
similar logic to function(){} argument parsing.

R=arv@chromium.org
BUG=4020
LOG=N

Committed: https://crrev.com/1dbc43272954e8cfdf7be9a57c953a74b2a4d9da
Cr-Commit-Position: refs/heads/master@{#27773}

[arv (Not doing code reviews), 2015-04-10 14:35:50]

Very nice. 

Maybe you can add some tests to test/message/ to verify the error message and
position.

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

https://codereview.chromium.org/1078093002/diff/1/src/messages.js#newcode204
src/messages.js:204: bad_getter_arity:              ["Getters should not have
any formal parameters."],
Maybe use "must" here

Getter must not have any ...
Setter must have exactly one ...

[caitp (gmail), 2015-04-10 15:03:39]

+1 on getting rid of the separate parameter list parsers

I would prefer it if error reporting was kept at least as-is, or is improved on,
though. Meaningful, actionable errors are important both for novices and more
senior developers, they save a lot of time.

[wingo, 2015-04-10 16:47:02]

On 2015/04/10 15:03:39, caitp wrote:
> +1 on getting rid of the separate parameter list parsers
> 
> I would prefer it if error reporting was kept at least as-is, or is improved
on,
> though. Meaningful, actionable errors are important both for novices and more
> senior developers, they save a lot of time.

I gather you're referring to the "rest parameter must be last formal parameter"
message?   The getter/setter messages are indeed better in this patch.

But as you note:

d8> function foo(...a, b) { return a}
(d8):1: SyntaxError: Unexpected token ,
function foo(...a, b) { return a}
                 ^
SyntaxError: Unexpected token ,

Kinda silly that the error was "unexpected token" when we know what token we're
looking for.  Would it be better if Expect(Token::Value) caused errors like
this:

(d8):1: SyntaxError: Unexpected token ,
function foo(...a, b) { return a}
                 ^
SyntaxError: Unexpected token , while expecting )

Dunno.  Would seem to be an easy change but I don't if we can find a wording
that is sufficiently helpful.

[arv (Not doing code reviews), 2015-04-10 16:51:05]

On 2015/04/10 16:47:02, wingo wrote:
> On 2015/04/10 15:03:39, caitp wrote:
> > +1 on getting rid of the separate parameter list parsers
> > 
> > I would prefer it if error reporting was kept at least as-is, or is improved
> on,
> > though. Meaningful, actionable errors are important both for novices and
more
> > senior developers, they save a lot of time.
> 
> I gather you're referring to the "rest parameter must be last formal
parameter"
> message?   The getter/setter messages are indeed better in this patch.
> 
> But as you note:
> 
> d8> function foo(...a, b) { return a}
> (d8):1: SyntaxError: Unexpected token ,
> function foo(...a, b) { return a}
>                  ^
> SyntaxError: Unexpected token ,
> 
> Kinda silly that the error was "unexpected token" when we know what token
we're
> looking for.  Would it be better if Expect(Token::Value) caused errors like
> this:
> 
> (d8):1: SyntaxError: Unexpected token ,
> function foo(...a, b) { return a}
>                  ^
> SyntaxError: Unexpected token , while expecting )
> 
> Dunno.  Would seem to be an easy change but I don't if we can find a wording
> that is sufficiently helpful.

That seems useful but it out of scope for this CL.

We can talk about the preferred wording when we make a CL for that.

[caitp (gmail), 2015-04-10 16:53:46]

On 2015/04/10 16:47:02, wingo wrote:
> On 2015/04/10 15:03:39, caitp wrote:
> > +1 on getting rid of the separate parameter list parsers
> > 
> > I would prefer it if error reporting was kept at least as-is, or is improved
> on,
> > though. Meaningful, actionable errors are important both for novices and
more
> > senior developers, they save a lot of time.
> 
> I gather you're referring to the "rest parameter must be last formal
parameter"
> message?   The getter/setter messages are indeed better in this patch.
> 
> But as you note:
> 
> d8> function foo(...a, b) { return a}
> (d8):1: SyntaxError: Unexpected token ,
> function foo(...a, b) { return a}
>                  ^
> SyntaxError: Unexpected token ,
> 
> Kinda silly that the error was "unexpected token" when we know what token
we're
> looking for.  Would it be better if Expect(Token::Value) caused errors like
> this:
> 
> (d8):1: SyntaxError: Unexpected token ,
> function foo(...a, b) { return a}
>                  ^
> SyntaxError: Unexpected token , while expecting )
> 
> Dunno.  Would seem to be an easy change but I don't if we can find a wording
> that is sufficiently helpful.

The way I would do it, if `*is_rest` is true and peek() is a comma, report the
existing param_after_rest error. If it's not a ), and then Expect(Token::RPAREN)

[wingo, 2015-04-10 17:02:41]

On 2015/04/10 16:53:46, caitp wrote:
> On 2015/04/10 16:47:02, wingo wrote:
> > On 2015/04/10 15:03:39, caitp wrote:
> > > +1 on getting rid of the separate parameter list parsers
> > > 
> > > I would prefer it if error reporting was kept at least as-is, or is
improved
> > on,
> > > though. Meaningful, actionable errors are important both for novices and
> more
> > > senior developers, they save a lot of time.
> > 
> > I gather you're referring to the "rest parameter must be last formal
> parameter"
> > message?   The getter/setter messages are indeed better in this patch.
> > 
> > But as you note:
> > 
> > d8> function foo(...a, b) { return a}
> > (d8):1: SyntaxError: Unexpected token ,
> > function foo(...a, b) { return a}
> >                  ^
> > SyntaxError: Unexpected token ,
> > 
> > Kinda silly that the error was "unexpected token" when we know what token
> we're
> > looking for.  Would it be better if Expect(Token::Value) caused errors like
> > this:
> > 
> > (d8):1: SyntaxError: Unexpected token ,
> > function foo(...a, b) { return a}
> >                  ^
> > SyntaxError: Unexpected token , while expecting )
> > 
> > Dunno.  Would seem to be an easy change but I don't if we can find a wording
> > that is sufficiently helpful.
> 
> The way I would do it, if `*is_rest` is true and peek() is a comma, report the
> existing param_after_rest error. If it's not a ), and then
Expect(Token::RPAREN)

Funny, I came to the same conclusion :)  Still, worth thinking about whether
Consume() could produce better errors.

[wingo, 2015-04-10 17:03:53]

On 2015/04/10 17:02:41, wingo wrote:
> Expect(Token::RPAREN)
> 
> Funny, I came to the same conclusion :)  Still, worth thinking about whether
> Consume() could produce better errors.

Er, Expect() I mean.

[wingo, 2015-04-10 17:15:00]

Updated patch fixes nits; ptal.  I'm knocking off for the day so if it looks
useful to you please feel free to cq+.

[caitp (gmail), 2015-04-10 17:37:49]

On 2015/04/10 17:15:00, wingo wrote:
> Updated patch fixes nits; ptal.  I'm knocking off for the day so if it looks
> useful to you please feel free to cq+.

Could be a bit better, but looks good to me

One thing I'm a bit iffy about us how much work it would be to allow binding
patterns in the formal parameters list, but I'm on mobile and haven't looked too
close yet

[arv (Not doing code reviews), 2015-04-10 17:51:00]

LGTM

It looks like you got some changes from another patch in there. This might not
apply cleanly.

[wingo, 2015-04-13 07:40:48]

On 2015/04/10 17:51:00, arv wrote:
> LGTM
> 
> It looks like you got some changes from another patch in there. This might not
> apply cleanly.

Fixed, thanks for spotting that.  I had to rebase because of a parallel change
to add "undefined_error_loc".  I'll make a followup to make a
FormalParametersErrorLocations or something because this is getting a bit
unwieldy.

[wingo, 2015-04-13 07:40:56]

The CQ bit was checked by wingo@igalia.com

[wingo, 2015-04-13 07:40:56]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1078093002/#ps40001
(title: "Rebase on top of "undefined" error detection, remove bits of utils.h
patch that crept in")

[commit-bot: I haz the power, 2015-04-13 07:41:08]

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

[commit-bot: I haz the power, 2015-04-13 08:07:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-13 08:07:23]

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