Closed Bug 1096376 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: anba, Assigned: arai)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

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
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)
Attachment #8549635 - Flags: review?(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.
Attachment #8549635 - Flags: review?(luke) → review?(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.
Attachment #8549635 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/4b6c10b02156
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.