514567 - ES5 strict mode: Object literals may not contain repeated data properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

From Annex C:

It is a syntax error if strict mode code contains an ObjectLiteral with more than one definition of any data property (11.1.5).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

We already have a strict warning when code does this.

Comment 2

[Jim Blandy :jimb]

Yeah, I think we already have warnings for a lot of these conditions.  The work to be done here is simply to enable them as appropriate in strict mode code.

Comment 3

[Jim Blandy :jimb]

Attachment: Fix error processing in PrimaryExpr. [landed]

This code seems to assume that one can proceed if pn3 is NULL, but we
never check it again further down.  Instead, we create TOK_COLON nodes
whose left (and perhaps right) children are NULL.  It seems to me that
the TOK_RC case in js_EmitTree will choke on this.

Comment 4

[Brendan Eich [:brendan]]

NewBinary returns null if either kid is null, which is why this old code tried to "save code size" by not doing the usual null-test and early return. But then the destructuring shorthand change came in between and used pn3, breaking the hard to see connection between the null-tolerance above and the NewBinary call below.

Thanks for fixing -- r=me if you want it.

/be

Comment 5

[Jim Blandy :jimb]

Attachment: Define JSAutoAtomList, a variant of JSAutoAtomList with a destructor.

Comment 6

[Jim Blandy :jimb]

Attachment: Detect duplicate property names in object literals in ES5 strict mode.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 401939 [details] [diff] [review]
Fix error processing in PrimaryExpr. [landed]

fwiw, NewBinary does null check its arguments, but this is clearer.

Comment 8

[Brendan Eich [:brendan]]

ES5_ prefix on STRICT_MODE helps distinguish from JSOPTION_STRICT, but ES5 will age badly. Can we use something less E(cma) and 5 inflected? MODE vs. OPTION may be enough (or not ;-).

/be

Comment 9

[Jim Blandy :jimb]

(In reply to comment #8)
> ES5 will
> age badly. Can we use something less E(cma) and 5 inflected? MODE vs. OPTION
> may be enough (or not ;-).

I see.  I'd been thinking of the semantics of this flag as tied to this specific edition of the standard, but that's not right: later standards may add other directives, but they will continue to define "use strict" on their own terms, perhaps making slight changes.  So the succession of directives and the succession of standards are orthogonal.

STRICT_MODE_CODE is the exact phrase used in the standard.  JSOPTION_STRICT is part of the API.  I think we'll just have to document carefully.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2ed35be3e01d

Comment 11

[Jim Blandy :jimb]

That's only one patch landed out of three.

Should I have marked this bug up differently to make it more clear?

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 404916 [details] [diff] [review]
Detect duplicate property names in object literals in ES5 strict mode.

>One ugliness with this patch is that with JSOPTION_STRICT enabled, you
>get two warnings:
>
>[...] it seems to me that we don't really
>need to call js_CheckRedeclaration any more for JSOP_INITPROP.

Sounds good to me. Followup bug?

>+MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     234, 1, JSEXN_SYNTAXERR, "duplicated property name {0} in object literal")

Technically those things are called object initializers, not literals. I'm not sure it's worth fighting what every JS hacker knows to be true, though :) so either way is fine with me.

I think "duplicate" is better than "duplicated", and it would be better still to come up with plainer language, along the lines of:

   property name '{0}' appears multiple times in the same object initializer

Whatever prose you like is good enough for me.

In jsemit.h:
>+    inline bool doStrictChecks();

Maybe needStrictChecks()?  The word "do" gives the wrong impression.

In jsparse.cpp:
>+                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_ASSERT(op == JSOP_NOP || op == JSOP_GETTER ||
>+                              op == JSOP_SETTER);

If I understand correctly, there's a special macro for this kind of assertion:
  JS_NOT_REACHED("bad op in object initializer");

>+                    ALE_SET_INDEX(seen.add(tc->compiler, atom), attributesMask);

add() can return NULL, indicating OOM.

r=me with that fixed.

Comment 13

[Jason Orendorff [:jorendorff]]

I take it back. This seems testable in script (via eval) so r=me with the above fixed and some js/src/tests.

Comment 14

[Robert Sayre]

