Re: [heycam/webidl] Introduce the observable array type (proxy-based) (#840)

bzbarsky requested changes on this pull request.

This is looking pretty good!  There are some semantic differences from existing indexed setter things in `defineProperty`, but that seems ok, I think.  The existing indexed setter semantics of just ignoring attempts to make indices non-configurable instead of throwing are not great.

I have some nits/comments/questions, but the two main substantive things are:

1) The behavior of `defineProperty` when `[[Value]]` is not present in a descriptor for an index property name.
2) The weirdness around the in-range check at the top of "set the length".

> @@ -6775,6 +6784,127 @@ 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 a combination of a mutable list of objects of type |T|, as well as
+behavior to perform when author code modifies the contents of the list.
+
+The parametrized type must not be a [=dictionary type=], [=sequence type=], or [=record type=].
+
+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=].
+
+For an attribute whose type is an observable array type, specification authors can specify a series

"series of"

> @@ -11580,6 +11733,11 @@ in which case they are exposed on every object that [=implements=] the interface
             \[[Enumerable]]: <emu-val>true</emu-val>, \[[Configurable]]: |configurable|}.
         1.  Let |id| be |attr|'s [=identifier=].
         1.  Perform [=!=] <a abstract-op>DefinePropertyOrThrow</a>(|target|, |id|, |desc|).
+            1.  If |attr|'s type is an [=observable array type=] with type argument |T|, then set

This seems over-indented.  I'd think it should just be another item after the DefinePropertyOrThrow, not a sub-item under it.

> @@ -11671,6 +11831,17 @@ in which case they are exposed on every object that [=implements=] the interface
                 1.  Return <emu-val>undefined</emu-val>.
             1.  Set |idlObject| to the IDL [=interface type=] value that represents a reference
                 to |esValue|.
+            1.  If |attribute|'s type is an [=observable array type=] with type argument |T|:
+                1.  Let |newValues| be the result of [=converted to an IDL value|converting=] |V| to

So this happens up front, great.

> @@ -11671,6 +11831,17 @@ in which case they are exposed on every object that [=implements=] the interface
                 1.  Return <emu-val>undefined</emu-val>.
             1.  Set |idlObject| to the IDL [=interface type=] value that represents a reference
                 to |esValue|.
+            1.  If |attribute|'s type is an [=observable array type=] with type argument |T|:
+                1.  Let |newValues| be the result of [=converted to an IDL value|converting=] |V| to
+                    an IDL value of type <a lt="sequence type">sequence&lt;|T|&gt;</a>.
+                1.  Let |oa| be |idlObject|'s |attribute|'s [=backing observable array exotic object=].
+                1.  [=observable array exotic object/Set the length=] of |oa|.\[[ProxyHandler]] to 0.
+                1.  Let |i| be 0.
+                1.  While |i| &lt; |newValues|'s [=list/size=]:
+                    1.  Perform the algorithm steps given by

An exception here would mean we only append the first however many things.  That said, that seems fine; it basically behaves just like `concat()` would.

> +    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>defineProperty</code>", |defineProperty|).
+    1.  Let |deleteProperty| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-deleteProperty]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>deleteProperty</code>", |deleteProperty|).
+    1.  Let |get| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-get]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>get</code>", |get|).
+    1.  Let |getOwnPropertyDescriptor| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-getOwnPropertyDescriptor]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>getOwnPropertyDescriptor</code>", |getOwnPropertyDescriptor|).
+    1.  Let |has| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-has]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>has</code>", |has|).
+    1.  Let |ownKeys| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-ownKeys]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>ownKeys</code>", |ownKeys|).
+    1.  Let |preventExtensions| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-preventExtensions]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>preventExtensions</code>", |preventExtensions|).
+    1.  Let |set| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-set]], « », |realm|).
+    1.  Perform [=!=] [$CreateDataProperty$](|handler|, "<code>set</code>", |set|).
+    1.  Return [=!=] [$ProxyCreate$](|innerArray|, |handler|).

Do we want to just use the forwarding `setPrototypeOf` like this proposal does?  It seems we could go either way on that, and we generally don't prevent setting prototypes of random things in the web platform, so OK.

> +    1.  If |P| is "length", then return the result of
+        [=observable array exotic object/setting the length=] given |handler| and |V|.
+    1.  If |P| [=is an array index=], then return the result of
+        [=observable array exotic object/setting the indexed value=] given |handler|, |P|, and |V|.
+    1.  Return [=?=] |O|.\[[Set]](|P|, |V|, |Receiver|).
+</div>
+
+<h4 id="es-observable-array-abstract-operations">Abstract operations</h4>
+
+<div algorithm>
+    To <dfn for="observable array exotic object" lt="set the length|setting the length">set the length</dfn>
+    of an observable array exotic object given |handler| and |newLen|: 
+
+    1.  Set |newLen| to [=?=] [$ToUint32$](|newLen|).
+    1.  Let |numberLen| be [=?=] [$ToNumber$](|newLen|).
+    1.  If |newLen| ≠ |numberLen|, then throw a {{RangeError}} exception.

