783978 - disallow duplicated argument names when default arguments are used

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

({
    get "" (x = (function() {
        return {
            r: function() {
                for (e in x) {}
            }
        }
}), x

asserts js debug shell on m-c changeset 35b8d6ef5d46 without any CLI arguments at Assertion failure: !prevDecl->isClosed(),

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   102515:abc8c217f032
user:        Luke Wagner
date:        Mon Aug 06 07:56:45 2012 -0700
summary:     Bug 767013 - only store aliased variables in scope objects (r=bhackett)

Comment 2

[Luke Wagner [:luke]]

Attachment: fix

D'oh, default arguments allow one to alias a formal before it is duplicated.

Fixing this to maintain the unaliased behavior is impossible: in the unaliased case for function f(x,y=x,x), y will bind to arguments[0] and arguments[0] is not even present in the call object (that is what bug 767013 did).  Rather than changing the aliased/unaliased behavior so that 'y' would refer to arguments[2] (non-trivial in our 1-pass name binding algo), jorendorff, dherman, and I are of the opinion that we should just disallow duplicate arguments when defaults are used.  This follows the "you use new syntax, you cannot use old bad syntax" precedent set by destructuring args, which also disallow dups.

Note: I had to change the error text in js.msg.  Instead of explicitly including a mention of duplicates, I generalized it to "in this context" (which will cover any other cases we choose to disallow, like rest params).  "In this context" is a bit vague, but I believe the message is still very actionable.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 653460 [details] [diff] [review]
fix

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

::: js/src/frontend/Parser.cpp
@@ +1020,4 @@
>   *
>   * If 'duplicatedArg' is non-null, then DefineArg assigns to it any previous
> + * argument with the same name. The caller may use this to report an error when
> + * one of the abovementioned features occurs after a duplicate.

Instead of implementing a check for duplicates after destructuring/defaults and another check for destructuring/defaults after duplicates, could we track both and do a single check at the end of argument parsing? (This might make it convenient to emit a more specific error message, too.)

Comment 4

[Luke Wagner [:luke]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Instead of implementing a check for duplicates after destructuring/defaults
> and another check for destructuring/defaults after duplicates, could we
> track both and do a single check at the end of argument parsing? (This might
> make it convenient to emit a more specific error message, too.)

That does sound better, but it would require us to loosen the assertion that caught this bug (the one that asserts that, if you add a duplicate, the original wasn't previously closed-over).

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d905357e1b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5d905357e1b