Last Comment Bug 783978 - disallow duplicated argument names when default arguments are used
: disallow duplicated argument names when default arguments are used
Status: RESOLVED FIXED
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
: general
Mentors:
Depends on:
Blocks: jsfunfuzz 767013
  Show dependency treegraph
 
Reported: 2012-08-20 03:01 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-08-21 19:09 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (12.50 KB, text/plain)
2012-08-20 03:01 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fix (12.04 KB, patch)
2012-08-20 11:55 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-08-20 03:01:42 PDT
Created attachment 653325 [details]
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] 2012-08-20 03:16:14 PDT
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] 2012-08-20 11:55:15 PDT
Created attachment 653460 [details] [diff] [review]
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] 2012-08-21 08:49:31 PDT
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] 2012-08-21 09:00:33 PDT
(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 6 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:09:44 PDT
https://hg.mozilla.org/mozilla-central/rev/c5d905357e1b

Note You need to log in before you can comment on or make changes to this bug.