365869 - Add a strict warning for when an object literal has two properties with the same name

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jesse Ruderman]

> uneval({a:4, a:5})
({a:5})

This should give a strict warning, because it usually indicates a bug in the code.

(While investigating bug 341872, Gavin noticed that HelperApps.prototype in helperApps.js has two "RemoveObserver" functions.  If that triggered a strict warning, it might have prevented a typo from becoming a bug in Firefox.)

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: One approach (diff -w)

This is one approach to the bug. Note that in the newly created 'else' case, report will always be JSREPORT_ERROR, I'm not sure what the proper control flow there should be, though.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 250430 [details] [diff] [review]
One approach (diff -w)

At least call it JSPROP_INITIALIZER or something like that (JSPROP_LITERAL?).  Otherwise this seems ok. I didn't understand what you meant about "newly created 'else' clause", though.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

From the patch:
     report = ((oldAttrs | attrs) & JSPROP_READONLY)
              ? JSREPORT_ERROR
              : JSREPORT_WARNING | JSREPORT_STRICT;

     if (report != JSREPORT_ERROR) {
[comment elided]
         if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)))
             return JS_TRUE;
         if ((~(oldAttrs ^ attrs) & (JSPROP_GETTER | JSPROP_SETTER)) == 0)
             return JS_TRUE;
         if (!(oldAttrs & JSPROP_PERMANENT))
             return JS_TRUE;
         report = JSREPORT_ERROR;
     }

This is the code that's there now; in the patch it's in an else clause. Note that report == JSREPORT_ERROR no matter what if we end up actually reporting the error. I'm not sure if this is desired, but, as written, it's slightly misleading.

I'll rename the pseudo-attribute JSPROP_INITIALIZER.

Comment 4

[Brendan Eich [:brendan]]

I'm confused -- why is report always JSREPORT_ERROR in the code you put in the new outer else clause?  It could be JSREPORT_WARNING | JSREPORT_STRICT if neither of oldAttrs and attrs contains JSPROP_READONLY.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Because the if statement right after the assignment is basically:
if (report != JSREPORT_ERROR)
    report = JSREPORT_ERROR;

Comment 6

[Brendan Eich [:brendan]]

You are ignoring early returns....

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Right - if any of those conditions are met, we won't report an error. Otherwise, we'll set report = JSREPORT_ERROR, so what's the point of the JSREPORT_WARNING | JSREPORT_STRICT?

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Right - if any of those conditions are met, we won't report an error.
> Otherwise, we'll set report = JSREPORT_ERROR, so what's the point of the
> JSREPORT_WARNING | JSREPORT_STRICT?

Not sure -- seemed like there was a need for it at one point.  You could eliminate the ?: that synthesizes it, yeah.  Hmm.

A little cvs annotate/log/diff work shows this code went in for bug 40760, and it was just obfuscated from the start.  Please fix!

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch (diff -w)

I simply removed report = JSREPORT_ERROR... I'm not sure if that preserves the original intent, though.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 250851 [details] [diff] [review]
Updated patch (diff -w)

How about just eliminating report and inlining the inverse of its initializer's ? condition -- !((oldAttrs | attrs) & JSPROP_READONLY) -- in the if condition?

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that (-w)

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 253064 [details] [diff] [review]
With that (-w)

Sorry, forgot about this. I think it's right from the -w; I'll read the plain diff on checkin.

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I just checked the latest patch in, but have a followup patch that I'll attach in a second.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Allow for overriding properties

Before this patch you'd get:
js> ({__proto__:null})
typein:1: strict warning: redeclaration of property __proto__

which isn't really useful, IMO.

Comment 15

[Alex Vincent [:WeirdAl]]

mrbkap: re comment 14, this is actually a not-very-nice regression for Foo.prototype.toString() definitions; we hit the same warning.

Is this testable via xpcshell?  I hear javascript.options.werror = true means strict warnings become catchable errors.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 255861 [details] [diff] [review]
Allow for overriding properties

Brendan: ping?

Comment 17

[Brian Crowder]

Comment on attachment 255861 [details] [diff] [review]
Allow for overriding properties

mrbkap asked me to snatch this review.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fixed for real now.

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365869.js,v  <--  regress-365869.js
initial revision: 1.1

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070320 win/mac*/linux