715821 - Don't have __define[GS]etter__ use CheckRedeclaration handle conflicts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

CheckRedeclaration is standing in the way of property-splitting work I'm doing.  Moreover, its behavior doesn't clearly correspond to anything in the spec, which makes it more than a bit confusing.  I plan to remove it, and to replace its uses with code that more clearly corresponds to spec algorithms.  (Or to nothing at all, for several calls which are near-vestigial now, after a couple other changes.)

The first step is to remove the CheckRedeclaration calls outside the parser.  There are only two -- in __defineGetter__ and __defineSetter__.  It seems clearest to do these in a separate patch from the parser bits.

***

The semantics of these, and passing { get: arguments[1], enumerable: true, configurable: true } as the descriptor object, are a little tricky, so I'll try to run through the oddities of our semantics, and how this patch mostly preserves them.

== No existing property ==

If there's no property at all, __d[GS]__ creates a new enumerable configurable property with only the half specified.  So the descriptor we pass in has to have those two bits set in it.  That's the easy part.

== Existing accessor property ==

If there's an existing accessor descriptor, __d[GS]__ updates the appropriate method half.  This patch obviously does that.  More interestingly, the current implementation passes enumerable/configurable as true/true.  Because O.dP merges specified attributes into the updated descriptor, explicitly specifying them here does the trick.

== Existing data property ==

Otherwise, there's an existing data descriptor.  In the old code, for CheckRedeclaration to permit the change, we must have ((oldAttrs | attrs) & JSPROP_READONLY) == 0.  That constant-folds to ((oldAttrs & JSPROP_READONLY) == 0.  So it doesn't allow changes to existing, non-writable properties, regardless of configurability.  But ES5 explicitly says that [[DefineOwnProperty]] is allowed to make such changes.  See the note at the end of ES5 8.12.9:

http://es5.github.com/#x8.12.9

This patch thus permits changes to non-writable but configurable properties.  I think these semantics are preferable given the spec statement that [[DefineOwnProperty]] allows such changes.  It's also worth noting that v8 implements __defineGetter__ and such with the overwrite-non-writable-but-configurable semantics:

http://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/v8natives.js?r=10149#280

That leaves the configurable bit in the existing property to be addressed.  We're not allowed to modify non-configurable properties, and O.dP will properly see that the existing property is non-configurable and will throw.

***

Those are the patch semantics.  Tests pass with the patch, with a few little changes to existing tests.

First, previously __defineGetter__ and such could add properties to ArrayBuffer instances, despite those instances appearing to be non-extensible.  That was wrong, and this patch fixes that.  I removed the test in question, since when such definitions are prohibited, there's nothing left of the test that's meaningful.

Second, I changed the errors __defineGetter__ throws to be TypeErrors.  I think this makes more sense semantically, and it makes us consistent with v8's algorithm done using an Object.defineProperty kernel.  This required one test be changed to expect a TypeError rather than a SyntaxError.

Third, because different code's generating some of the errors now, an error message changed.  I had to update one (extension-y) test for this.

***

Hopefully that should explain all you need to know to review this.  If you still lack any confidence, I suggest you compare this patch's implementation to the v8 one noted above -- they're pretty much carbon copies of each other, which hopefully will provide whatever confidence you might still lack in it.  :-)

Comment 1

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b102e74b3b8

It belatedly occurs to me that I should have written some tests for this.  Please keep this bug open until I do that, posthaste.

Comment 2

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51cc1b5c935a

Those are some tests, I think we're good now -- close this when this changeset gets merged to m-c.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/0b102e74b3b8

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/51cc1b5c935a