Closed Bug 365869 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: mrbkap)

References

Details

Attachments

(2 files, 2 obsolete files)

> 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.)
Attached patch One approach (diff -w) (obsolete) — Splinter Review
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #250430 - Flags: review?(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
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.
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
Because the if statement right after the assignment is basically:
if (report != JSREPORT_ERROR)
    report = JSREPORT_ERROR;
You are ignoring early returns....

/be
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?
(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
Attached patch Updated patch (diff -w) (obsolete) — Splinter Review
I simply removed report = JSREPORT_ERROR... I'm not sure if that preserves the original intent, though.
Attachment #250430 - Attachment is obsolete: true
Attachment #250851 - Flags: review?(brendan)
Attachment #250430 - Flags: review?(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
Attached patch With that (-w)Splinter Review
Attachment #250851 - Attachment is obsolete: true
Attachment #253064 - Flags: review?(brendan)
Attachment #250851 - Flags: review?(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
Attachment #253064 - Flags: review?(brendan) → review+
I just checked the latest patch in, but have a followup patch that I'll attach in a second.
Before this patch you'd get:
js> ({__proto__:null})
typein:1: strict warning: redeclaration of property __proto__

which isn't really useful, IMO.
Attachment #255861 - Flags: review?(brendan)
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.
Depends on: 371292
Comment on attachment 255861 [details] [diff] [review]
Allow for overriding properties

Brendan: ping?
Comment on attachment 255861 [details] [diff] [review]
Allow for overriding properties

mrbkap asked me to snatch this review.
Attachment #255861 - Flags: review?(brendan) → review+
Fixed for real now.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365869.js,v  <--  regress-365869.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed 1.9.0 20070320 win/mac*/linux
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.