Bug 514567: Detect duplicate property names. r=jorendorff
authorJim Blandy <jimb@mozilla.org>
Thu, 19 Nov 2009 12:35:55 -0800
changeset 35310 37c3734c11f8d9a01fe9f685ede03b13fb4da75b
parent 35309 02f8b1993ae1508a7f7805e3a7eeebf89a11ca73
child 35311 b23078c3e55afba4d277f197a0d8a24bff6bcef0
push id10560
push userrsayre@mozilla.com
push dateTue, 01 Dec 2009 18:15:12 +0000
treeherdermozilla-central@e2860a4dcf0c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs514567
milestone1.9.3a1pre
Bug 514567: Detect duplicate property names. r=jorendorff
js/src/js.msg
js/src/jsemit.h
js/src/jsparse.cpp
js/src/tests/ecma_5/strict/11.1.5.js
js/src/tests/ecma_5/strict/jstests.list
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -308,8 +308,9 @@ MSG_DEF(JSMSG_CANT_SET_ARRAY_ATTRS,   22
 MSG_DEF(JSMSG_EVAL_ARITY,             226, 0, JSEXN_TYPEERR, "eval accepts only one parameter")
 MSG_DEF(JSMSG_MISSING_FUN_ARG,        227, 2, JSEXN_TYPEERR, "missing argument {0} when calling function {1}")
 MSG_DEF(JSMSG_JSON_BAD_PARSE,         228, 0, JSEXN_SYNTAXERR, "JSON.parse")
 MSG_DEF(JSMSG_JSON_BAD_STRINGIFY,     229, 0, JSEXN_ERR, "JSON.stringify")
 MSG_DEF(JSMSG_XDR_CLOSURE_WRAPPER,    230, 1, JSEXN_INTERNALERR, "can't XDR closure wrapper for function {0}")
 MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,     231, 0, JSEXN_TYPEERR, "value is not a non-null object")
 MSG_DEF(JSMSG_DEPRECATED_OCTAL,       232, 0, JSEXN_SYNTAXERR, "octal literals and octal escape sequences are deprecated")
 MSG_DEF(JSMSG_STRICT_CODE_WITH,       233, 0, JSEXN_SYNTAXERR, "strict mode code may not contain 'with' statements")
+MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     234, 1, JSEXN_SYNTAXERR, "property name {0} appears more than once in object literal")
--- a/js/src/jsemit.h
+++ b/js/src/jsemit.h
@@ -221,16 +221,18 @@ struct JSTreeContext {              /* t
 
     uintN blockid() { return topStmt ? topStmt->blockid : bodyid; }
 
     bool atTopLevel() { return !topStmt || (topStmt->flags & SIF_BODY_BLOCK); }
 
     /* Test whether we're in a statement of given type. */
     bool inStatement(JSStmtType type);
 
+    inline bool needStrictChecks();
+
     /* 
      * sharpSlotBase is -1 or first slot of pair for [sharpArray, sharpDepth].
      * The parser calls ensureSharpSlots to allocate these two stack locals.
      */
     int sharpSlotBase;
     bool ensureSharpSlots();
 };
 
