Jeff, I implemented your proposed algorithm in Javascript and ran it against my earlier test framework. It passed all cases. I had to add explicit checks for non-extensibility, otherwise Object.defineProperty would throw. See: http://code.google.com/p/es-lab/source/browse/trunk/src/es5adapt/setProperty.js?spec=svn678&r=678#114
If no one raises any further objections, I'll add the updated algorithm to the wiki page. Cheers, Tom 2012/4/25 Tom Van Cutsem <[email protected]> > 2012/4/24 Jeff Walden <[email protected]> > >> In the [[SetP]] implementation on this page: >> >> http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring >> >> In step 2, the property lookup should stop when a data descriptor of any >> sort, writable or non-writable is uncovered. A property closer to the >> start of a lookup shadows one further along the prototype chain, and these >> semantics don't preserve that. >> > > Right. > > >> Step 5c should return true after calling the setter. >> > > Right again. > > >> Step 5d(i) to recheck for extensibility is redundant with >> [[DefineOwnProperty]]'s check of the same. >> > > This is true, except the difference is observable if Receiver is a proxy: > with the current algorithm, that proxy's defineProperty trap will not be > called. With the extensibility check removed, it will. > > The step 5d(i) check was inspired by the corresponding step 4 check in ES5 > 8.12.4 [[CanPut]] (the spec for [[SetP]] was based on the structure of > [[CanPut]]). In other words: in ES5, technically, the extensibility check > is also performed twice: once in [[CanPut]] and once when > [[DefineOwnProperty]] is called in [[Put]]. > > I would argue to keep the redundant check in there to avoid spurious proxy > trap invocations, but I don't feel strongly about this (the invariant > enforcement mechanism will force a non-extensible proxy's defineProperty > trap to return a consistent return value anyway). > > >> Technically, only step 2 needs to be changed in order to actually make >> the logic sane on the first point. And the second point could be fixed >> with a one-line addition, and the third with a one-line removal. But the >> algorithm's unwieldy enough with just adding more steps (particularly to >> step 2), I think you want a somewhat broader refactoring. I make this >> proposal: >> >> [[SetP]](Receiver, P, V) >> When the [[SetP]] internal method of O is called with initial receiver >> Receiver, property name P, and value V, the following steps are taken: >> >> 1. Let ownDesc be the result of calling the [[GetOwnProperty]] internal >> method of O with argument P. >> 2. If ownDesc is not undefined, then >> a. If IsAccessorDescriptor(ownDesc) is true, then >> i. Let setter be ownDesc.[[Set]]. >> ii. If setter is undefined, return false. >> iii. Call the [[Call]] internal method of setter providing Receiver >> as the this value and providing V as the sole argument. >> iv. Return true. >> b. Otherwise IsDataDescriptor(ownDesc) must be true. >> i. If ownDesc.[[Writable]] is false, return false. >> ii. If Receiver === O, then >> 1. Let updateDesc be the Property Descriptor { [[Value]]: V }. >> 2. Return the result of calling the [[DefineOwnProperty]] >> internal method of Receiver passing P, updateDesc, and false as arguments. >> iii. Else >> 1. Let newDesc be the Property Descriptor {[[Value]]: V, >> [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true}. >> 2. Return the result of calling the [[DefineOwnProperty]] >> internal method of Receiver passing P, newDesc, and false as arguments. >> 3. Let proto be the value of the [[Prototype]] internal property of O. >> 4. If proto is null, then define the property on Receiver: >> a. Let newDesc be the Property Descriptor {[[Value]]: V, [[Writable]]: >> true, [[Enumerable]]: true, [[Configurable]]: true}. >> b. Return the result of calling the [[DefineOwnProperty]] internal >> method of Receiver passing P, newDesc, and false as arguments. >> 5. Return the result of calling the [[SetP]] internal method of proto >> with arguments Receiver, P, and V. >> >> Aside from fixing the noted bugs, this makes one further notable change. >> When the property lookup to determine whether there's a setting conflict >> bottoms out at the end of the prototype chain, without finding the >> property, this algorithm simple defines the property on the receiver as a >> fully mutable property. It doesn't reget the property on the receiver to >> determine if anything's "changed", to set the property consistent with its >> attributes at that instant. First, this seems more efficient. Under the >> current algorithm any property miss must make an effort to reget the >> original property, even "just in case". Second, I have difficulty >> imagining how changes would legitimately happen, in a way that we might >> consider good coding style. But perhaps I'm missing some reason why this >> reget is a design requirement; please let me know if I've missed it. >> > > Your alternative looks good to me. As noted above, I based myself on ES5 > [[Put]] which, because of the call to [[CanPut]], also did the lookup twice. > > The only observable case I can come up with is something along the lines > of: > > var parent = Proxy({}, { > set: function(target, name, value, receiver) { > Object.defineProperty(receiver, name, > { value: 0, > writable: true, > enumerable: true, > configurable: false }); > return Reflect.set(target, name, value, receiver); > } > }); > var child = Object.create(parent); > child.x = 1; > > The last line would trigger parent's "set" trap, which then defines a > non-configurable 'x' on child, and continues the lookup by calling > Reflect.set explicitly. This Reflect.set call reaches the root of the > protochain and then tries to add 'x' again to 'child'. With an explicit > re-check, this will update x's value from 0 to 1 (leaving other attributes > unchanged). Without the re-check, adding 'x' as a configurable property > fails since it's non-configurable. > > As you note, this is not a very motivating example for burdening all > property assignments with redundant checks. In fact, I think the above > behavior without the re-check is equally fine. > > >> Anyway, comments welcome on this -- I'm working on implementing it now, >> so feedback is particularly timely for me, and I'll be able to provide >> implementation feedback quickly. >> > > I previously wrote a little test page that compared the output of ES5's > [[Put]] algorithm against the refactored version: > http://es-lab.googlecode.com/svn/trunk/src/es5adapt/testSetProperty.html > (source is here: > http://code.google.com/p/es-lab/source/browse/#svn%2Ftrunk%2Fsrc%2Fes5adapt > ) > > When I find the time I'll test your proposal to see if it passes. > > That said, any simplification we can make to this algorithm is welcome, so > I like your changes. I would perhaps add a check for [[Extensible]] in step > 4.b. to avoid trapping defineProperty on non-extensible proxy Receivers) > > Cheers, > Tom > > >> >> Jeff >> _______________________________________________ >> es-discuss mailing list >> [email protected] >> https://mail.mozilla.org/listinfo/es-discuss >> > >
_______________________________________________ es-discuss mailing list [email protected] https://mail.mozilla.org/listinfo/es-discuss
