From 5bfdb45b63fdd451409af7b0f76203954c94b1d2 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 26 Nov 2009 10:23:52 -0800 Subject: [PATCH] Bug 499524: Always check for duplicates when destructuring params are present. r=igor Detect duplicate names in parameter lists that include destructuring parameters, regardless of whether the duplication becomes before or after the destructuring. Let strict mode complaints take care of themselves after the body has been parsed. In BindDestructuringArg, there should never be an entry in tc->decls for the given name if the call to js_LookupLocal didn't detect a duplicate argument, so we can simply assert that tc->decl.lookup returns NULL, instead of checking it. In HashLocalName, we can tighten the assertion: both the new and existing entries must be JSLOCAL_ARG, since we detect all non-ARG (i.e., destructuring) duplicates early. --- js/src/jsfun.cpp | 2 +- js/src/jsparse.cpp | 51 ++++++++++---------- js/src/tests/js1_8/regress/jstests.list | 5 +- js/src/tests/js1_8/regress/regress-499524.js | 47 ++++++++++++++++++ 4 files changed, 77 insertions(+), 28 deletions(-) create mode 100644 js/src/tests/js1_8/regress/regress-499524.js diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp index f0821d16558c..6323fcc6cffb 100644 --- a/js/src/jsfun.cpp +++ b/js/src/jsfun.cpp @@ -2700,7 +2700,7 @@ HashLocalName(JSContext *cx, JSLocalNameMap *map, JSAtom *name, } if (entry->name) { JS_ASSERT(entry->name == name); - JS_ASSERT(entry->localKind == JSLOCAL_ARG); + JS_ASSERT(entry->localKind == JSLOCAL_ARG && localKind == JSLOCAL_ARG); dup = (JSNameIndexPair *) cx->malloc(sizeof *dup); if (!dup) return JS_FALSE; diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp index 0f27a02194fa..6b6aea8522ee 100644 --- a/js/src/jsparse.cpp +++ b/js/src/jsparse.cpp @@ -1731,7 +1731,6 @@ static JSBool BindDestructuringArg(JSContext *cx, BindData *data, JSAtom *atom, JSTreeContext *tc) { - JSAtomListElement *ale; JSParseNode *pn; /* Flag tc so we don't have to lookup arguments on every use. */ @@ -1741,10 +1740,6 @@ BindDestructuringArg(JSContext *cx, BindData *data, JSAtom *atom, tc->flags |= TCF_FUN_PARAM_EVAL; JS_ASSERT(tc->flags & TCF_IN_FUNCTION); - ale = tc->decls.lookup(atom); - pn = data->pn; - if (!ale && !Define(pn, atom, tc)) - return JS_FALSE; JSLocalKind localKind = js_LookupLocal(cx, tc->fun, atom, NULL); if (localKind != JSLOCAL_NONE) { @@ -1752,6 +1747,11 @@ BindDestructuringArg(JSContext *cx, BindData *data, JSAtom *atom, JSREPORT_ERROR, JSMSG_DESTRUCT_DUP_ARG); return JS_FALSE; } + JS_ASSERT(!tc->decls.lookup(atom)); + + pn = data->pn; + if (!Define(pn, atom, tc)) + return JS_FALSE; uintN index = tc->fun->u.i.nvars; if (!BindLocalVariable(cx, tc->fun, atom, JSLOCAL_VAR, true)) @@ -2578,7 +2578,8 @@ FunctionDef(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc, JSAtomListElement *ale; #if JS_HAS_DESTRUCTURING JSParseNode *item, *list = NULL; - bool destructuringArg = false, duplicatedArg = false; + bool destructuringArg = false; + JSAtom *duplicatedArg = NULL; #endif /* Make a TOK_FUNCTION node. */ @@ -2807,27 +2808,26 @@ FunctionDef(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc, case TOK_NAME: { - /* - * Check for a duplicate parameter name, a "feature" that - * ECMA-262 requires. This is a SpiderMonkey strict warning, - * and a strict mode error, as of ECMAScript 5. - * - * Further, if any argument is a destructuring pattern, forbid - * duplicates. We will report the error either now if we have - * seen a destructuring pattern already, or later when we find - * the first pattern. - */ JSAtom *atom = CURRENT_TOKEN(ts).t_atom; - if (tc->needStrictChecks() && - js_LookupLocal(cx, fun, atom, NULL) != JSLOCAL_NONE) { -#if JS_HAS_DESTRUCTURING - if (destructuringArg) - goto report_dup_and_destructuring; - duplicatedArg = true; -#endif - } if (!DefineArg(pn, atom, fun->nargs, &funtc)) return NULL; +#ifdef JS_HAS_DESTRUCTURING + /* + * ECMA-262 requires us to support duplicate parameter names, but if the + * parameter list includes destructuring, we consider the code to have + * opted in to higher standards, and forbid duplicates. We may see a + * destructuring parameter later, so always note duplicates now. + * + * Duplicates are warned about (strict option) or cause errors (strict + * mode code), but we do those tests in one place below, after having + * parsed the body. + */ + if (js_LookupLocal(cx, fun, atom, NULL) != JSLOCAL_NONE) { + duplicatedArg = atom; + if (destructuringArg) + goto report_dup_and_destructuring; + } +#endif if (!js_AddLocal(cx, fun, atom, JSLOCAL_ARG)) return NULL; break; @@ -2842,7 +2842,8 @@ FunctionDef(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc, #if JS_HAS_DESTRUCTURING report_dup_and_destructuring: - js_ReportCompileErrorNumber(cx, TS(tc->compiler), NULL, + JSDefinition *dn = ALE_DEFN(funtc.decls.lookup(duplicatedArg)); + js_ReportCompileErrorNumber(cx, TS(tc->compiler), dn, JSREPORT_ERROR, JSMSG_DESTRUCT_DUP_ARG); return NULL; diff --git a/js/src/tests/js1_8/regress/jstests.list b/js/src/tests/js1_8/regress/jstests.list index d3f2e9416345..bafa09715ffc 100644 --- a/js/src/tests/js1_8/regress/jstests.list +++ b/js/src/tests/js1_8/regress/jstests.list @@ -10,8 +10,8 @@ script regress-433279-03.js skip script regress-442333-01.js script regress-452491.js script regress-453492.js -skip-if(isDebugBuild) fails-if(!isDebugBuild) script regress-455981-01.js -skip-if(isDebugBuild) fails-if(!isDebugBuild) script regress-455981-02.js +script regress-455981-01.js +script regress-455981-02.js script regress-457065-01.js script regress-457065-02.js script regress-458076.js @@ -81,3 +81,4 @@ script regress-479353.js script regress-479740.js script regress-481800.js script regress-483749.js +script regress-499524.js diff --git a/js/src/tests/js1_8/regress/regress-499524.js b/js/src/tests/js1_8/regress/regress-499524.js new file mode 100644 index 000000000000..1c90b9811c2d --- /dev/null +++ b/js/src/tests/js1_8/regress/regress-499524.js @@ -0,0 +1,47 @@ +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + +/* + * Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/licenses/publicdomain/ + */ + +function isSyntaxError(code) { + try { + eval(code); + return false; + } catch (exception) { + if (SyntaxError.prototype.isPrototypeOf(exception)) + return true; + throw exception; + }; +}; + +/* + * Duplicate parameter names must be tolerated (as per ES3), unless + * the parameter list uses destructuring, in which case we claim the + * user has opted in to a modicum of sanity, and we forbid duplicate + * parameter names. + */ +assertEq(isSyntaxError("function f(x,x){}"), false); + +assertEq(isSyntaxError("function f(x,[x]){})"), true); +assertEq(isSyntaxError("function f(x,{y:x}){})"), true); +assertEq(isSyntaxError("function f(x,{x}){})"), true); + +assertEq(isSyntaxError("function f([x],x){})"), true); +assertEq(isSyntaxError("function f({y:x},x){})"), true); +assertEq(isSyntaxError("function f({x},x){})"), true); + +assertEq(isSyntaxError("function f([x,x]){}"), true); +assertEq(isSyntaxError("function f({x,x}){}"), true); +assertEq(isSyntaxError("function f({y:x,z:x}){}"), true); + +assertEq(isSyntaxError("function f(x,x,[y]){}"), true); +assertEq(isSyntaxError("function f(x,x,{y}){}"), true); +assertEq(isSyntaxError("function f([y],x,x){}"), true); +assertEq(isSyntaxError("function f({y},x,x){}"), true); + +assertEq(isSyntaxError("function f(a,b,c,d,e,f,g,h,b,[y]){}"), true); +assertEq(isSyntaxError("function f([y],a,b,c,d,e,f,g,h,a){}"), true); +assertEq(isSyntaxError("function f([a],b,c,d,e,f,g,h,i,a){}"), true); +assertEq(isSyntaxError("function f(a,b,c,d,e,f,g,h,i,[a]){}"), true);