Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Darien Valentine
The following instrinsic functions invoke the ToPropertyDescriptor internal operation to convert JS-object property descriptors into "Property Descriptors" (spec-internal object type):

- `Reflect.defineProperty`
- `Object.create`
- `Object.defineProperty`
- `Object.defineProperties`
- a `Proxy` instance if its handler includes `getOwnPropertyDescriptor`

The ToPropertyDescriptor operation uses HasProperty to check for and the presence of the following properties of a JS object property descriptor in the order enumerable, configurable, value, writable, get, set.

If any two incompatible properties are found to be “had properties,” an error is thrown.

It seems unfortunate that this works:

```js
function breakEveryPropertyDescriptor() {
  Object.prototype.value = undefined;
  Object.prototype.get = undefined;
}
```

But this has been mentioned before. I found this prior discussion:


As explained there, one can address this, if sufficiently paranoid, by only ever passing in null-prototype objects (or objects with a prototype you control) as JS-object property descriptors.

It gets a bit more complex. FromPropertyDescriptor produces JS-object PDs which also inherit from %ObjectPrototype%. Naturally you can perform the own-property check yourself — but it has a consequence I find particularly interesting:

```
const obj1 = {};
const obj2 = new Proxy({}, Reflect);

breakEveryPropertyDescriptor();

const pd = Object.assign(Object.create(null), { value: 1 });

try {
  Reflect.defineProperty(obj1, 'foo', pd);
  console.log('successful on obj1');
} catch {
  console.log('unsuccessful on obj1');
}

try {
  Reflect.defineProperty(obj2, 'foo', pd);
  console.log('successful on obj2');
} catch {
  console.log('unsuccessful on obj2');
}
```

In the Proxy case, the input JS PD object is converted to an internal Property Descriptor (`{ value: 1 }`) and then back to a fresh JS object (now with %ObjectPrototype%) — and then back to an internal Property Descriptor again. But it doesn’t successfully round trip! At this the result of HasProperty(Obj, "get") changes from what it would have been if the PD had not passed through Reflect.defineProperty, and the descriptor has become `{ value: 1, get: undefined }`.

My understanding was that, in theory, using `Reflect` as your handler object should mean the behavior of all the trapped operations should be the same as it would have been for an ordinary object. This may be an incorrect assumption on my part (are there other examples like this?), but it does seem desirable for that to be true.

I’m curious about whether changes to these algorithms to alter this behavior in some way would be web-safe. The obvious solution would be switching HasProperty to HasOwnProperty in ToPropertyDescriptor and/or creating objects that inherit from null in FromPropertyDescriptor. But I’m guessing those changes would not be safe. In any case, I wanted to share the non-parity edge case with Reflect.defineProperty in case this was not already a known quirk.

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Darien Valentine
On reflection (harrrr) I realize that there is an almost certainly safe change which can be made to address this in the Reflect.defineProperty case, which is to make Reflect.defineProperty alone return an object with a null prototype. I believe this would make it behave the same as ordinary [[defineOwnProperty]] in all cases.

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Isiah Meadows-2
In reply to this post by Darien Valentine
Personally, I'd prefer it just be handled this way:

- If either `desc.get` or `desc.set` is not `undefined`, treat it as a
data descriptor.
- Otherwise, treat it as a value descriptor and ignore `desc.get` and
`desc.set`.

It'd be much easier to reason about, and I've personally had a few
issues around this area - it's annoying to have to build objects
polymorphically for the only area where [[HasProperty]] actually has
any major semantic meaning and the object in question isn't a
key/value dictionary (like for `Object.defineProperties`). Most
everything else is just based on [[Get]], so it's just odd and
annoying.

-----

Isiah Meadows
[hidden email]
www.isiahmeadows.com

On Tue, Sep 4, 2018 at 3:32 AM Darien Valentine <[hidden email]> wrote:

