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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

(Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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).
We already have a strict warning when code does this.
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

8 years ago
Created attachment 401939 [details] [diff] [review]
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.
Assignee: general → jim
Status: NEW → ASSIGNED
Attachment #401939 - Flags: review?(mrbkap)
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
(Assignee)

Comment 5

8 years ago
Created attachment 404915 [details] [diff] [review]
Define JSAutoAtomList, a variant of JSAutoAtomList with a destructor.
Attachment #404915 - Flags: review?
(Assignee)

Comment 6

8 years ago
Created attachment 404916 [details] [diff] [review]
Detect duplicate property names in object literals in ES5 strict mode.
Attachment #404916 - Flags: review?
(Assignee)

Updated

8 years ago
Depends on: 514585
(Assignee)

Updated

8 years ago
Attachment #404915 - Flags: review? → review?(jorendorff)
(Assignee)

Updated

8 years ago
Attachment #404916 - Flags: review? → review?(jorendorff)
Comment on attachment 401939 [details] [diff] [review]
Fix error processing in PrimaryExpr. [landed]

fwiw, NewBinary does null check its arguments, but this is clearer.
Attachment #401939 - Flags: review?(mrbkap) → review+
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
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #401939 - Attachment description: Fix error processing in PrimaryExpr. → Fix error processing in PrimaryExpr. [landed]

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/2ed35be3e01d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

8 years ago
That's only one patch landed out of three.

Should I have marked this bug up differently to make it more clear?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #404915 - Flags: review?(jorendorff) → review+
Attachment #404916 - Flags: review?(jorendorff) → review+
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.
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

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
(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.
(Assignee)

Comment 16

8 years ago
Created attachment 406174 [details] [diff] [review]
Detect duplicate property names.

Refresh, with the changes Jason suggested made.
Attachment #404916 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
(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.
"literal" is shorter than "initialiser" and it avoids the Flagrant UK spelling that Ecma and ISO insist upon. Two for two! ;-)

/be
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
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
(Assignee)

Comment 21

8 years ago
(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.
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Comment 23

8 years ago
Created attachment 406760 [details] [diff] [review]
Detect duplicate property names.

Now --- with tests!  (No code changes.)
Attachment #406174 - Attachment is obsolete: true
Attachment #406760 - Flags: review?(jorendorff)
(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 on attachment 406760 [details] [diff] [review]
Detect duplicate property names.

Jim, please post a patch with updated tests for review.
Attachment #406760 - Flags: review?(jorendorff)
(Assignee)

Comment 26

8 years ago
Created attachment 412251 [details] [diff] [review]
Detect duplicate property names.

Tests revised per jorendorff's and Waldo's comments.
Attachment #406760 - Attachment is obsolete: true
Attachment #412251 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #412251 - Flags: review? → review?(jorendorff)
(Assignee)

Comment 27

8 years ago
(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 on attachment 412251 [details] [diff] [review]
Detect duplicate property names.

Great!
Attachment #412251 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 29

8 years ago
http://hg.mozilla.org/tracemonkey/rev/02f8b1993ae1
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

8 years ago
Flags: in-testsuite+

Comment 31

8 years ago
http://hg.mozilla.org/mozilla-central/rev/02f8b1993ae1
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 541455
No longer depends on: 541455
You need to log in before you can comment on or make changes to this bug.