I don't follow this step.  `newLen` after step 1 is a 32-bit unsigned integer.  `numberLen` will be exactly equal to `newLen`, since all 32-bit integers are exactly representable as doubles.

I assume that what this really wants to do is `ToNumber` the incoming value (which could fail), then `ToUint32` the resulting number (this part is infallible), then compare the 32-bit int to the number.  It's worth calling out that this is numeric comparison as doubles or something, so a 0 int tests equal to a `-0` double, but tests unequal to a `NaN`.



> +            |handler|.\[[BackingList]][|indexToDelete|] and |indexToDelete|.
+        1.  [=list/Remove=] the last item from |handler|.\[[BackingList]].
+        1.  Set |indexToDelete| to |indexToDelete| &minus; 1.
+</div>
+
+<div algorithm>
+    To <dfn for="observable array exotic object" lt="set the indexed value|setting the indexed value">set the indexed value</dfn>
+    of an observable array exotic object given |handler|, |P|, and |V|: 
+
+    1.  Let |oldLen| be |handler|.\[[BackingList]]'s [=list/size=].
+    1.  Let |index| be [=!=] [$ToUint32$](|P|).
+    1.  If |index| > |oldLen|, return <emu-val>false</emu-val>.
+    1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+        |V| to the type given by |handler|.\[[Type]].
+    1.  If |index| &lt; |oldLen|, then:
+        1.  Perform the algorithm steps given by |handler|.\[[DeleteAlgorithm]], given

It's worth noting somewhere where the overall concept is defined that a set will do the delete algorithm, then the set algorithm, and only then change the value.  Because a plausible alternative would have been to just do the set algorithm and let the implementation figure out that there's the implicit delete of the old value...  Either choice is OK, I think; we just need to make sure we make it clear to someone who has not read these specific steps which one is happening.

> +        1.  If |descriptor|.\[[Value]] is present, then return the result of
+            [=observable array exotic object/setting the length=] given |handler| and
+            |descriptor|.\[[Value]].
+        1.  Return <emu-val>true</emu-val>.
+    1. 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|.\[[Configurable]] is present and has the value <emu-val>false</emu-val>,
+            then return <emu-val>false</emu-val>.
+        1.  If |descriptor|.\[[Enumerable]] is present and has the value <emu-val>false</emu-val>,
+            then return <emu-val>false</emu-val>.
+        1.  If |descriptor|.\[[Writable]] is present and has the value <emu-val>false</emu-val>,
+            then return <emu-val>false</emu-val>.
+        1.  If |descriptor|.\[[Value]] is present, then return the result of
+            [=observable array exotic object/setting the indexed value=] given |handler|, |P|, and
+            |descriptor|.\[[Value]].

If `[[Value]]` is not present, do we really want to fall through to `O.[[DefineOwnProperty]]`?  That will create an indexed property on the array with the value `undefined`, if I read the ES spec correctly.  Seems weird to me to allow that.

> +
+<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 "length", then return <emu-val>false</emu-val>.
+    1.  If |P| [=is an array index=], then:
+        1.  Let |oldLen| be |handler|.\[[BackingList]]'s [=list/size=].
+        1.  Let |index| be [=!=] [$ToUint32$](|P|).
+        1.  If |index| ≠ |oldLen| &minus; 1, then return
+            <emu-val>false</emu-val>.
+        1.  Perform the algorithm steps given by |handler|.\[[DeleteAlgorithm]], given
+            |handler|.\[[BackingList]][|index|] and |index|.
+        1.  [=list/Remove=] the last item from |handler|.\[[BackingList]].
+        1.  Perform [=!=] |O|.\[[DefineOwnProperty]]("length", PropertyDescriptor{\[[Value]]: |oldLen| &minus; 1}).

Why is this step needed?  I don't think it's needed...  nothing looks at "length" on the target array, right?

> +</div>
+
+<h4 id="es-observable-array-ownKeys"><code>ownKeys</code></h4>
+
+<div algorithm="observable array exotic object ownKeys trap">
+    The steps for the <code>ownKeys</code> proxy trap for
+    [=observable array exotic objects=], given |O|, are as follows:
+
+    1.  Let |handler| be the <emu-val>this</emu-val> value.
+    1.  Let |length| be |handler|.\[[BackingList]]'s [=list/size=].
+    1.  Let |keys| be an empty [=list=].
+    1.  Let |i| be 0.
+    1.  [=While=] |i| &lt; |length|:
+        1.  [=list/Append=] [=!=] [$ToString$](|i|) to |keys|.
+        1.  Set |i| to |i| + 1.
+    1.  [=list/Extend=] |keys| with [=!=] |O|.\[[OwnPropertyKeys]]().

Aha, here the "define a random index with undefined as value on the target array" bit would be observable.  So yes, I think we should prevent defining it in `defineProperty` by either no-opping or throwing or treating no `[[Value]]` as undefined and doing the usual IDL type coercion (e.g. converting to 0 for numeric types).

-- 
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-372257720