>
> The following instrinsic functions invoke the ToPropertyDescriptor internal operation to convert JS-object property descriptors into "Property Descriptors" (spec-internal object type):
>
> - `Reflect.defineProperty`
> - `Object.create`
> - `Object.defineProperty`
> - `Object.defineProperties`
> - a `Proxy` instance if its handler includes `getOwnPropertyDescriptor`
>
> The ToPropertyDescriptor operation uses HasProperty to check for and the presence of the following properties of a JS object property descriptor in the order enumerable, configurable, value, writable, get, set.
>
> If any two incompatible properties are found to be “had properties,” an error is thrown.
>
> It seems unfortunate that this works:
>
> ```js
> function breakEveryPropertyDescriptor() {
>   Object.prototype.value = undefined;
>   Object.prototype.get = undefined;
> }
> ```
>
> But this has been mentioned before. I found this prior discussion:
>
> https://esdiscuss.org/topic/topropertydescriptor-hasproperty-hasownproperty
>
> As explained there, one can address this, if sufficiently paranoid, by only ever passing in null-prototype objects (or objects with a prototype you control) as JS-object property descriptors.
>
> It gets a bit more complex. FromPropertyDescriptor produces JS-object PDs which also inherit from %ObjectPrototype%. Naturally you can perform the own-property check yourself — but it has a consequence I find particularly interesting:
>
> ```
> const obj1 = {};
> const obj2 = new Proxy({}, Reflect);
>
> breakEveryPropertyDescriptor();
>
> const pd = Object.assign(Object.create(null), { value: 1 });
>
> try {
>   Reflect.defineProperty(obj1, 'foo', pd);
>   console.log('successful on obj1');
> } catch {
>   console.log('unsuccessful on obj1');
> }
>
> try {
>   Reflect.defineProperty(obj2, 'foo', pd);
>   console.log('successful on obj2');
> } catch {
>   console.log('unsuccessful on obj2');
> }
> ```
>
> In the Proxy case, the input JS PD object is converted to an internal Property Descriptor (`{ value: 1 }`) and then back to a fresh JS object (now with %ObjectPrototype%) — and then back to an internal Property Descriptor again. But it doesn’t successfully round trip! At this the result of HasProperty(Obj, "get") changes from what it would have been if the PD had not passed through Reflect.defineProperty, and the descriptor has become `{ value: 1, get: undefined }`.
>
> My understanding was that, in theory, using `Reflect` as your handler object should mean the behavior of all the trapped operations should be the same as it would have been for an ordinary object. This may be an incorrect assumption on my part (are there other examples like this?), but it does seem desirable for that to be true.
>
> I’m curious about whether changes to these algorithms to alter this behavior in some way would be web-safe. The obvious solution would be switching HasProperty to HasOwnProperty in ToPropertyDescriptor and/or creating objects that inherit from null in FromPropertyDescriptor. But I’m guessing those changes would not be safe. In any case, I wanted to share the non-parity edge case with Reflect.defineProperty in case this was not already a known quirk.
> _______________________________________________
> es-discuss mailing list
> [hidden email]
> https://mail.mozilla.org/listinfo/es-discuss
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Darien Valentine
@Isiah:

That seems like it would be likely to be a safe change since it would only make currently invalid PDs become valid. However I’m unsure if it’s sufficiently unambiguous as stated on account of PDs which set only metadata without also overwriting an existing get/set/value. For example, would be the effect of the PD `{ get: undefined, set: undefined, enumerable: false }` be on an existing accessor property `{ configurable: true, get: fn, enumerable: true }`? Does it say to set the existing property to be unenumerable, like `{ enumerable: false }` does presently? Or does it ask to redefine it as a data property whose value is `undefined`?.

Also, on its own, such a change would not address the Reflect.defineProperty non-parity quirk.

On Tue, Sep 4, 2018 at 3:39 AM Isiah Meadows <[hidden email]> wrote:
Personally, I'd prefer it just be handled this way:

- If either `desc.get` or `desc.set` is not `undefined`, treat it as a
data descriptor.
- Otherwise, treat it as a value descriptor and ignore `desc.get` and
`desc.set`.

It'd be much easier to reason about, and I've personally had a few
issues around this area - it's annoying to have to build objects
polymorphically for the only area where [[HasProperty]] actually has
any major semantic meaning and the object in question isn't a
key/value dictionary (like for `Object.defineProperties`). Most
everything else is just based on [[Get]], so it's just odd and
annoying.

-----

Isiah Meadows
[hidden email]
www.isiahmeadows.com

