Improve error message for duplicate parameters

Improve error message for duplicate parameters

Duplicate parameters are banned both overall in strict mode and also
in arrow functions. Our error message for both cases blamed strict
mode, which is confusing. This patch fixes the message to point to
arrow functions as a possible source as well.

R=wingo, adamk
LOG=N

Committed: https://crrev.com/5c036cd7729d6cfaa6c390bfc070fdc02a8d800d
Cr-Commit-Position: refs/heads/master@{#29662}

[Dan Ehrenberg, 2015-07-13 23:05:34]

Patchset #2 (id:20001) has been deleted

[adamk, 2015-07-13 23:55:59]

adamk@chromium.org changed reviewers:
+ wingo@igalia.com
- ulan@chromium.org

[adamk, 2015-07-13 23:55:59]

Can you add an error message test for the arrow function case?

I'm also curious whether we can detect which case we're dealing with. Is that at
all a possibility? The wording of the new error message sounds a bit odd to me,
grammatically ("Strict mode and arrow functions" is hard to parse).

If we have to combine the two, I'd shorten it to something like:

"Duplicate parameter name not allowed in this context"

or

"Duplicate parameter name not allowed here"

Also, wingo is probably a better reviewer for another eye on arrow functions, if
need be (swapped for ulan).

[Dan Ehrenberg, 2015-07-14 03:21:37]

On 2015/07/13 23:55:59, adamk wrote:
> Can you add an error message test for the arrow function case?
> 
> I'm also curious whether we can detect which case we're dealing with. Is that
at
> all a possibility? The wording of the new error message sounds a bit odd to
me,
> grammatically ("Strict mode and arrow functions" is hard to parse).
> 
> If we have to combine the two, I'd shorten it to something like:
> 
> "Duplicate parameter name not allowed in this context"
> 
> or
> 
> "Duplicate parameter name not allowed here"
> 
> Also, wingo is probably a better reviewer for another eye on arrow functions,
if
> need be (swapped for ulan).

It can definitely be done, but it'll be a little bit of bookkeeping (which may
or may not fit the style of the code). Neither the strictness nor whether it's
parsing an arrow function are known locally when the error is recorded most of
the time, and errors aren't usually modified later (as it could be from
ValidateFormalParameters). Not sure how important this is; what do you think?
I'll put wingo as a reviewer when I send out the next version.

[Dan Ehrenberg, 2015-07-14 17:38:37]

On 2015/07/14 03:21:37, Dan Ehrenberg wrote:
> On 2015/07/13 23:55:59, adamk wrote:
> > Can you add an error message test for the arrow function case?
> > 
> > I'm also curious whether we can detect which case we're dealing with. Is
that
> at
> > all a possibility? The wording of the new error message sounds a bit odd to
> me,
> > grammatically ("Strict mode and arrow functions" is hard to parse).
> > 
> > If we have to combine the two, I'd shorten it to something like:
> > 
> > "Duplicate parameter name not allowed in this context"
> > 
> > or
> > 
> > "Duplicate parameter name not allowed here"
> > 
> > Also, wingo is probably a better reviewer for another eye on arrow
functions,
> if
> > need be (swapped for ulan).
> 
> It can definitely be done, but it'll be a little bit of bookkeeping (which may
> or may not fit the style of the code). Neither the strictness nor whether it's
> parsing an arrow function are known locally when the error is recorded most of
> the time, and errors aren't usually modified later (as it could be from
> ValidateFormalParameters). Not sure how important this is; what do you think?
> I'll put wingo as a reviewer when I send out the next version.

Used the suggested error message in the new version.

[adamk, 2015-07-14 18:10:40]

Sorry to ask again, but can you add a message test for the arrow function case?

[Dan Ehrenberg, 2015-07-14 19:09:36]

On 2015/07/14 18:10:40, adamk wrote:
> Sorry to ask again, but can you add a message test for the arrow function
case?

Done, sorry for not doing that the first time.

[adamk, 2015-07-14 19:16:19]

lgtm % flag (see below)

https://codereview.chromium.org/1236863008/diff/80001/test/message/arrow-form...
File test/message/arrow-formal-parameters.js (right):

https://codereview.chromium.org/1236863008/diff/80001/test/message/arrow-form...
test/message/arrow-formal-parameters.js:4: 
Please add a --harmony-arrow-functions flag line

[Dan Ehrenberg, 2015-07-14 20:24:25]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-07-14 20:24:26]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1236863008/#ps100001
(title: "pass flags to test")

[commit-bot: I haz the power, 2015-07-14 20:24:45]

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

[commit-bot: I haz the power, 2015-07-14 20:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-14 20:38:50]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/7661)

(exceeded global retry quota)

[Dan Ehrenberg, 2015-07-14 20:59:17]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-07-14 20:59:17]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1236863008/#ps120001
(title: "Fix webkit test")

[commit-bot: I haz the power, 2015-07-14 20:59:27]

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

[commit-bot: I haz the power, 2015-07-14 21:59:00]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-07-14 21:59:13]

Patchset 6 (id:??) landed as
https://crrev.com/5c036cd7729d6cfaa6c390bfc070fdc02a8d800d
Cr-Commit-Position: refs/heads/master@{#29662}

[wingo, 2015-07-15 08:55:15]

I would note that naming a test "strict-formal-parameters" as opposed to arrow
formal parameters can be misleading as arrow function parameters are indeed
StringFormalParameters, which is a strange grammar production in the spec which
does not imply strict mode, only that params should not be duplicates.  ACK that
that's impossible to communicate to a user though :)

I think I would have kept the error messages apart.  Better to be told "arrow
functions can't have duplicate parameters" than "duplicate parameters not
allowed in this context".  YMMV though.

[adamk, 2015-07-15 20:05:01]

On 2015/07/15 08:55:15, wingo wrote:
> I would note that naming a test "strict-formal-parameters" as opposed to arrow
> formal parameters can be misleading as arrow function parameters are indeed
> StringFormalParameters, which is a strange grammar production in the spec
which
> does not imply strict mode, only that params should not be duplicates.  ACK
that
> that's impossible to communicate to a user though :)
> 
> I think I would have kept the error messages apart.  Better to be told "arrow
> functions can't have duplicate parameters" than "duplicate parameters not
> allowed in this context".  YMMV though.

I don't think it's a big deal (I suspect that duplicate parameter names are
always a programming error), but I'd be happy to review a patch that added the
necessary bookkeeping to produce different error messages for these cases (in
the same patch one could probably also fix the arrow function message to point
to the dup parameter).