Re: [heycam/webidl] Introduce the observable array type (proxy-based) (#840) from Boris Zbarsky on 2020-02-13 (public-webapps-github@w3.org from February 2020)

From: Boris Zbarsky <notifications@github.com>
Date: Wed, 12 Feb 2020 21:28:58 -0800
To: heycam/webidl <webidl@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <heycam/webidl/pull/840/review/357827329@github.com>
bzbarsky commented on this pull request.

There are some caveats in the specific comments, but in general, this seems doable.  Actual implementation might be a bit of a pain in various ways, both in terms of hooking at a low level into attribute getters and setters, and in terms of introducing a totally new type of object.  It's probably doable somewhat sanely, but I haven't thought through the details yet.  

Presumably the interface the property is defined on would need to maintain a mirror of the list that it syncs using the delete/set hooks.  From that point of view, the separate delete and set callbacks when one of the existing indexed properties changed are not ideal: it means either implementations have to diverge from the spec and do a fused delete+set or the mirrored list needs to support a temporary hole, or the mirrored list ends up doing extra shifting around of the list contents.  Adding an optional "old value" to the set hook would address this, I think... though if a mirror is being maintained anyway, just the index is enough, right?  I'm not 100% sure whether there are use cases for not maintaining a mirrored list.

I'm not sure how easy it would be to optimize this sort of thing in JITs, but maybe it's not a huge issue?

One question I do have, just in terms of which bits here are inherent to the proposal vs which are accidental.  What would the observable behavior changes be, if any, if the proxy had _two_ arrays backing it: one the proxy target and one for storing all the indexed props and length state (and possibly all props?).   It'd require a lot more hooks (e.g. `getOwnPropertyDescriptor`), but assuming we implemented those?

Or put another way, is the proposal black-box-identical to having a proxy that returns true for `IsArray` via "magic" (only possible in ES spec terms if target is an array, but engines may have other ways of doing the magic), returns Array.prototype from getPrototypeOf, stores a list of _IDL_ values, not an array of ES values, and defines all the proxy traps appropriately to work with that list of IDL values.  Or, still black-box-identically, have an IDL object with an indexed getter and indexed setter, but returning true from `IsArray` and with `Array.prototype` on the proto chain.

I _think_ that except for the cases when T is a dictionary or sequence type these are all black-box equivalent.  If that's the case, then implementation and optimization is likely _much_ simpler, at the cost of not obviously matching the spec...  If they are not black-box-equivalent, it's worth understanding where they diverge.

> @@ -6775,6 +6782,122 @@ The [=type name=] of a frozen array
 type is the concatenation of the type name for |T| and the string
 "<code>Array</code>".
 
+<h4 id="idl-observable-array" interface lt="ObservableArrayT|ObservableArray&lt;T&gt;">Observable array types — ObservableArray&lt;|T|&gt;</h4>
+
+An <dfn id="dfn-observable-array-type" export>observable array type</dfn> is a parametrized type
+whose values are references to objects of type |T|. The contents of the array can be mutated, both

No, the values are Proxy objects of some sort, as far as I can tell.  The proxy's target is then an Array that stores values of type T (which may be objects or may be numbers or may be whatever).

> +
+Similar to [=sequence types=] and [=frozen array types=], observable array types wrap around
+ECMAScript array types, imposing additional semantics on their usage.
+
+Observable array types must only be used as the type of [=regular attributes=].
+
+<p class="note">We could also allow them as operation return values with a bit more work. Please 
+<a href="https://github.com/heycam/webidl/issues/new?title=Enhancement%20request%20for%20observable%20array%20return%20values">file an issue</a>
+if you would like to use them as such.</p>
+
+For an attribute whose type is an observable array type, specification authors can specify a series
+algorithms:
+
+*   <dfn for="observable array attribute">set an indexed value</dfn>, which accepts an IDL value
+    that is about to be set in the observable array, and the index at which it is being set;
+*   <dfn for="observable array attribute">delete an indexed value</dfn>, which accepts an IDL value

Why does this need both the value and the index? And if it does need both, why does "set an indexed value" not need the old value?  Ah, I guess because sets of an existing thing do the delete first; it might be worth noting that somewhere in the informative text, not just in the algorithms.

> +    <!-- TODO: use highlight="idl" after parser update. -->
+    <pre>
+        [Exposed=Window]
+        interface Building {
+          attribute ObservableArray&lt;Employee> employees;
+        };
+    </pre>
+
+    The behavior of the attribute could be defined like so:
+
+    <blockquote>
+        The [=observable array attribute/set an indexed value=] algorithm for
+        <code class="idl">Building</code>'s <code class="idl">employees</code> attribute, given
+        |employee| and |index|, is:
+
+        1.  If |employee| is not allowed to enter the building today, then throw an

s/an/a/

> +
+    The behavior of the attribute could be defined like so:
+
+    <blockquote>
+        The [=observable array attribute/set an indexed value=] algorithm for
+        <code class="idl">Building</code>'s <code class="idl">employees</code> attribute, given
+        |employee| and |index|, is:
+
+        1.  If |employee| is not allowed to enter the building today, then throw an
+            "{{NotAllowedError}}" {{DOMException}}.
+        1.  If |index| is greater than 200, then throw a "{{QuotaExceededError}}" {{DOMException}}.
+        1.  Put |employee| to work!
+
+        The [=observable array attribute/delete an indexed value=] algorithm for
+        <code class="idl">Building</code>'s <code class="idl">employees</code> attribute, given
+        |employee|, is:

And index?

> +        1.  If |descriptorObj|.\[[Value]] is present and |descriptorObj|.\[[Value]] > |oldLen|,
+            return <emu-val>false</emu-val>.
+    1. Else if |P| [=is an array index=], then:
+        1.  If [=!=] [$IsAccessorDescriptor$](|descriptor|) is <emu-val>true</emu-val>, then return
+            <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Configurable]] field and |descriptor|.\[[Configurable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Enumerable]] field and |descriptor|.\[[Enumerable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Writable]] field and |descriptor|.\[[Writable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If [=!=] [$ToUint32$](|P|) > |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |descriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.

I don't think that holds if ToUint32(P) == oldLen.

> +        1.  If [=!=] [$IsAccessorDescriptor$](|descriptor|) is <emu-val>true</emu-val>, then return
+            <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Configurable]] field and |descriptor|.\[[Configurable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Enumerable]] field and |descriptor|.\[[Enumerable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Writable]] field and |descriptor|.\[[Writable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If [=!=] [$ToUint32$](|P|) > |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |descriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |existingIDLValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the

I don't think this holds either, if handler.[[Type]] is a dictionary type, say.  And the reason it does not is that if the initial dictionary had a missing property it will still be missing and [[Get]]s for it will fall through to `Object.prototype`, which the page can override.

Similar for cases when handler.[[Type]] is a sequence type.

I _think_ records are OK, as are interface types and various non-object types.

Maybe we can/should just disallow dictionaries and sequences as T?  It's sort of weird to have those pass-by-value types as indexed props anyway, since every get would return a new object.

> +        1.  If |descriptor| has a \[[Configurable]] field and |descriptor|.\[[Configurable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Enumerable]] field and |descriptor|.\[[Enumerable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Writable]] field and |descriptor|.\[[Writable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If [=!=] [$ToUint32$](|P|) > |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |descriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |existingIDLValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the
+            conversions when we first stored the value.
+        1.  Perform the algorithm steps given by |handler|.\[[DeleteAlgorithm]], given

We should probably have some sort of guards against the delete/set callbacks calling into the clear/reset algorithms...  Not that I expect people to do that on purpose, but I do expect people to do stuff in there at some point that calls out into JS or something.  Though if they do that we might have a problem anyway.  For example, JS could reset the length to 0, and then defining a property at P would end up with holes.

I'm not sure whether there's a good solution to that if we want to allow the delete/set algorithms to cause the operation to throw instead of completing.  :(

> +        1.  Set |descriptor|.\[[Value]] to |esValue|.
+    1. Return [=?=] |O|.\[[DefineOwnProperty]](|P|, |descriptor|).
+</div>
+
+<h4 id="es-observable-array-deleteProperty"><code>deleteProperty</code></h4>
+
+<div algorithm="observable array exotic object deleteProperty trap">
+    The steps for the <code>deleteProperty</code> proxy trap for
+    [=observable array exotic objects=], given |O| and |P|, are as follows:
+
+    1.  Let |handler| be the <emu-val>this</emu-val> value.
+    1.  If |P| [=is an array index=], then:
+        1.  Let |oldLenDesc| be [=!=] [$OrdinaryGetOwnProperty$](|O|, "length").
+        1.  Assert: [=!=] [$IsDataDescriptor$](|oldLenDesc|) is <emu-val>true</emu-val>.
+        1.  Let |oldLen| be |oldLenDesc|.\[[Value]].
+        1.  If [=!=] [$ToUint32$](|P|) ≠ |oldLen|, return <emu-val>false</emu-val>.

`oldLen - 1`?

> +
+<div algorithm="observable array exotic object deleteProperty trap">
+    The steps for the <code>deleteProperty</code> proxy trap for
+    [=observable array exotic objects=], given |O| and |P|, are as follows:
+
+    1.  Let |handler| be the <emu-val>this</emu-val> value.
+    1.  If |P| [=is an array index=], then:
+        1.  Let |oldLenDesc| be [=!=] [$OrdinaryGetOwnProperty$](|O|, "length").
+        1.  Assert: [=!=] [$IsDataDescriptor$](|oldLenDesc|) is <emu-val>true</emu-val>.
+        1.  Let |oldLen| be |oldLenDesc|.\[[Value]].
+        1.  If [=!=] [$ToUint32$](|P|) ≠ |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the

Same caveats as in defineProperty.

> +    1.  Let |handler| be the <emu-val>this</emu-val> value.
+    1.  If |P| [=is an array index=], then:
+        1.  Let |oldLenDesc| be [=!=] [$OrdinaryGetOwnProperty$](|O|, "length").
+        1.  Assert: [=!=] [$IsDataDescriptor$](|oldLenDesc|) is <emu-val>true</emu-val>.
+        1.  Let |oldLen| be |oldLenDesc|.\[[Value]].
+        1.  If [=!=] [$ToUint32$](|P|) ≠ |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the
+            conversions in
+            <a href="#es-observable-array-defineProperty">the <code>defineProperty</code> trap</a>.
+        1.  Perform the algorithm steps given by |handler|.\[[DeleteAlgorithm]], given
+            |idlValue| and |P|.
+        1.  Perform [=!=] |O|.\[[Delete]](|O|, |P|).

Given that we know `O` is an Array, do we need this step, or can we just let the normal array "length got changed" behavior work?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/840#pullrequestreview-357827329
Received on Thursday, 13 February 2020 05:29:12 UTC