Bug 630770 - Correctly warn about duplicate parameters when the strict option is enabled. r=jimb
authorJeff Walden <jwalden@mit.edu>
Wed, 02 Feb 2011 01:33:23 -0800
changeset 62063 e518c56eaffd8536e9b5c6bc1080a12607c80ee0
parent 62062 5f7eb378dac68f1444bd8747e11f0572bca4daa7
child 62064 73efdea6fda017564243a2c5e23dc136fc487cee
push idunknown
push userunknown
push dateunknown
reviewersjimb
bugs630770
milestone2.0b11pre
Bug 630770 - Correctly warn about duplicate parameters when the strict option is enabled. r=jimb
js/src/jsparse.cpp
js/src/tests/ecma_5/extensions/jstests.list
js/src/tests/ecma_5/extensions/strict-option-redeclared-parameter.js
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -1465,17 +1465,18 @@ CheckStrictParameters(JSContext *cx, JST
     JS_ASSERT(tc->inFunction());
 
     if (!tc->needStrictChecks() || tc->bindings.countArgs() == 0)
         return true;
 
     JSAtom *argumentsAtom = cx->runtime->atomState.argumentsAtom;
     JSAtom *evalAtom = cx->runtime->atomState.evalAtom;
 
-    HashSet<JSAtom *> parameters(cx);
+    /* name => whether we've warned about the name already */
+    HashMap<JSAtom *, bool> parameters(cx);
     if (!parameters.init(tc->bindings.countArgs()))
         return false;
 
     /* Start with lastVariable(), not lastArgument(), for destructuring. */
     for (Shape::Range r = tc->bindings.lastVariable(); !r.empty(); r.popFront()) {
         jsid id = r.front().id;
         if (!JSID_IS_ATOM(id))
             continue;
@@ -1491,22 +1492,28 @@ CheckStrictParameters(JSContext *cx, JST
             /*
              * JSOPTION_STRICT is supposed to warn about future keywords, too,
              * but we took care of that in the scanner.
              */
             JS_ALWAYS_TRUE(!ReportBadParameter(cx, tc, name, JSMSG_RESERVED_ID));
             return false;
         }
 
-        HashSet<JSAtom *>::AddPtr p = parameters.lookupForAdd(name);
-        if (p && !ReportBadParameter(cx, tc, name, JSMSG_DUPLICATE_FORMAL))
-            return false;
-
-        if (!parameters.add(p, name))
-            return false;
+        /*
+         * Check for a duplicate parameter: warn or report an error exactly
+         * once for each duplicated parameter.
+         */
+        if (HashMap<JSAtom *, bool>::AddPtr p = parameters.lookupForAdd(name)) {
+            if (!p->value && !ReportBadParameter(cx, tc, name, JSMSG_DUPLICATE_FORMAL))
+                return false;
+            p->value = true;
+        } else {
+            if (!parameters.add(p, name, false))
+                return false;
+        }
     }
 
     return true;
 }
 
 JSParseNode *
 Parser::functionBody()
 {
--- a/js/src/tests/ecma_5/extensions/jstests.list
+++ b/js/src/tests/ecma_5/extensions/jstests.list
@@ -14,15 +14,16 @@ script extension-methods-reject-null-und
 skip-if(!xulRuntime.shell) script function-definition-with.js # needs evaluate()
 script iterator-in-catch.js
 fails script nested-delete-name-in-evalcode.js # bug 604301, at a minimum
 script proxy-strict.js
 script regress-bug567606.js
 script regress-bug607284.js
 script regress-bug629723.js
 script strict-function-statements.js
+script strict-option-redeclared-parameter.js
 script string-literal-getter-setter-decompilation.js
 script uneval-strict-functions.js
 script watch-inherited-property.js
 script watch-replaced-setter.js
 script watch-setter-become-setter.js
 script watch-value-prop-becoming-setter.js
 script watchpoint-deletes-JSPropertyOp-setter.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/strict-option-redeclared-parameter.js
@@ -0,0 +1,30 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 630770;
+var summary =
+  'Correctly warn about duplicate parameters when the strict option is enabled';
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+// Verify that duplicate parameters, with the strict option set, don't provoke
+// an assertion.  Ideally we'd also verify that we warn exactly once per
+// duplicated parameter name, but at present there's no way to test that
+// without more effort (further customizing the shell JSErrorReporter) than we
+// want to make now.
+options("strict");
+eval("function a(x, x, x, x) { }");
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("All tests passed!");