(In reply to comment #11)
> That's only one patch landed out of three.
> 
> Should I have marked this bug up differently to make it more clear?

Many patches in one bug makes our system sad. I know it's a little annoying when you're incrementally fixing something.

Comment 15

[Jim Blandy :jimb]

(In reply to comment #12)
> (From update of attachment 404916 [details] [diff] [review])
> >One ugliness with this patch is that with JSOPTION_STRICT enabled, you
> >get two warnings:
> >
> >[...] it seems to me that we don't really
> >need to call js_CheckRedeclaration any more for JSOP_INITPROP.
> 
> Sounds good to me. Followup bug?

Filed as bug 5225158.

> >+MSG_DEF(JSMSG_DUPLICATE_PROPERTY,     234, 1, JSEXN_SYNTAXERR, "duplicated property name {0} in object literal")
> 
> Technically those things are called object initializers, not literals. I'm not
> sure it's worth fighting what every JS hacker knows to be true, though :) so
> either way is fine with me.

The grammar production is called "ObjectLiteral".  So I think it's okay.

> I think "duplicate" is better than "duplicated", and it would be better still
> to come up with plainer language, along the lines of:
> 
>    property name '{0}' appears multiple times in the same object initializer
> 
> Whatever prose you like is good enough for me.

You're right that something plainer would be better.  At the moment I've got:

"property name {0} appears more than once in object literal"

> In jsemit.h:
> >+    inline bool doStrictChecks();
> 
> Maybe needStrictChecks()?  The word "do" gives the wrong impression.

Yeah --- good suggestion.

> In jsparse.cpp:
> >+                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_ASSERT(op == JSOP_NOP || op == JSOP_GETTER ||
> >+                              op == JSOP_SETTER);
> 
> If I understand correctly, there's a special macro for this kind of assertion:
>   JS_NOT_REACHED("bad op in object initializer");

Ah, yes.  I'd used that elsewhere, and forgotten.

> >+                    ALE_SET_INDEX(seen.add(tc->compiler, atom), attributesMask);
> 
> add() can return NULL, indicating OOM.

Yeeg.  Fixed.

Comment 16

[Jim Blandy :jimb]

Attachment: Detect duplicate property names.

Refresh, with the changes Jason suggested made.

Comment 17

[Jim Blandy :jimb]

(In reply to comment #13)
> I take it back. This seems testable in script (via eval) so r=me with the above
> fixed and some js/src/tests.

Ah --- I have some tests I've been using during development.  Let me get them folded in.

Comment 18

[Brendan Eich [:brendan]]

"literal" is shorter than "initialiser" and it avoids the Flagrant UK spelling that Ecma and ISO insist upon. Two for two! ;-)

/be

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 406174 [details] [diff] [review]
Detect duplicate property names.

>+inline bool JSTreeContext::needStrictChecks() {
>+    return (JS_HAS_STRICT_OPTION(compiler->context) ||
>+            flags & TCF_STRICT_MODE_CODE);
>+}

Ultra-nit: parenthesize the & expression only, the whole return expr doesn't need parens.

>         JSBool afterComma;
>         JSParseNode *pnval;

Could do with a blank line here?

>+        /*
>+         * 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)
>+            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");

Perhaps here too?

>+                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)) {

|| at end of line

Underhanging trailing args want to be indented more to start in same column as first arg.

/be

Comment 20

[Brendan Eich [:brendan]]

Looks great otherwise, don't mind the nitpicking. I was concerned in bug 522158 comment 1 about a JSOP_INITPROP-only check, where you might need something for JSOP_INITELEM, but the parser can do all the checking. Not sure why we didn't do the old strict warning there... mrbkap may recall.

/be

Comment 21

[Jim Blandy :jimb]

(In reply to comment #18)
> "literal" is shorter than "initialiser" and it avoids the Flagrant UK spelling
> that Ecma and ISO insist upon. Two for two! ;-)

Brendan Eich and Noah Webster --- BFF!

I don't mind the nitpicking at all; I appreciate the care.

Comment 22

[Jim Blandy :jimb]

(In reply to comment #19)
> (From update of attachment 406174 [details] [diff] [review])
> >+inline bool JSTreeContext::needStrictChecks() {
> >+    return (JS_HAS_STRICT_OPTION(compiler->context) ||
> >+            flags & TCF_STRICT_MODE_CODE);
> >+}
> 
> Ultra-nit: parenthesize the & expression only, the whole return expr doesn't
> need parens.

In the past, I've told you that Emacs has difficulty doing this, and needs the parens for guidance.  However, since then I've learned more, and this time I was able to figure out how to Emacs to do the right thing; if anyone's curious, they can try it out at:

http://hg.mozilla.org/users/jblandy_mozilla.com/mozilla-elisp/

I've addressed these nits.

Comment 23

[Jim Blandy :jimb]

Attachment: Detect duplicate property names.

Now --- with tests!  (No code changes.)

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #22)
> (In reply to comment #19)
> > (From update of attachment 406174 [details] [diff] [review] [details])
> > >+inline bool JSTreeContext::needStrictChecks() {
> > >+    return (JS_HAS_STRICT_OPTION(compiler->context) ||
> > >+            flags & TCF_STRICT_MODE_CODE);

Thanks, I had forgotten -- an overparenthesized multiline return is not a big deal, my nit should have been about the near-"precplaint" of the unparenthesized flags & TCF_STRICT_MODE_CODE test.

/be

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 406760 [details] [diff] [review]
Detect duplicate property names.

Jim, please post a patch with updated tests for review.

Comment 26

[Jim Blandy :jimb]

Attachment: Detect duplicate property names.

Tests revised per jorendorff's and Waldo's comments.

Comment 27

[Jim Blandy :jimb]

(In reply to comment #15)
> (In reply to comment #12)
> > (From update of attachment 404916 [details] [diff] [review] [details])
> > >One ugliness with this patch is that with JSOPTION_STRICT enabled, you
> > >get two warnings:
> > >
> > >[...] it seems to me that we don't really
> > >need to call js_CheckRedeclaration any more for JSOP_INITPROP.
> > 
> > Sounds good to me. Followup bug?
> 
> Filed as bug 5225158.

That should be bug 522158.

Comment 28

[Jason Orendorff [:jorendorff]]

Comment on attachment 412251 [details] [diff] [review]
Detect duplicate property names.

Great!

Comment 29

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/02f8b1993ae1

Comment 30

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/37c3734c11f8

Comment 31

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/02f8b1993ae1