@@ -298,16 +300,25 @@ struct JSTreeContext {              /* t
                                  TCF_FUN_PARAM_ARGUMENTS |                    \
                                  TCF_FUN_HEAVYWEIGHT     |                    \
                                  TCF_FUN_IS_GENERATOR    |                    \
                                  TCF_FUN_USES_OWN_NAME   |                    \
                                  TCF_HAS_SHARPS          |                    \
                                  TCF_STRICT_MODE_CODE)
 
 /*
+ * Return true if we need to check for conditions that elicit
+ * JSOPTION_STRICT warnings or strict mode errors.
+ */
+inline bool JSTreeContext::needStrictChecks() {
+    return JS_HAS_STRICT_OPTION(compiler->context) ||
+           (flags & TCF_STRICT_MODE_CODE);
+}
+
+/*
  * Span-dependent instructions are jumps whose span (from the jump bytecode to
  * the jump target) may require 2 or 4 bytes of immediate operand.
  */
 typedef struct JSSpanDep    JSSpanDep;
 typedef struct JSJumpTarget JSJumpTarget;
 
 struct JSSpanDep {
     ptrdiff_t       top;        /* offset of first bytecode in an opcode */
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -7967,73 +7967,87 @@ PrimaryExpr(JSContext *cx, JSTokenStream
         return pn;
       }
 
       case TOK_LC:
       {
         JSBool afterComma;
         JSParseNode *pnval;
 
+        /*
+         * A map from property names we've seen thus far to bit masks.
+         * (We use ALE_INDEX/ALE_SET_INDEX).  An atom's mask includes
+         * JSPROP_SETTER if we've seen a setter for it, JSPROP_GETTER
+         * if we've seen as getter, and both of those if we've just
+         * seen an ordinary value.
+         */
+        JSAutoAtomList seen(tc->compiler);
+
         pn = NewParseNode(PN_LIST, tc);
         if (!pn)
             return NULL;
         pn->pn_type = TOK_RC;
         pn->pn_op = JSOP_NEWINIT;
         pn->makeEmpty();
 
         afterComma = JS_FALSE;
         for (;;) {
+            JSAtom *atom;
             ts->flags |= TSF_KEYWORD_IS_NAME;
             tt = js_GetToken(cx, ts);
             ts->flags &= ~TSF_KEYWORD_IS_NAME;
             switch (tt) {
               case TOK_NUMBER:
                 pn3 = NewParseNode(PN_NULLARY, tc);
                 if (!pn3)
                     return NULL;
                 pn3->pn_dval = CURRENT_TOKEN(ts).t_dval;
+                if (tc->needStrictChecks())
+                    atom = js_AtomizeDouble(cx, pn3->pn_dval);
+                else
+                    atom = NULL; /* for the compiler */
                 break;
               case TOK_NAME:
 #if JS_HAS_GETTER_SETTER
                 {
-                    JSAtom *atom;
-
                     atom = CURRENT_TOKEN(ts).t_atom;
                     if (atom == cx->runtime->atomState.getAtom)
                         op = JSOP_GETTER;
                     else if (atom == cx->runtime->atomState.setAtom)
                         op = JSOP_SETTER;
                     else
                         goto property_name;
 
                     ts->flags |= TSF_KEYWORD_IS_NAME;
                     tt = js_GetToken(cx, ts);
                     ts->flags &= ~TSF_KEYWORD_IS_NAME;
                     if (tt != TOK_NAME) {
                         js_UngetToken(ts);
                         goto property_name;
                     }
-                    pn3 = NewNameNode(cx, CURRENT_TOKEN(ts).t_atom, tc);
+                    atom = CURRENT_TOKEN(ts).t_atom;
+                    pn3 = NewNameNode(cx, atom, tc);
                     if (!pn3)
                         return NULL;
 
                     /* We have to fake a 'function' token here. */
                     CURRENT_TOKEN(ts).t_op = JSOP_NOP;
                     CURRENT_TOKEN(ts).type = TOK_FUNCTION;
                     pn2 = FunctionExpr(cx, ts, tc);
                     pn2 = NewBinary(TOK_COLON, op, pn3, pn2, tc);
                     goto skip;
                 }
               property_name:
 #endif
               case TOK_STRING:
+                atom = CURRENT_TOKEN(ts).t_atom;
                 pn3 = NewParseNode(PN_NULLARY, tc);
                 if (!pn3)
                     return NULL;
-                pn3->pn_atom = CURRENT_TOKEN(ts).t_atom;
+                pn3->pn_atom = atom;
                 break;
               case TOK_RC:
                 goto end_obj_init;
               default:
                 js_ReportCompileErrorNumber(cx, ts, NULL, JSREPORT_ERROR,
                                             JSMSG_BAD_PROP_ID);
                 return NULL;
             }
@@ -8078,16 +8092,53 @@ PrimaryExpr(JSContext *cx, JSTokenStream
             pn2 = NewBinary(TOK_COLON, op, pn3, pnval, tc);
 #if JS_HAS_GETTER_SETTER
           skip:
 #endif
             if (!pn2)
                 return NULL;
             pn->append(pn2);
 
+            /*
+             * In strict mode code, check for duplicate property names.  Treat
+             * getters and setters as distinct attributes of each property.  A
+             * plain old value conflicts with a getter or a setter.
+             */ 
+            if (tc->needStrictChecks()) {
+                unsigned attributesMask;
+                if (op == JSOP_NOP)
+                    attributesMask = JSPROP_GETTER | JSPROP_SETTER;
+                else if (op == JSOP_GETTER)
+                    attributesMask = JSPROP_GETTER;
+                else if (op == JSOP_SETTER)
+                    attributesMask = JSPROP_SETTER;
+                else {
+                    JS_NOT_REACHED("bad opcode in object initializer");
+                    attributesMask = 0;
+                }
+
+                JSAtomListElement *ale = seen.lookup(atom);
+                if (ale) {
+                    if (ALE_INDEX(ale) & attributesMask) {
+                        const char *name = js_AtomToPrintableString(cx, atom);
+                        if (!name ||
+                            !js_ReportStrictModeError(cx, ts, tc, NULL,
+                                                      JSMSG_DUPLICATE_PROPERTY, name)) {
+                            return NULL;
+                        }
+                    }
+                    ALE_SET_INDEX(ale, attributesMask | ALE_INDEX(ale));
+                } else {
+                    ale = seen.add(tc->compiler, atom);
+                    if (!ale)
+                        return NULL;
+                    ALE_SET_INDEX(ale, attributesMask);
+                }
+            }                    
+
             tt = js_GetToken(cx, ts);
             if (tt == TOK_RC)
                 goto end_obj_init;
             if (tt != TOK_COMMA) {
                 js_ReportCompileErrorNumber(cx, ts, NULL, JSREPORT_ERROR,
                                             JSMSG_CURLY_AFTER_LIST);
                 return NULL;
             }
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/strict/11.1.5.js
@@ -0,0 +1,140 @@
+/* -*- 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/
+ */
+
+/* Simple identifier labels. */
+assertEq(testLenientAndStrict('({x:1, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({x:1, y:1})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+assertEq(testLenientAndStrict('({x:1, y:1, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+/* Property names can be written as strings, too. */
+assertEq(testLenientAndStrict('({x:1,   "x":1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({"x":1, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({"x":1, "x":1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+/* Numeric property names. */
+assertEq(testLenientAndStrict('({1.5:1, 1.5:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({1.5:1, 15e-1:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({6.02214179e23:1, 6.02214179e23:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({6.02214179e23:1, 3.1415926535:1})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+/* Many properties, to exercise JSAtomList's hash-table variant. */
+assertEq(testLenientAndStrict('({a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1, i:1, j:1, k:1, l:1, m:1, n:1, o:1, p:1, q:1, r:1, s:1, t:1, u:1, v:1, w:1, x:1, y:1, z:1})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+assertEq(testLenientAndStrict('({a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1, i:1, j:1, k:1, l:1, m:1, n:1, o:1, p:1, q:1, r:1, s:1, t:1, u:1, v:1, w:1, x:1, y:1, a:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+/*
+ * Getters, setters, and value properties should conflict only when
+ * appropriate.
+ */
+assertEq(testLenientAndStrict('({get x() {}, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({x:1, get x() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({set x() {}, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({x:1, set x() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({get x() {}, set x() {}})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+assertEq(testLenientAndStrict('({set x() {}, get x() {}})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+assertEq(testLenientAndStrict('({get x() {}, set x() {}, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({set x() {}, get x() {}, x:1})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({get x() {}, get x() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({get x() {}, set x() {}, y:1})',
+                              parsesSuccessfully,
+                              parsesSuccessfully),
+         true);
+
+/* Use the old getter/setter syntax as well. */
+assertEq(testLenientAndStrict('({get x() {}, x getter: function() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({x getter: function() {}, get x() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
+
+assertEq(testLenientAndStrict('({x getter: function() {}, x getter: function() {}})',
+                              parsesSuccessfully,
+                              parseRaisesException(SyntaxError)),
+         true);
--- a/js/src/tests/ecma_5/strict/jstests.list
+++ b/js/src/tests/ecma_5/strict/jstests.list
@@ -1,6 +1,7 @@
 url-prefix ../../jsreftest.html?test=ecma_5/strict/
 script 8.7.2.js
 script 10.4.2.js
+script 11.1.5.js
 script 12.10.1.js
 script B.1.1.js
 script B.1.2.js