On Tue, Sep 4, 2018 at 3:32 AM Darien Valentine <[hidden email]> wrote:
>
> The following instrinsic functions invoke the ToPropertyDescriptor internal operation to convert JS-object property descriptors into "Property Descriptors" (spec-internal object type):
>
> - `Reflect.defineProperty`
> - `Object.create`
> - `Object.defineProperty`
> - `Object.defineProperties`
> - a `Proxy` instance if its handler includes `getOwnPropertyDescriptor`
>
> The ToPropertyDescriptor operation uses HasProperty to check for and the presence of the following properties of a JS object property descriptor in the order enumerable, configurable, value, writable, get, set.
>
> If any two incompatible properties are found to be “had properties,” an error is thrown.
>
> It seems unfortunate that this works:
>
> ```js
> function breakEveryPropertyDescriptor() {
>   Object.prototype.value = undefined;
>   Object.prototype.get = undefined;
> }
> ```
>
> But this has been mentioned before. I found this prior discussion:
>
> https://esdiscuss.org/topic/topropertydescriptor-hasproperty-hasownproperty
>
> As explained there, one can address this, if sufficiently paranoid, by only ever passing in null-prototype objects (or objects with a prototype you control) as JS-object property descriptors.
>
> It gets a bit more complex. FromPropertyDescriptor produces JS-object PDs which also inherit from %ObjectPrototype%. Naturally you can perform the own-property check yourself — but it has a consequence I find particularly interesting:
>
> ```
> const obj1 = {};
> const obj2 = new Proxy({}, Reflect);
>
> breakEveryPropertyDescriptor();
>
> const pd = Object.assign(Object.create(null), { value: 1 });
>
> try {
>   Reflect.defineProperty(obj1, 'foo', pd);
>   console.log('successful on obj1');
> } catch {
>   console.log('unsuccessful on obj1');
> }
>
> try {
>   Reflect.defineProperty(obj2, 'foo', pd);
>   console.log('successful on obj2');
> } catch {
>   console.log('unsuccessful on obj2');
> }
> ```
>
> In the Proxy case, the input JS PD object is converted to an internal Property Descriptor (`{ value: 1 }`) and then back to a fresh JS object (now with %ObjectPrototype%) — and then back to an internal Property Descriptor again. But it doesn’t successfully round trip! At this the result of HasProperty(Obj, "get") changes from what it would have been if the PD had not passed through Reflect.defineProperty, and the descriptor has become `{ value: 1, get: undefined }`.
>
> My understanding was that, in theory, using `Reflect` as your handler object should mean the behavior of all the trapped operations should be the same as it would have been for an ordinary object. This may be an incorrect assumption on my part (are there other examples like this?), but it does seem desirable for that to be true.
>
> I’m curious about whether changes to these algorithms to alter this behavior in some way would be web-safe. The obvious solution would be switching HasProperty to HasOwnProperty in ToPropertyDescriptor and/or creating objects that inherit from null in FromPropertyDescriptor. But I’m guessing those changes would not be safe. In any case, I wanted to share the non-parity edge case with Reflect.defineProperty in case this was not already a known quirk.
> _______________________________________________
> es-discuss mailing list
> [hidden email]
> https://mail.mozilla.org/listinfo/es-discuss

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Allen Wirfs-Brock
In reply to this post by Darien Valentine

On Sep 4, 2018, at 12:32 AM, Darien Valentine <[hidden email]> wrote:

My understanding was that, in theory, using `Reflect` as your handler object should mean the behavior of all the trapped operations should be the same as it would have been for an ordinary object. This may be an incorrect assumption on my part (are there other examples like this?), but it does seem desirable for that to be true.

Yes, this is surprising (at least to me) and slightly disturbing.

At one point, early in the development of ES6 the draft spec. had a [[OriginalDescriptor]] field  (that may not be the actual name I used) in internal PropertyDescrptors that carried along a reference to the original descriptor object from which the PropertyDescriptor was derived. When the Proxy traps needed to reify a PropertyDescriptor as an object it would use the [[OriginalDescriptor]] if it had a value. The reason for having [[OriginalDescriptor]] wasn’t because of inheriting from %ObjectPrototype%.  It was to enable Proxies to extend property descriptor objects with additional attributes (for example, imagine a public: <boolean> attribute) that the Proxy handler could then interpret.

[[OriginalDescriptor]] was removed from the spec. after some implementors pushed back with cncerns about the added complexity and skepticism about the value of the added utility.

I’m curious about whether changes to these algorithms to alter this behavior in some way would be web-safe. 

Unlikely, part of the original intent of the descriptor object design was that prototypal inheritance could be used compose descriptor objects (BTW, there are es-discuss threads about these inheritance issues far older than the one you referenced).  consider:

function friendlyDefaults (desc) {return Object.setPrototypeOf(desc, {enumerable: true, configurable: true, writable: true})};

var obj = Object.create(Object.prototype, {
   p1: friendlyDefaults({value: 1}),
   p2: friendlyDefaults({value: 2}),
   p3: friendlyDefaults({value: 3})
});

I won’t want to bet that nobody on the web has ever used prototypal inheritance to construct their property descriptors.

Similarly, I won’t want to bet that nobody on the web has written a Proxy trap handler where they have invoked the hasOwnProperty method on a reified descriptor object.

I think that bringing back the [[OriginalDescriptor]] concept might be web-safe, but good luck on convincing implementors that this problem is worth the effort. (I actually think, the overhead would not be so bad because implementations don’t actually  create PropertyDescriptors in normal operation.  It is only operations that start with a Object.* or Reflect.* operation that create the round tripping concern.)


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Reflect.defineProperty + FromPropertyDescriptor & ToPropertyDescriptor

Michael Dyck
On 2018-09-04 12:02 PM, Allen Wirfs-Brock wrote:
>
> At one point, early in the development of ES6 the draft spec. had a
> [[OriginalDescriptor]] field  (that may not be the actual name I used) in
> internal PropertyDescrptors that carried along a reference to the original
> descriptor object from which the PropertyDescriptor was derived.

It looks like you're talking about [[Origin]].

Working drafts 12 though 25 had this wording or similar:
   A Property Descriptor may be derived from an object that has properties
   that directly correspond to the fields of a Property Descriptor. Such a
   derived Property Descriptor has an additional field named [[Origin]]
   whose value is the object from which the Property Descriptor was derived.

(In case that helps anyone researching this.)

-Michael

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss