Introduce an implementation of the ECMAScript 4 Object.defineProperty method, the signature of which looks something like: Object.defineProperty(obj, name, value, Object.WRITABLE | Object.ITERABLE | Object.DELETABLE)

Is this a 1.9.1 blocker?

Who own's this, sayre?

Note: the current version of Object.defineProperty is: Object.defineProperty(obj, name, descriptor). I'll take a stab at it.

This maps (as far as I can tell) the 3.1 spec to our current implementation. Note the TODO (no bug filed (yet?)) under the 'flexible' property.

Comment on attachment 329092 [details] [diff] [review] patch v1 >+ if (hasFlexible && !js_ValueToBoolean(v)) { >+ /* >+ * TODO: This is only half of flexible: we need to seal the value if >+ * [[Flexible]] is true. >+ */ >+ attrs |= JSPROP_PERMANENT; >+ } I don't see how flexible being true imples sealing anything. What step(s) in 8.6.2.9 [[DefineOwnProperty]] are you referring to here? /be

From the description of flexible (copy/pasted from the table at 8.6.1): If true, attempts to delete the property or change its attributes will succeed. Otherwise, the property is said to be sealed. See the description of the delete operator in section 11.4.1, and the reflective Object methods . A data property which is both sealed and read-only is frozen.

See Waldemar's comments from the es3.x-discuss@mozilla.org list: 8.6.2.9: I find this to be currently unintelligible due to too many errors and unstated or unwarranted assumptions. Some of these include: - What is a generic descriptor? I see the definition of IsGenericDescriptor but it's not described what it would be used for. - How do we know that the value of the [[Flexible]] field of Result(1) isn't Unspecified? 8.6.1 doesn't state whether the attributes are optional or mandatory. - Step 3b is invalid. IsGenericDescriptor, IsDataDescriptor, and IsAccessorDescriptor (I assume that's what 3b is testing) can all be false simultaneously. - "Create an own accessor property named P of object O ...": There is no such thing as an accessor property. There are named data properties, named procedural properties, and internal properties (see the beginning of 8.6). None of the three maps to property descriptors, as none of the three can contain all of the fields that a property descriptor can have -- for example, a property descriptor could have both [[const]] and [[setter]] present, and there is no way to map such a thing to either a named data property or a named procedural property. - Step 5: What does "is the same value mean" here for the [[value]], [[getter]], or [[setter]] properties? There is no well-defined notion of value equality (even === doesn't work for NaN's), while function equality requires solving the halting problem. - Steps 5 and 6: I don't fully understand the intent here, but it appears that having a const definition of a name N followed by a var definition of name N is ok (the var definition would be silently ignored), while having a var definition of a name N followed by a const definition of name N would be an error. This is weird. - Step 8: Once again, what is a generic descriptor? How do we know that it already has the right kind (named data property vs. named procedural property) for step 12 to be possible? - Step 10.a.ii.1: Same problem with [[value]] equality as above. - Step 10.b.i.1: Why is it forbidden to redefine a flexible, writable value into a different non-writable value? Nothing prevents one from doing this in two steps (first change the thing to a getter and then change it to a different non-writable value), so semantically this restriction doesn't make sense. - Step 11.a.x: Same equality problem. The halting problem need not be solved -- Waldemar was concerned that a getter would be automatically synthesized in some cases. "Same value" however cannot be === (NaN and signed zero). The other points stand. /be

We need to get a better spec here before implementing. /be

Per discussion at the summit, we can't block on this, as the spec isn't up to snuff.

I normally use this partial workaround that follows IE8's implementation. I also think this bug should be changed to be about implementing both Object.defineProperty and Object.getOwnPropertyDescriptor.

Elijah: Agreed, this bug should include Object.defineProperties as well as defineProperty and getOwnPropertyDescriptor. Object.getOwnPropertyNames may belong in this bug too. At a glance, I think we want a separate bug on each of these groups: * Object.create; * Object.freeze, Object.isFrozen; * Object.seal, Object.isSealed; * Object.preventExtensions, Object.isExtensible. But perhaps the last three groups all make one big hunk of related work, with thin API veneer on top. Blake, Rob, Jim: can you guys agree and file blockers of this bug? And anything else I'm missing from a quick read and a memory-castle tour of ES5's Object additions? Thanks, /be

Woah, I somehow got the following in my workaround's initial comment (I must have accidentally pasted it). Also, I put in my generic license thingy. It was supposed to say public domain :P. >>+ /* >>+ * TODO: This is only half of flexible: we need to seal the value if >>+ * [[Flexible]] is true. >>+ */

(In reply to comment #11) > At a glance, I think we want a separate bug on each of these groups: > > * Object.create; > * Object.freeze, Object.isFrozen; > * Object.seal, Object.isSealed; > * Object.preventExtensions, Object.isExtensible. Blake says this looks okay to him; seems fine to me, too.

Subtasks are now filed as bugs on which this one depends: bug 492840: Object.create bug 492844: Object.freeze, Object.isFrozen bug 492845: Object.seal, Object.isSealed bug 492849: Object.preventExtensions, Object.isExtensible

Stealing, I'm going to need this for Object.create at a minimum, quite possibly for others as well, and it doesn't seem wise to introduce interperson dependencies here if we don't have to. I will beg forgiveness/repurpose permission to steal that I thought was necessary for testing a tracemonkey bug but which ended up not being needed a couple months ago. :-)

The algorithm has grown immensely in complexity since I last looked at it, probably before mrbkap wrote his patch here. The previous simple patch is entirely inadequate when property descriptors may omit fields as the latest spec (wrongly, in my opinion) requires. I don't trust any implementation of this unless it exactly mirrors the specification. Next step: write a mini-reference implementation and exhaustively test that all this native implementation and the reference produce the exact results in all cases. I don't trust this patch until I've done that; there are simply too many paths of control flow to test here to trust any amount of human judgment to catch all the issues.

Omitted properties are imputed the undefined value, which is falsy. Since these properties have the default "secure" or "high integrity" sense (i.e., writable defaults to false, enumerable to false, configurable to false), by design the defaulting does not create three states where only two possible boolean values would exist in a more explicit API. Not to say the test burden is low, but this doesn't seem to cause state explosion under the hood. Whether you have N APIs or 1, the desired atomicity promises mean there has to be a big rectifier down there than can parse and make sense of the linear combinations. At least with one API we don't have to foist open and commit mandatory transactional APIs on JS users! /be

How's this going, Jeff?

Been busy with other bugwork lately, haven't touched it in the last couple weeks, but getting back on it pretty soon.

(In reply to comment #17) > Omitted properties are imputed the undefined value, which is falsy. Since these > properties have the default "secure" or "high integrity" sense (i.e., writable > defaults to false, enumerable to false, configurable to false), by design the > defaulting does not create three states where only two possible boolean values > would exist in a more explicit API. This is wrong: var o = { foo: 17 }; // These two calls have different behavior, because the former returns // silently due to no fields being present, while the latter makes the // property readonly -- even though writable: false is the ostensibly- // imputed default. Object.defineProperty(o, "foo", {}); Object.defineProperty(o, "foo", { writable: false }); > Not to say the test burden is low, but this doesn't seem to cause state > explosion under the hood. Whether you have N APIs or 1, the desired atomicity > promises mean there has to be a big rectifier down there than can parse and > make sense of the linear combinations.> At least with one API we don't have to > foist open and commit mandatory transactional APIs on JS users! The proposal was never to split property definition completely, only to have a property definition that replaces the entire property and a property alteration method that only changes attributes. There's no reason each method couldn't be atomic; each would serve a different purpose from the other. Back to looking at this again, having cleared away a bunch of other bugs in the meantime...

(In reply to comment #20) > (In reply to comment #17) > > Omitted properties are imputed the undefined value, which is falsy. Since these > > properties have the default "secure" or "high integrity" sense (i.e., writable > > defaults to false, enumerable to false, configurable to false), by design the > > defaulting does not create three states where only two possible boolean values > > would exist in a more explicit API. > > This is wrong: > > var o = { foo: 17 }; > > // These two calls have different behavior, because the former returns > // silently due to no fields being present, while the latter makes the > // property readonly -- even though writable: false is the ostensibly- > // imputed default. > Object.defineProperty(o, "foo", {}); > Object.defineProperty(o, "foo", { writable: false }); Ok, but no three-states -- empty property descriptor special case. I think you should post this example up on es-discuss@mozilla.org as a followup to the thread you started. Stick up for yourself! :-P Srsly, I think you make a good point. Next week's TC39 meeting is gonna finalize the spec, so now is the time. > The proposal was never to split property definition completely, only to have a > property definition that replaces the entire property and a property alteration > method that only changes attributes. There's no reason each method couldn't be > atomic; each would serve a different purpose from the other. Right, defineProperty and changeProperty. I'm hip, not sure how this'll play out in TC39, but we need the es-discuss salvo to get the war started. /be

The iterate-over-the-entire-input-space part of the file isn't there yet, but it's enough of a start to see where this would need to go. I'm briefly jumping over to implement Object.getOwnPropertyDescriptor in bug 505587 since this proto-test harness needs it and it's moderately simple.

Hm, the more I think about it the more I think I should probably bite the bullet and implement this such that Object.defineProperties can also easily be implemented. (The issue? Atomicity.) Looks like a mini-transactional API for JSScope is in order, I think...

The plural method is much worse due to atomicity, and it's not especially more useful than the singular one -- punted it to bug 513387.

js> var s = new String() js> Object.getOwnPropertyDescriptor(s, "length").toSource() ({value:0, writable:false, enumerable:false, configurable:false}) js> Object.defineProperty(s, "length", { value: 17 }) js> Object.getOwnPropertyDescriptor(s, "length").toSource() ({value:0, writable:true, enumerable:false, configurable:true}) There's a bug here somehow, just not sure how yet...

I think I've fixed the bug in comment 27 in passing in an updated patch that's on my laptop now -- I fixed one dumb bug that's probably it but haven't run through the logic to check. I've also finished up the exhaustive-test harness and run it against the patch. Everything passed with an iteration that forgot to also include allowing individual property-descriptor fields to be omitted; I'll run the modified version that does allow each field to be omitted overnight (it probably does require at least that much time), and if everything is in shape this should be good to go shortly. I'll probably write a handful of manual tests in parallel with posting the patch, just to cover bases in case the exhaustive test script itself has bugs. (It's a very literal, very stupid translation of the spec, so I expect there aren't any.) One other thing: the final patch will depend on bug 515285, which will likely be wrapped up in the next few days.

...or not, of course the exhaustive test had bugs in it! Current stumbling is over get/set as undefined, with or without the other defined as a function. To be resolved tomorrow, I hope, sleep for now...

I think this might be a fully complete harness. Every time I turn around I find a bug in either it or the patch, tho, so I could be wrong -- there's an awful lot of state space to run through to say for sure, even if I flip the flag that theoretically should cut down on the work by a large factor (isValidDescriptor, but I'm not confident enough in the patch to do so).

This is very nearly good to go, I think. Supporting { get: undefined }, { set: undefined }, and { get: undefined, set: undefined } requires some complexifying changes to the interaction of the values for JSScopeProperty::[gs]etter and JSScopeProperty::attrs, but if those are okay (they may not be -- hammering on property gets/sets, watches, and other niggly things is probably needed) the rest is probably fine. This uncovered one latent bug, however, in the case of a transition from: { [gs]et: ..., ..., configurable: true, enumerable: true } => { [gs]et: ..., ..., enumerable: false } The patch's code does the right thing, but the underlying mechanisms are buggy in this case; I'll file the bug after I post an updated test script here.

If run with -j -f script -e 'main()' all tests pass except for ones which botch [[Enumerable]] as detailed in the to-be-filed bug (see TestRunner.prototype._reportAllErrors).

I forgot that I submitted a partial workaround for this bug a while ago. Don't use those workarounds as they don't correctly implement it (even partially). The latest version of the workaround ("Xccessors") can be found at http://github.com/eligrey/Xccessors in case anyone here wants to use Object.defineProperty in other browsers.

The latent bug of the previous patch is still unfixed in this (separate bug will handle it fine), but aside from that this has some good changes -- a greater variety of more precise error messages, some changes to remove what appears to be a UMR, removal of a crash or two as well, dropping the current property before defining the new one or throwing an error (avoid deadlocks if, perhaps, TypeError is being extended or modified on multiple threads or something like that -- details an exercise for the reader). However, it seems to have picked up a crash (possibly due to updates to the TM tree) when I run the test script that I haven't figured out yet. Scope property management seems to be wrong somewhere, and since the patch mostly doesn't touch that I'm not sure what's wrong -- more debugging and eyeballs needed.

So, technically [[DefineOwnProperty]] is a per-object mechanism; different behaviors are prescribed for Object's [[DefineOwnProperty]] and Array's [[DefineOwnProperty]] (String as well, if memory serves, but I seem to recall its differences may be more minimal). I've so far ignored the Array requirement and hoped that slow-ifying would be sufficient. Looking closer, array_defineProperty doesn't perform any of those extra checks, and strictly speaking it shouldn't, at least not if it's to follow the current model of the defineProperty hook (which doesn't perform any argument validation -- should it, perhaps?). Time for JSObjectOps.defineOwnProperty, I think? It'd make the implementation more similar to the spec, which can only be a good thing.

JSObjectOps.defineProperty *is* defining an "own" property. Don't bloat the names with gratuitous "Own" parts. Just change what needs to be changed. We already have some validation (or missing validation, i.e. the bug you noted) in get/set vs. data property transitions. /be

(In reply to comment #36) > JSObjectOps.defineProperty *is* defining an "own" property. Don't bloat the > names with gratuitous "Own" parts. Just change what needs to be changed. Truly, I don't care what the name is, but it's more than just a difference in argument validation. To correctly implement [[DefineOwnProperty]] I'd have to change defineProperty to take a property descriptor (basically a boxed-up form for getter/setter/attrs/value), and it seems extremely unlikely to me that in most circumstances that overhead would be acceptable as a general premise when in most instances all the sanity-checking inanity of [[DefineOwnProperty]] is unnecessary.

We talked on IRC about this and that (don't want boxing or object descriptor for internal API that has same expressiveness as the ES5 user-facing one -- structs and native types instead). Then I suggested separating defineProperty into two ops, createProperty and changeProperty. Hope that works out! /be

Object.defineProperties atomicity is dead, good riddance -- expect that in the next patch here (basically copy-paste from the previous version here that wasn't atomic).

I returned to this today and discovered that I can't remember how I thought JSObjectOps modifications as proposed here would be sufficient to address the [[DefineOwnProperty]] customization based on object class. The descriptor-field and sanity-checking logic are simply too intertwined for me to be confident of my ability to disentangle them, and at this late date I don't want to try -- so I haven't. DefineProperty just splits on object class/type now into helpers, and the Array one can call into the other in the cases where ES5 specifies such. This adds the test script from here as a shell test, along with a little bit of testing of Array defineProperty and a minimal bit of defineProperties. The monster defineProperties test fails with JIT enabled; I need to enlist help in figuring this out, because my examination shows it's a bug nestled away in trace-tree management code. :-\ Given that such a monster script is needed to trigger it, I'm hoping we can get this landed and fix that bug in a fast followup -- deadline for this is more important that full passing everywhere always, I think.

Comment on attachment 402501 [details] [diff] [review] Full patch, decided to leave JSObjectOps alone for now jorendorff has to split, alas, tagging for review by mrbkap (leaving old review just in case he wants to finish it off)... This plus Object.create are sufficient to bring us to parity with current WebKit trunk -- I really think we need them.

Waldo, please fix the tests to call reportCompare() and update ecma_5/Object/jstests.list. Thanks.

bc, Waldo and I like assertEq. It's quick. The semantics are precise. It distinguishes -0 from +0. On success it does nothing. The implementation (already in js/tests/shell.js) is quite short and easy to read. It's built into the JS shell, so you can write tests using it that can be run directly, without loading shell.js (this is useful sometimes in the debugger). And anyway, these days our test runner detects uncaught exceptions and correctly treats them as failures. I think reportCompare would be more lenient than we intend in some of the tests here. Can assertEq be declared an acceptable way of doing things in js/tests?

assertEq is fine with me. I don't like that it throws on a failure and prevents the rest of the tests from running and terminates the shell with a non-zero exit code, but can live with it if you guys can. I'm not asking to replace assertEq with reportCompare but to use reportCompare at least once to signify that the test has passed. reportCompare and its friends have side-effects which record the test results which are used by the reftest framework to report test success. Most of the tests using assertEq without a call to reportCompare include a final line like print("All tests passed!") All you need to do in these cases to support the jsreftests is include a call to reportCompare at the end of the script so that at least one result is recorded.

PS. the tests live in js/src/tests now on tracemonkey.

Thanks, bc! Still almost done with this review, alas. Here's what I have so far. In js.msg: >+MSG_DEF(JSMSG_OBJECT_NOT_EXTENSIBLE, 233, 0, JSEXN_TYPEERR, "properties cannot be defined upon object") "upon object" is kinda weird. How about "the object is not extensible"? >+MSG_DEF(JSMSG_PROP_NOT_CONFIGURABLE, 234, 0, JSEXN_TYPEERR, "non-configurable property cannot be replaced with a configurable property") >+MSG_DEF(JSMSG_CANT_CHANGE_PROP_ENUMERABILITY, 235, 0, JSEXN_TYPEERR, "can't change the enumerability of a non-configurable property") >+MSG_DEF(JSMSG_CANT_CHANGE_TO_DATA_DESCRIPTOR, 236, 0, JSEXN_TYPEERR, "a non-configurable property with a getter or setter can't be changed to store a value") >+MSG_DEF(JSMSG_CANT_CHANGE_TO_ACCESSOR_DESCRIPTOR, 237, 0, JSEXN_TYPEERR, "a non-configurable property with a value can't be changed to have a getter or setter") >+MSG_DEF(JSMSG_CANT_CHANGE_WRITABILITY,238, 0, JSEXN_TYPEERR, "can't change writability of a non-configurable property") >+MSG_DEF(JSMSG_CANT_CHANGE_VALUE, 239, 0, JSEXN_TYPEERR, "can't change value of a non-configurable, non-writable property") >+MSG_DEF(JSMSG_CANT_CHANGE_GET_SET_IF_NOT_CONFIGURABLE, 240, 0, JSEXN_TYPEERR, "can't change the getter or setter of a non-configurable property") These error messages seem to say "O hai, I see you're redefining a non-configurable property. Clearly what you intended to do was redefine the property to be exactly the same as it already was. So I'll helpfully point out exactly in what way your descriptor is different from what I've already got." Of course the common case will be that the user didn't intend to redefine anything at all. So it seems like we should just say something like, "can't redefine non-configurable property {0}". >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >--- a/js/src/jsobj.cpp >+++ b/js/src/jsobj.cpp >@@ -2014,19 +2014,19 @@ obj_getOwnPropertyDescriptor(JSContext * > jsval getter = JSVAL_VOID, setter = JSVAL_VOID; > if (OBJ_IS_NATIVE(obj)) { > JSScopeProperty *sprop = reinterpret_cast<JSScopeProperty *>(prop); > if (attrs & JSPROP_GETTER) >- getter = js_CastAsObjectJSVal(sprop->getter); >+ getter = sprop->getter ? sprop->getterValue() : JSVAL_VOID; > if (attrs & JSPROP_SETTER) >- setter = js_CastAsObjectJSVal(sprop->setter); >+ setter = sprop->setter ? sprop->setterValue() : JSVAL_VOID; > } This patch changes the semantics of JSScopeProperty by allowing JSPROP_GETTER or JSPROP_SETTER to be set while sprop->getter and ->setter are null. I am still sifting through the code for places where we depend on this invariant, which is why this patch doesn't have review yet. :( >@@ -2091,16 +2091,598 @@ >+static JSBool >+HasProperty(JSContext* cx, JSObject* obj, jsid id, jsval& v, JSBool& answerp) Pointer arguments for the out parameters, please, throughout this patch (so that you can tell they're out parameters at the call sites). >+ECMAPropertyDescriptor::ECMAPropertyDescriptor() >+ : id(INT_JSVAL_TO_JSID(JSVAL_ZERO)), >[...] >+ hasGet(JS_FALSE), >+ hasSet(JS_FALSE), >+ hasValue(JS_FALSE), >+ hasWritable(JS_FALSE), >+ hasEnumerable(JS_FALSE), >+ hasConfigurable(JS_FALSE) s/JS_FALSE/false/ >+static JSClass descriptorArray_class = { I think you can instead use JSTVU_TRACE for this temporary rooting, which would be RAII-ier: class TempDescriptorArrayRooter : private JSTempValueRooter { public: typedef js::Vector<ECMAPropertyDescriptor, 1> Descriptors; TempDescriptorArrayRooter(JSContext *cx) { JS_PUSH_TEMP_ROOT_TRACE(cx, trace, this); } ~TempDescriptorArrayRooter() { JS_POP_TEMP_ROOT(cx, this); } static void trace(JSTracer *trc, JSTempValueRooter *tvr) { const Descriptors &descs = static_cast<TempDescriptorArrayRooter *>(tvr)->descs; ... } Descriptors descs; }; DefinePropertyObject and DefinePropertyArray are not the greatest names; I read them and think "define property-object" and "define property-array" though of course there are no such things. `DefinePropertyOnObject` would be OK. If you can do better, go for it! >+static JSBool >+DefinePropertyObject(JSContext *cx, JSObject *obj, ECMAPropertyDescriptor &desc, bool throwError, >+ bool &rval) If this can be a const reference, please make it so. >+ /* 8.12.9 step 1. */ >+ JSProperty *current; >+ JSObject *obj2; >+ if (!obj->lookupProperty(cx, desc.id, &obj2, &current)) >+ return JS_FALSE; >+ if (obj != obj2 && current) { >+ obj2->dropProperty(cx, current); >+ current = NULL; >+ } This change reveals that a SHARED PERMANENT property inherited by obj is not really an own property of obj. I think to be correct you would have to clone the property to obj and proceed from here. >+ /* 8.12.9 steps 5-6 (note 5 is merely a special case of 6). */ >+ jsval v = JSVAL_VOID; >+ JSScopeProperty *sprop = reinterpret_cast<JSScopeProperty *>(current); >+ do { >+ if (desc.isAccessorDescriptor()) { >+ if (!sprop->isAccessorDescriptor()) >+ break; >+ >+ if (desc.hasGet && >+ !js_SameValue(desc.getterValue(), >+ sprop->getter && (sprop->attrs & JSPROP_GETTER) >+ ? sprop->getterValue() >+ : JSVAL_VOID, cx)) { >+ break; >+ } >+ >+ if (desc.hasSet && >+ !js_SameValue(desc.setterValue(), >+ sprop->setter && (sprop->attrs & JSPROP_SETTER) >+ ? sprop->setterValue() >+ : JSVAL_VOID, cx)) { >+ break; I think the spirit of ES5 Sec. 8.12.9 step 6 is to skip the rest of the algorithm only if no changes are being requested. So in the code above, sprop->isAccessorDescriptor() must return true only if the existing property is slotless. See my comment about JSScopeProperty::is{Accessor,Data}Descriptor below. Places like this where we check a bit to determine whether we can safely call some other method always seem a little fragile to me; if you wanted to add JSScopeProperty::ecma[GS]etterValue() methods, I wouldn't object. >+ if (desc.isDataDescriptor()) { >+ if (!sprop->isDataDescriptor()) >+ break; Same sort of thing here-- sprop->isDataDescriptor() must return true only if we have a valid slot and the stub getter and setter. >+ } else { >+ /* 8.12.9 step 11. */ >+ JS_ASSERT(desc.isAccessorDescriptor() && sprop->isAccessorDescriptor()); If you take the JSScopeProperty::is{Data,Accessor}Descriptor changes I propose below, then this assertion can fail. There's another case. That is, ES5 Sec. 8.12.9 steps 9-11 don't cover the case where current is neither an ES5 DataDescriptor nor an ES5 AccessorDescriptor. I think in that case, if the existing property is not configurable, we should probably always Reject. >+ /* 8.12.9 step 12. */ >+ uintN attrs; >+ JSPropertyOp getter, setter; >+ if (desc.isGenericDescriptor()) { >+ uintN unchanged = 0; >+ if (desc.hasConfigurable) >+ unchanged |= JSPROP_PERMANENT; >+ if (desc.hasEnumerable) >+ unchanged |= JSPROP_ENUMERATE; I think these should be !desc.hasConfigurable and !desc.hasEnumerable. It's weird that your tests didn't catch this; it sounds observable. >+ rval = true; >+ obj->dropProperty(cx, current); >+ return obj->defineProperty(cx, desc.id, v, getter, setter, attrs); >+} It sure would be nice if we could figure out what end-to-end integrity guarantees we're trying to make here, and assert them at the end before redefining. >+ * Our optimization of storage of the length property of arrays makes >+ * it very difficult to properly implement defining the property. Is this right? If I understand correctly, Array length properties are non-configurable, so the only thing you could do to one is change its value or make it non-writable. Er, maybe that's what you meant. :) But in that case it should say "...to properly implement redefining length to be non-writable". The other cases are, I think, not that difficult. I could be wrong--the spec isn't pretty. (Just to be completely clear, it's fine with me to leave this unimplemented for now.) In obj_defineProperty: >+ /* 15.2.3.6 step 2. */ >+ JSAutoTempIdRooter nameidr(cx); >+ if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) >+ return JS_FALSE; Should this be argc >= 1? In struct ECMAPropertyDescriptor: >+ /* Bits indicating which values are set. */ >+ uint8_t hasGet : 1; >+ uint8_t hasSet : 1; >+ uint8_t hasValue : 1; >+ uint8_t hasWritable : 1; >+ uint8_t hasEnumerable : 1; >+ uint8_t hasConfigurable : 1; If bool bitsets work on the platforms we care about, let's do that instead. AFAIK they do. In jsprvtd.h: >+static inline JSPropertyOp >+js_CastAsPropertyOp(JSObject *object) >+{ >+ return JS_DATA_TO_FUNC_PTR(JSPropertyOp, object); >+} >+ > } /* export "C++" */ > #endif /* __cplusplus */ Since this is in __cplusplus only, please omit the 'static' before 'inline'. (It didn't have it in the original.) In jsscope.h, in struct JSScopeProperty: >+ bool configurable() { return (attrs & JSPROP_PERMANENT) == 0; } >+ bool enumerable() { return (attrs & JSPROP_ENUMERATE) != 0; } >+ bool writable() { return (attrs & JSPROP_READONLY) == 0; } Very nice. However: >+ bool isDataDescriptor() { >+ return (attrs & (JSPROP_SETTER | JSPROP_GETTER)) == 0; >+ } >+ bool isAccessorDescriptor() { >+ return (attrs & (JSPROP_SETTER | JSPROP_GETTER)) != 0; >+ } It's hard to say what these should do when the property is beyond the scope of ES5 property descriptors--e.g. when the property has a native getter or setter, or both a non-stub getter and a slot, or the stub getter and no slot. I think they should return false. So my best guesses would be: isDataDescriptor(): !getter && !setter && !(attrs & (JSPROP_SHARED | JSPROP_SETTER | JSPROP_GETTER)) isAccessorDescriptor(): (attrs & (JSPROP_SHARED | JSPROP_SETTER | JSPROP_GETTER)) == (JSPROP_SHARED | JSPROP_SETTER | JSPROP_GETTER) To emphasize the point, you might want to rename these isECMA{Data,Accessor}Descriptor. In the tests: >+/* >+ * Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/licenses/publicdomain/ >+ */ Any objection to putting this on two lines? // Any copyright is dedicated to the Public Domain. // http://creativecommons.org/licenses/publicdomain/ >+function assertEq(v1, v2, m) >+{ >+ if (!SameValue(v1, v2)) >+ { >+ throw "assertion failed: " + >+ "got " + uneval(v1) + ", expected " + uneval(v2) + >+ (m ? ": " + m : ""); >+ } >+} assertEq is already in the root shell.js. >+function isValidDescriptor(propdesc) >+{ >+ if (false) >+ { >+ return true; Huh? >+ // We permit null here simply because this test's author believes the >+ // implementation may sometime be susceptible to making mistakes in this >+ // regard and would prefer to be cautious. >+ if (propdesc.get !== null && propdesc.get !== undefined && !IsCallable(propdesc.get)) >+ return false; >+ if (propdesc.set !== null && propdesc.set !== undefined && !IsCallable(propdesc.set)) >+ return false; I don't understand. Why is it more cautious to permit null than to ban it? >+function DescriptorState() >+{ >+ /* >+ * Array of states from { -1 (omit) } U { [0,valueComponent.length) }, or >+ * null when all states have been exhausted, indicating the properties to >+ * place on the next generated property descriptor. >+ */ >+ this._state = >+ DescriptorState.STATES.map(function map(v) { return v.length - 1; }); >+} >+DescriptorState.STATES = >+ [ >+ /* value */ >+ [-Infinity, JSVAL_INT_MIN, -0, +0, 1.5, JSVAL_INT_MAX, Infinity, >+ NaN, "foo", "bar", null, undefined, true, false, {}, /a/], >+ /* get */ >+ [undefined, function get1() { return 1; }, function get2() { return 2; }, >+ null, 5], >+ /* set */ >+ [undefined, function set1() { return 1; }, function set2() { return 2; }, >+ null, 5], >+ /* enumerable */ >+ [true, false], >+ /* configurable */ >+ [true, false], >+ /* writable */ >+ [true, false], >+ ]; >+DescriptorState.prototype = >+ { >+ _generateDescriptor: function() >+ { >+ var state = this._state; >+ var STATES = DescriptorState.STATES; >+ var desc = {}; >+ if (state[0] !== -1) >+ desc.value = STATES[0][state[0]]; >+ if (state[1] !== -1) >+ desc.get = STATES[1][state[1]]; >+ if (state[2] !== -1) >+ desc.set = STATES[2][state[2]]; >+ if (state[3] !== -1) >+ desc.enumerable = STATES[3][state[3]]; >+ if (state[4] !== -1) >+ desc.configurable = STATES[4][state[4]]; >+ if (state[5] !== -1) >+ desc.writable = STATES[5][state[5]]; >+ >+ return desc; >+ }, >+ nextDescriptor: function nextDescriptor() >+ { >+ var state = this._state; >+ if (state === null) >+ return null; >+ >+ var STATES = DescriptorState.STATES; >+ for (var i = 0, sz = STATES.length; i < sz; i++) >+ { >+ while (state[i] >= -1) >+ { >+ var desc = this._generateDescriptor(); >+ >+ if (--state[i] < -1) >+ { >+ // Underflow; carry over the decrement to the higher state-values, >+ // if possible. >+ for (var j = i + 1; j < sz; j++) >+ { >+ if (state[j] > -1) >+ { >+ for (var k = i; k < j; k++) >+ state[k] = STATES[k].length - 1; >+ state[j]--; >+ break; >+ } >+ } >+ if (j === sz) >+ { >+ // If the loop terminated, we ran out of carry positions, so every >+ // entry but the first must be -1. We signal completion by >+ // nulling out this._state. >+ this._state = null; >+ } >+ } >+ >+ if (isValidDescriptor(desc)) >+ return desc; >+ } >+ } >+ >+ assertEq(true, false, "unreachable"); >+ } >+ }; I didn't review this for correctness. I think you could save some head-scratching here by using active voice: function mapTestDescriptors(f) { var OMIT = {}; var desc; function put(p, v) { if (v !== OMIT) desc[p] = v; } var values = [OMIT, -Infinity, JSVAL_INT_MIN, -0, +0, 1.5, JSVAL_INT_MAX, Infinity]; var getters = [OMIT, undefined, function get1() { return 1; }, function get2() { return 2; }, null, 5]; var setters = [OMIT, undefined, function set1() { return 1; }, function set2() { return 2; }, null, 5]; var bools = [OMIT, true, false]; for each (var value in values) for each (var get in getters) for each (var set in setters) for each (var enumerable in bools) for each (var configurable in bools) for each (var writable in bools) { desc = {}; put("value", value); put("get", get); put("set", set); put("enumerable", enumerable); put("configurable", configurable); put("writable", writable); f(desc); } }

(In reply to comment #48) > These error messages seem to say "O hai, I see you're redefining a > non-configurable property. Clearly what you intended to do was redefine the > property to be exactly the same as it already was. So I'll helpfully point out > exactly in what way your descriptor is different from what I've already got." > > Of course the common case will be that the user didn't intend to redefine > anything at all. So it seems like we should just say something like, > "can't redefine non-configurable property {0}". Good call, will do. > >@@ -2091,16 +2091,598 @@ > >+static JSBool > >+HasProperty(JSContext* cx, JSObject* obj, jsid id, jsval& v, JSBool& answerp) > > Pointer arguments for the out parameters, please, throughout this patch (so > that you can tell they're out parameters at the call sites). I had thought we were using references now rather than pointers when the value in particular could not validly be null; what's the actual policy? > I think you can instead use JSTVU_TRACE for this temporary rooting, which > would be RAII-ier: I didn't know about this functionality, thanks for pointing it out. > That is, ES5 Sec. 8.12.9 steps 9-11 don't cover the case where current is > neither an ES5 DataDescriptor nor an ES5 AccessorDescriptor. I think in that > case, if the existing property is not configurable, we should probably always > Reject. I'm not entirely sure what I think of this yet, but adding an entirely unique type of descriptor, one only we use, seems incredibly dangerous. Maybe if we make the distinction very clear this might work, but my spider sense is buzzing faster than a Core 2 Duo right now. > >+ /* 8.12.9 step 12. */ > >+ uintN attrs; > >+ JSPropertyOp getter, setter; > >+ if (desc.isGenericDescriptor()) { > >+ uintN unchanged = 0; > >+ if (desc.hasConfigurable) > >+ unchanged |= JSPROP_PERMANENT; > >+ if (desc.hasEnumerable) > >+ unchanged |= JSPROP_ENUMERATE; > > I think these should be !desc.hasConfigurable and !desc.hasEnumerable. It's > weird that your tests didn't catch this; it sounds observable. The sense of this is reversed from the other two branches here, as is the (sprop->attrs & ...) | (desc.attrs & ...) line; I'll change this to be consistent with the other two (or the other way around, just skimming comments before starting work on coding the responses). > >+ rval = true; > >+ obj->dropProperty(cx, current); > >+ return obj->defineProperty(cx, desc.id, v, getter, setter, attrs); > >+} > > It sure would be nice if we could figure out what end-to-end integrity > guarantees we're trying to make here, and assert them at the end before > redefining. If it were possible I think this would merely duplicate the above logic. :-\ > >+ * Our optimization of storage of the length property of arrays makes > >+ * it very difficult to properly implement defining the property. > > Is this right? If I understand correctly, Array length properties are > non-configurable, so the only thing you could do to one is change its value or > make it non-writable. > > Er, maybe that's what you meant. :) But in that case it should say "...to > properly implement redefining length to be non-writable". The other cases are, > I think, not that difficult. I could be wrong--the spec isn't pretty. Blame ECMA. If you read Array.[[DefineOwnProperty]] you'll see that [[Configurable]] for the length property of an array is entirely different from that of any other property in the language; ![[Configurable]] means it can't be deleted, but it does *not* mean writability or enumerability can't be changed from initial values (maybe not even from modified values, memory hazy). Had I noticed this earlier I would have complained to the working group about it, for obvious reasons, but I didn't notice. So it goes. > In obj_defineProperty: > >+ /* 15.2.3.6 step 2. */ > >+ JSAutoTempIdRooter nameidr(cx); > >+ if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) > >+ return JS_FALSE; > > Should this be argc >= 1? Why so? Object.defineProperty(O, Name, Property) has the name, if present, as the second argument. (If I'm the one missing something obvious, don't worry about it, I'm just making comments from memory before responding in code.) > >+function assertEq(v1, v2, m) > >+{ > >+ if (!SameValue(v1, v2)) > >+ { > >+ throw "assertion failed: " + > >+ "got " + uneval(v1) + ", expected " + uneval(v2) + > >+ (m ? ": " + m : ""); > >+ } > >+} > > assertEq is already in the root shell.js. Other engines to which I've been feeding this file manually don't have assertEq, and conditionally defining it seems to throw some of them off a bit. > >+function isValidDescriptor(propdesc) > >+{ > >+ if (false) > >+ { > >+ return true; > > Huh? That was the super-hyper-mega-exhaustive test condition, where the product of all the possible different field values would be tested. Cutting out the trivially invalid combinations where value/accessor semantics conflicted, by setting the condition to false as here, cut down on testing times immensely, but I left the longcut in place just in case the exhaustively exhaustive test is ever useful. > >+ // We permit null here simply because this test's author believes the > >+ // implementation may sometime be susceptible to making mistakes in this > >+ // regard and would prefer to be cautious. > >+ if (propdesc.get !== null && propdesc.get !== undefined && !IsCallable(propdesc.get)) > >+ return false; > >+ if (propdesc.set !== null && propdesc.set !== undefined && !IsCallable(propdesc.set)) > >+ return false; > > I don't understand. Why is it more cautious to permit null than to ban it? "permit" in the sense of test descriptors that include a null getter or setter, since stub getter/setter means that field is NULL and therefore might conceivably compare equal to null given subtly wrong comparison semantics. > I didn't review this for correctness. I think you could save some > head-scratching here by using active voice: Hm, probably right. Will at least think about it, most likely will make the changes roughly as suggested (although no for each, want this to be usable across engines).

(In reply to comment #49) > > That is, ES5 Sec. 8.12.9 steps 9-11 don't cover the case where current is > > neither an ES5 DataDescriptor nor an ES5 AccessorDescriptor. I think in that > > case, if the existing property is not configurable, we should probably always > > Reject. > > I'm not entirely sure what I think of this yet, but adding an entirely unique > type of descriptor, one only we use, seems incredibly dangerous. Maybe if we > make the distinction very clear this might work, but my spider sense is buzzing > faster than a Core 2 Duo right now. It's not really a question of adding another type of descriptor. The JSAPI already allows you to define properties that aren't ES5 data properties or ES5 accessor properties. This is already in SpiderMonkey. It would require breaking API changes to remove it. We could ignore this fact, and claim we don't care what happens when you use Object.defineProperty around existing non-ES5 properties. I don't think that's realistic. Object.defineProperty is going to be exposed to content scripts. We have to think a little bit about it and make sure scripts can't use it to break what few integrity invariants the language gives us (and which something might be counting on). For example, this treats quickstubbed properties as data properties and all other XPConnect properties as accessor properties. (Come to think of it Object.getOwnPropertyDescriptor already has this problem, sigh. Filed bug 520882.) The patch looks like it would allow JS programs to redefine PERMANENT properties that have JSPropertyOp getters and setters. I think that's a mistake. We should reject. (A JSAPI test for that would be nice.) While looking at this again, I noticed a few more things: >+ if (sprop->isDataDescriptor() && >+ !js_NativeGet(cx, obj, obj, sprop, JSGET_NO_METHOD_BARRIER, &v)) >+ { >+ obj->dropProperty(cx, current); If js_NativeGet fails, the property is already unlocked, so this dropProperty call is unnecessary. Also, throughout this function there's a mix of JSObject "virtual" method calls and direct calls to functions that only work on native objects. I don't think we have code like this anywhere else. It's a reasonable way to write, but it runs against house style. (I get the impression Brendan likes code that's not shy about what it "knows" and uses the fast paths available to it.) (Another type of property we have, as an implementation detail, is SPROP_IS_METHOD. The latest version of your patch calls js_NativeGet, or something, so it's safe; if you change that you'll need to make sure the read barrier is hit.) >+ } >+ >+ >+ /* 8.12.9 steps 5-6 (note 5 is merely a special case of 6). */ Two blank lines together. > > >+ * Our optimization of storage of the length property of arrays makes > > >+ * it very difficult to properly implement defining the property. > > > > Is this right? If I understand correctly, Array length properties are > > non-configurable, so the only thing you could do to one is change its value or > > make it non-writable. > > > > Er, maybe that's what you meant. :) But in that case it should say "...to > > properly implement redefining length to be non-writable". The other cases are, > > I think, not that difficult. I could be wrong--the spec isn't pretty. > > Blame ECMA. If you read Array.[[DefineOwnProperty]] you'll see that > [[Configurable]] for the length property of an array is entirely different from > that of any other property in the language; ![[Configurable]] means it can't be > deleted, but it does *not* mean writability or enumerability can't be changed > from initial values (maybe not even from modified values, memory hazy). Had I > noticed this earlier I would have complained to the working group about it, for > obvious reasons, but I didn't notice. So it goes. My understanding is that !configurable means that * configurable and enumerable can't be changed * for an accessor property, getter and setter can't be changed * for a data property, value can only be changed if writable, and writable can only be changed from true to false. and that this is the same for the length property of arrays as for all other properties. I think this in part because the special [[DefineOwnProperty]] method for arrays just calls the default [[DefineOwnProperty]] method to apply any actual changes to the .length property. > > In obj_defineProperty: > > >+ /* 15.2.3.6 step 2. */ > > >+ JSAutoTempIdRooter nameidr(cx); > > >+ if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) > > >+ return JS_FALSE; > > > > Should this be argc >= 1? > > Why so? Uh, quite. Never mind!

Comment on attachment 402996 [details] [diff] [review] Updated to latest TM I think jorendorff is already on this.

(In reply to comment #50) > The patch looks like it would allow JS programs to redefine PERMANENT > properties that have JSPropertyOp getters and setters. I think that's a > mistake. We should reject. (A JSAPI test for that would be nice.) Don't need a JSAPI test, Object.defineProperty(parseInt, "length", {value:9}). > While looking at this again, I noticed a few more things: > > >+ if (sprop->isDataDescriptor() && > >+ !js_NativeGet(cx, obj, obj, sprop, JSGET_NO_METHOD_BARRIER, &v)) > >+ { > >+ obj->dropProperty(cx, current); > > If js_NativeGet fails, the property is already unlocked, so this dropProperty > call is unnecessary. > > Also, throughout this function there's a mix of JSObject "virtual" method > calls and direct calls to functions that only work on native objects. I > don't think we have code like this anywhere else. It's a reasonable way to > write, but it runs against house style. (I get the impression Brendan likes > code that's not shy about what it "knows" and uses the fast paths available > to it.) For this algorithm I care way way way more about readability than I do about being over-clever. The direct calls are only being used where I have to work around side effects the traditional API would impose. > My understanding is that !configurable means that > * configurable and enumerable can't be changed > * for an accessor property, getter and setter can't be changed > * for a data property, value can only be changed if writable, and > writable can only be changed from true to false. > and that this is the same for the length property of arrays as for all other > properties. > > I think this in part because the special [[DefineOwnProperty]] method for > arrays just calls the default [[DefineOwnProperty]] method to apply any actual > changes to the .length property. Oh gag. Somehow I've been misreading that the entire time I've worked on this, with respect to writability changes of non-configurable properties, array and otherwise. I hate [[DefineOwnProperty]].

Nit, but it matters: the js_ValueToECMA{Ui,I}nt32 precedent was not good, so please avoid ECMAFooBar typenames and ECMA in method names. We are not at war with the spec. That erstwhile acronym (Ecma now, I kid you not) is ugly to read and harsh to say and hear. We should use the simplest names we can, esp. with C++ namespaces insulating us from other code. /be

(In reply to comment #52) > > Also, throughout this function there's a mix of JSObject "virtual" method > > calls and direct calls to functions that only work on native objects. I > > don't think we have code like this anywhere else. It's a reasonable way to > > write, but it runs against house style. (I get the impression Brendan likes > > code that's not shy about what it "knows" and uses the fast paths available > > to it.) > > For this algorithm I care way way way more about readability than I do about > being over-clever. The direct calls are only being used where I have to work > around side effects the traditional API would impose. Jorendorff's point is not about cleverness, rather avoiding calling out through a poor-man's vtbl to code that may not play with the hardcoded, native-only paths intermixed. It's a good point if there's any chance of the vtbl not being js_ObjectOps. Even if there's no chance, the appearance of chance (and the runtime overhead) are problematic on their own, to lesser degrees. /be

Hm, I'm starting to think you're right that native-getter-setters descriptors have to be different from accessor and data descriptors (or, more likely, subsumed in accessor descriptors -- incidentally, the reason for the current behavior of Object.getOwnPropertyDescriptor is that it duplicates the behavior of __lookup[GS]etter__). However: If we do that, shared-permanent properties with native getter-setter, where we use such to avoid an extra property, can't be used to implement ECMA-specified behavior -- not when there's no way to distinguish native-getter-setter-as-optimization from native-getter-setter-as-dynamic-behavior. For example, the length property of functions is required to be exposed as a data descriptor. This patch -- as currently written -- would do that. I guess it lies for the descriptors that have native getters. We could further differentiate native getters/setters that don't have side effects by grabbing one of the three bits left in JSScopeProperty::flags. Is there another option? Another day, another special case to handle, more code and complexity...

This patch properly handles redefinition of the length property on functions (and adds a handful of tests for that), but it does so only because it invokes fun_getProperty underneath js_NativeGet.

JSPropertyOp getters and setters were "first". They can indeed imitate data properties in the ES5 sense, and do for shared/permanent proto-props, so they are "just" an optimization we need to reflect as a data property. Other browsers make DOM attributes (OMG IDL term) reflect as function getter/setter pairs (getters if readonly). This is a point of non-interoperation so far (__lookupGetter__ can tell) but not a big deal. Perhaps it is a bigger deal with ES5. /be

I tweaked the test roughly as suggested, made native getter/setter reflect as data properties but forbid overwriting them if !configurable, removed ECMA from names, and I think that's it at this point. More comments appreciated, want to get this in Real Soon Now...

In js_HasOwnProperty: >- if (prop) >- obj2->dropProperty(cx, prop); >+ if (*propp) >+ (*objp)->dropProperty(cx, *propp); Don't you mean to just delete those two lines? The caller drops the property, right? >+ /* Bits indicating which values are set. */ >+ uint8_t hasGet : 1; >+ uint8_t hasSet : 1; >+ uint8_t hasValue : 1; >+ uint8_t hasWritable : 1; >+ uint8_t hasEnumerable : 1; >+ uint8_t hasConfigurable : 1; Why can't these be `bool`? We have bool bitfields in JSScript at least. More to come...

Comment on attachment 417016 [details] [diff] [review] Updated a bit Besides bool for bitfields, uint8 attrs; should pack nicely (unless you are storing more bits there than fit in JSScopeProperty::attrs, of course). /be

>+MSG_DEF(JSMSG_OBJECT_NOT_EXTENSIBLE, 239, 0, JSEXN_TYPEERR, "properties can't be defined on an object that's not extensible") Error message nit: I think I'd prefer just "object is not extensible", since the longer message is a bit misleading (you can successfully call Object.defineProperty on a non-extensible object; you just can't *add* properties). >+MSG_DEF(JSMSG_CANT_APPEND_PROPERTIES_TO_UNWRITABLE_LENGTH_ARRAY, 241, 0, JSEXN_TYPEERR, "can't add numeric properties greater than or equal to array's length if length is unwritable") "Can't add elements to an array if its length property is unwritable". Not that it matters since we aren't using this message anywhere yet. In JSPropertyDescriptor::initialize: >+ if (js_ValueToBoolean(v)) >+ attrs |= JSPROP_ENUMERATE; >+ else >+ attrs &= ~JSPROP_ENUMERATE; I would prefer to see plain if (js_ValueToBoolean(v)) attrs |= JSPROP_ENUMERATE; here and in the other two places where this idiom is used, because it's shorter and because that emphasizes whichever behavior is non-default. + /* 8.10.5 step 5 */ + if (!HasProperty(cx, desc, ATOM_TO_JSID(cx->runtime->atomState.valueAtom), &v, &hasProperty)) + return false; + if (hasProperty) { + hasValue = JS_TRUE; + value = v; + } this->value is not a root. Is there a GC hazard here where desc's .value property has a getter that creates the value from scratch, and it is then collected before `this` is ever rooted? If so, then probably the same is true of this->get and this->set. >+static JSBool >+Reject(JSContext *cx, uintN errorNumber, bool throwError, bool *rval) >+{ >+ [...] >+} >+ >+static JSBool >+Reject(JSContext *cx, JSObject *obj, JSProperty *prop, uintN errorNumber, bool throwError, >+ jsid id, bool *rval) >+{ >+ obj->dropProperty(cx, prop); >+ return Reject(cx, errorNumber, throwError, id, rval); >+} RAII instead of the extra overload here? In DefinePropertyArray: >+ * We probably should optimize dense array property definitions where s/probably should/could/. The rest of the comment says why we don't. Still more to come, I'm afraid. Ran out of time for today. :-(

(In reply to comment #61) > Don't you mean to just delete those two lines? The caller drops the property, > right? Yikes, you're right. > Why can't these be `bool`? We have bool bitfields in JSScript at least. Bitfielding a bool seemed...sketchy. Looked it up, bool's an integral type, it's perfectly acceptable to bitfield it, so changed. (In reply to comment #62) > (From update of attachment 417016 [details] [diff] [review]) > Besides bool for bitfields, uint8 attrs; should pack nicely (unless you are > storing more bits there than fit in JSScopeProperty::attrs, of course). Hm, hadn't thought of that -- can change.

The changes to JSScopeProperty::trace suggest that the meaning of JSPROP_GETTER/SETTER has changed (`hasGetterObject() && getterObject()`). Is that the case? What exactly is the change and why don't we worry about it in other places where hasGetterObject() or the equivalent is used? In jsobj.cpp: >+HasProperty(JSContext* cx, JSObject* obj, jsid id, jsval* vp, JSBool* answerp) >+{ >+ if (!JS_HasPropertyById(cx, obj, id, answerp)) >+ return JS_FALSE; >+ if (!answerp) { >+ *vp = JSVAL_VOID; >+ return JS_TRUE; >+ } That should be `if (!*answerp)`, right? In DefinePropertyObject: >+ /* 8.12.9 step 12. */ >+ uintN unchanged = 0; >+ if (!desc.hasConfigurable) >+ unchanged |= JSPROP_PERMANENT; >+ if (!desc.hasEnumerable) >+ unchanged |= JSPROP_ENUMERATE; >+ >+ attrs = (desc.attrs & ~unchanged) | (sprop->attrs & unchanged); This changes the JSPROP_GETTER and SETTER attrs to whatever desc says, without actually removing the getter or setter if one is already present: js> obj = {set x(a) print("set")}; ({set x (a) print("set")}) js> Object.defineProperty(obj, "x", {get: function () "get"}); ({get x () "get", set x (a) print("set")}) // setter is shown... js> obj.x = 13; // setter should be called, I think, but isn't 13 The new tests in js/src/tests/ecma_5/Object need to be added to the jstests.list there. Incidentally why is that called "15.2.3.6-02.js"? There's no "15.2.3.6-01.js". This doesn't have enough tests. We should have tests for at least the basics, and more of the corner cases too: - defining accessor properties - re-defining an accessor property (using defineProperty to add a getter to a property that already has a setter, and vice versa. - redefining an accessor property as a data property - redefining a data property to be non-writeable, with and without changing the value at the same time - trying to define a new property on a sealed object (if `seal` is present) - that properties of sealed objects are treated as non-configurable (currently they aren't reflected that way, which seems like a bug) - trying to redefine a function's .length - trying to redefine a non-configurable property to be configurable - trying to change other details of a non-configurable property - redefining a configurable property without changing anything about it - defining a generic property - using getOwnPropertyDescriptor to examine properties defined by defineProperty (a fine place for your patented Cartesian Product Testing technique) - trying to define an array element (not sure what's supposed to happen but whatever it is, it needs a test) - trying to use a non-function as a getter or setter - trying to define properties using invalid descriptors, like Object.defineProperty(obj, "x", {get: f, writeable: true}). ... If this stuff is already in the Microsoft test suite or something, great--how can I run that? Lastly, a test to ponder: function C() {} Object.defineProperty(C.prototype, "x", {get: function () { return 0; }}); assertEq(new C().hasOwnProperty("x"), false); // fails assertEq(Object.getOwnPropertyDescriptor(new C), undefined); // succeeds The SHARED PERMANENT optimization needs to be rethought, sigh. Is there already a bug for this? r=me with all that addressed.

So, somehow the previous patch was missing the mega-exhaustive test, so it's not surprising it might be described as...underwhelming. Still need to go through rest of test requests and see what weren't in the missing file...

(In reply to comment #65) > The changes to JSScopeProperty::trace suggest that the meaning of > JSPROP_GETTER/SETTER has changed (`hasGetterObject() && getterObject()`). Is > that the case? What exactly is the change and why don't we worry about it in > other places where hasGetterObject() or the equivalent is used? has{G,S}etterObject aren't well-motivated methods; I've removed them entirely by inlining them. As for the rest, well, I've actually seen fairly few places where we test for getter/setter and then don't funnel through code that "generalizes" to JS getters/setters that aren't functions. The case where both get/set are undefined is the only weird case I know of right now, and I'm willing to let that one stay as is for now. Also, to be frank, I'm relying a bit on in-the-field testing and experimentation by bleeding-edge users. > In DefinePropertyObject: > >+ /* 8.12.9 step 12. */ > >+ uintN unchanged = 0; > >+ if (!desc.hasConfigurable) > >+ unchanged |= JSPROP_PERMANENT; > >+ if (!desc.hasEnumerable) > >+ unchanged |= JSPROP_ENUMERATE; > >+ > >+ attrs = (desc.attrs & ~unchanged) | (sprop->attrs & unchanged); > > This changes the JSPROP_GETTER and SETTER attrs to whatever desc says, without > actually removing the getter or setter if one is already present: Hm, I'm not sure why the mega-test didn't catch this. Logic adjusted to be much more resilient. > js> obj = {set x(a) print("set")}; > ({set x (a) print("set")}) > js> Object.defineProperty(obj, "x", {get: function () "get"}); > ({get x () "get", set x (a) print("set")}) // setter is shown... > js> obj.x = 13; // setter should be called, I think, but isn't > 13 Added to 15.2.3.6-02.js. Incidentally, with the tweaks above the second line crashes stringifying the object -- fixed that and added it to the test. > Incidentally why is that called "15.2.3.6-02.js"? There's no "15.2.3.6-01.js". There is if I don't omit files. :-) > This doesn't have enough tests. We should have tests for at least the basics, > and more of the corner cases too: > > - defining accessor properties > > - re-defining an accessor property (using defineProperty to add a getter > to a property that already has a setter, and vice versa. > > - redefining an accessor property as a data property > > - redefining a data property to be non-writeable, with and without changing > the value at the same time > > - trying to redefine a function's .length > > - trying to redefine a non-configurable property to be configurable > > - trying to change other details of a non-configurable property > > - redefining a configurable property without changing anything about it > > - defining a generic property > > - using getOwnPropertyDescriptor to examine properties defined by > defineProperty (a fine place for your patented Cartesian Product > Testing technique) > > - trying to use a non-function as a getter or setter These are all in the mega-test. > - trying to define a new property on a sealed object (if `seal` is present) > > - that properties of sealed objects are treated as non-configurable > (currently they aren't reflected that way, which seems like a bug) I think this needs to be punted to bugs for implementing ES5's mutability methods, which don't quite fully map onto how seal() currently works. (Well, they do to an extent, but seal is currently an object-level bit that's very eagerly checked, while ES5's stuff is either property-level or "this object's property set is fixed" and is checked later in the let's-do-something-to-this-property process in the spec.) > - trying to define an array element (not sure what's supposed to happen but > whatever it is, it needs a test) 15.2.3.6-02.js already has a little of this. I'm not really sure how much actually is needed; it's not clear how you might mega-test this in an interesting manner. > - trying to define properties using invalid descriptors, like > Object.defineProperty(obj, "x", {get: f, writeable: true}). I changed 15.2.3.6-01.js to store two sets of descriptors -- one for all possible descriptors from the possible values, one for all possible descriptors that are valid (or otherwise might present potential problems to us, like the null get/set fields as mentioned briefly here earlier). The exhaustive tests for defining a new property use the full set, the n-squared old-and-new tests use the valid set. (I tried using the valid set for old and full set for new to be more exhaustive, but that looked to be an order of magnitude slower when I tried it for 30s or so, which doesn't seem acceptable.) > If this stuff is already in the Microsoft test suite or something, great--how > can I run that? Doubt it, it's been awhile since I played with that -- at least a month or so, because it was back when I had a mini on my desk that I've since donated for tinderbox use. (I have a new box on order, but I'm not going to be back in MV until the latter half of January to set it up.) > Lastly, a test to ponder: > > function C() {} > Object.defineProperty(C.prototype, "x", {get: function () { return 0; }}); > assertEq(new C().hasOwnProperty("x"), false); // fails > assertEq(Object.getOwnPropertyDescriptor(new C), undefined); // succeeds > > The SHARED PERMANENT optimization needs to be rethought, sigh. Is there > already a bug for this? No; I should have seen this. Maybe I sort of did, once, actually -- is there a reason we have to mark accessors with JSPROP_SHARED? We don't have to trace the slot for such properties, but is that really such a huge win at the moment, for this particular case? Removing shared would fix the problem in the short term even if it's not a full, final fix.

The mega-test was too mega: it was timing out when I tried to run it locally. I've split it up into four parts so that it runs in a reasonable time. (Is four parts enough? I hope so, but if it isn't it's not difficult to add more, although it's a little tedious.) To do so I had to make these tests shell-only, because I needed the load() function or an equivalent to share data (and I doubt we wanted all the shared data quadruplicated), and I don't think the browser variant has such a function (really, it should, and load in the shell should be changed to take a callback function or something to conform to some sort of in-browser load API implemented with script tags). I also renamed several of the tests to have more descriptive names, because sequential numbering wasn't cutting it at all when I threw in the split-up-into-parts tests. Family has plans for most of tomorrow morning, so I'll probably push this after noon tomorrow. (Finally!)

Pushed to TM: http://hg.mozilla.org/tracemonkey/rev/766a6b2e74e7 http://hg.mozilla.org/tracemonkey/rev/82cf6cf9a223 http://hg.mozilla.org/tracemonkey/rev/e22652be1ed7

Hum, I forgot the rooting question a few comments back -- now fixed, by making PropertyDescriptor's constructor private (and changing AutoDescriptorArray to return pointers to clean descriptors within it (so any pointer/reference is guaranteed to be rooted). http://hg.mozilla.org/tracemonkey/rev/3628f488e607

I wrote some docs for these methods, still need them to be hooked up from some better location than deep in the JS reference: https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/defineProperties https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/defineProperty

waldo, ecma_5/Object/jstests.list needs to contain ecma_5/Object/defineProperty-setup.js

Erm, really? That file isn't a test itself, it's just used by several. Or do I need to head-fake the jstests thing for test-file packaging or something, by adding it there and adding a nugatory reportCompare call?

Having a file that is not a test and not included in a manifest doesn't affect the running of the tests, but does negate the -c option of jstests.py which checks the contents of manifests against the js files in the test directories. Since this is so trivial, I'll shut up.

Wait, I'm confused. Are you suggesting just "living" with bad -c results? Surely we can do better than that, even if it's just by listing such files in the manifest with a "not-a-test" directive or somesuch. But, it seems to me, anything like that should be a new bug, not something hanging off the end of this one.

I'm saying that if other people don't care, then I don't either. Adding the file to the manifest with a skip and a comment # this file is used by other tests but is not a test is probably the best approach for now.

fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1