1096376 - Don't allow duplicate parameter names when rest-parameter is present

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
function f(a, ...a){}
---

Expected: SyntaxError is thrown
Actual: No SyntaxError

When a rest-parameter is present the parameters list is no longer considered to be simple [1]. And non-simple parameter lists don't allow duplicate parameter names [2].

[1] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-static-semantics-issimpleparameterlist
[2] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-static-semantics-early-errors

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Disallow duplicated parameter when rest parameter is present in non-strict mode.

Moved `disallowDuplicateArgs` definition to the outside of the loop, and update it for each condition change (including `hasRest` for this bug), to make it easier to fix bug 1096377 and bug 1096378.

Also, check `duplicatedArg` for other parameters if rest parameter is found.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91855352a697
  (removed unnecessary `if` from the try though)

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8549635 [details] [diff] [review]
Disallow duplicated parameter when rest parameter is present in non-strict mode.

Review of attachment 8549635 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding to more appropriate reviewer for rest parameters.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8549635 [details] [diff] [review]
Disallow duplicated parameter when rest parameter is present in non-strict mode.

Review of attachment 8549635 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.

I had trouble convincing myself this is correct. Please move the `disallowDuplicateArgs = true;` lines so that it's clear the rules are enforced exactly in those places where we discover they need to be enforced. r=me with that change.

::: js/src/frontend/Parser.cpp
@@ +1658,5 @@
>                      return false;
>                  }
>  
>                  funbox->hasDestructuringArgs = true;
> +                disallowDuplicateArgs = true;

Please move this line just before the `if (duplicatedArg) {` line above.

@@ +1714,5 @@
>                             JSMSG_ACCESSOR_WRONG_ARGS, "setter", "one", "");
>                      return false;
>                  }
>                  *hasRest = true;
> +                disallowDuplicateArgs = true;

Please move this line just before the `if (duplicatedArg) {` line below.

@@ +1759,5 @@
>                          return false;
>                      }
>                      if (!hasDefaults) {
>                          hasDefaults = true;
> +                        disallowDuplicateArgs = true;

Please move this line just before the `if (duplicatedArg) {` line above.

Comment 4

[Tooru Fujisawa [:arai]]

Thank you! :D
Moved them and landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6c10b02156

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b6c10b02156

Comment 6

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility