Proto-walking proposal [[SetP]] comments

Tom Van Cutsem tomvc.be at gmail.com
Wed Apr 25 13:00:16 PDT 2012


2012/4/24 Jeff Walden <jwalden+es at mit.edu>

> 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
> es-discuss at mozilla.org
> https://mail.mozilla.org/listinfo/es-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20120425/7b6342b1/attachment.html>


More information about the es-discuss mailing list