Introduce the observable array type (proxy-based) #840

domenic · 2020-02-03T21:12:23Z

Part of #796.

This is intended as a better version of #836 that (on a spec level) uses proxy traps, instead of Web IDL legacy platform objects, to fix the issues noted in #796 (comment).

This also includes a second commit which uses the suggestions from @bzbarsky in #836 (comment) to avoid the resulting array having holes. This seems like a good thing in various ways, although I am a little unsure whether I successfully pulled it off.

This pull request also changes how spec authors are expected to interface with the observable array type, per some of the discussions in #836. Here is a pull request to the constructible stylesheets repository to show those in action: WICG/construct-stylesheets#117. It seems pretty reasonable. These changes in interface are portable back to the platform-object-based approach in #836, if we desire.

Preview | Diff

Part of #796.

bzbarsky

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.

index.bs

bzbarsky · 2020-02-12T22:14:15Z

index.bs

+
+*   <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.

It's not clear what the right stuff to pass in is; I'm kind of just making up hooks as we go along. WICG/construct-stylesheets#117 uses it in a handwavey way. I guess I'll try to make that less handwavey (as described above) and see what's still wanted.

index.bs

bzbarsky · 2020-02-13T04:59:01Z

index.bs

+            |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. :(

We could add a shared reentrancy guard into every proxy trap (or at least every mutating proxy trap). I.e. a per-handler [[IsMutating]] boolean which, if true, causes any mutating proxy traps to throw immediately. Do you think it's worth it?

Given that the only example of delete/set callbacks that we have is this very simple one I'm not sure whether this would be "setting ourselves up to avoid future problems", or "overbuilding complicated infrastructure we won't really need".

Maybe the middle road is to add just some prose prohibition on calling JS code or mutating the backing list from these handlers, and assume spec authors will follow it.

Yeah. I think this might be less of an issue in the new setup, depending on how mutations of the backing store are structured; will take a look.

index.bs

domenic

Thanks for taking a look, and sorry for letting your reply sit!

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. [...] I'm not 100% sure whether there are use cases for not maintaining a mirrored list.

I've been going back and forth on this. Now that we're trying to maintain the no-holes invariant, we could maintain the mirror list in the IDL spec and let spec authors refer to it without much trouble.

I went with the current approach based on @othermaciej's comments around #836 (comment), which indicated a desire to align with indexed setters/getters. Notably classes that use those must sync to any mirrors manually, if they want to, and some classes that use indexed setters don't seem to want such a mirror. (Example: HTMLOptionsCollection.)

I attempted to prototype what would be needed for the adoptedStyleSheets case in WICG/construct-stylesheets#117, but it ended up not being super-helpful, because it's building on non-rigorous foundations. (Apparently nothing in CSS actually dictates what style sheets are applied to a page; see discussion in WICG/construct-stylesheets#118.) Probably the best step is to try to rigorize those further and see if an IDL-maintained mirror list would be helpful or not. I suspect it would be.

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 [...]. I think that except for the cases when T is a dictionary or sequence type these are all black-box equivalent.

With the no-holes prohibition in place, I agree it should be black-box identical. (Without that the backing list gets awkward and needs to have holes in it.) I had some version of this locally before abandoning it for the current one, which reuses more machinery.

The fact that you caught the dictionary/sequence type problem is a little worrying that there might be other things we missed, but disallowing those for now seems good...

index.bs

domenic · 2020-02-19T19:11:04Z

index.bs

+
+*   <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


It's not clear what the right stuff to pass in is; I'm kind of just making up hooks as we go along. WICG/construct-stylesheets#117 uses it in a handwavey way. I guess I'll try to make that less handwavey (as described above) and see what's still wanted.

index.bs

domenic · 2020-02-19T21:21:03Z

Probably the best step is to try to rigorize those further and see if an IDL-maintained mirror list would be helpful or not. I suspect it would be.

My conclusion in WICG/construct-stylesheets#118 (comment) is that at the spec level we would only need the IDL-maintained mirror list ("backing list") and wouldn't even need the add/remove hooks. I'm unsure what to do with the hooks, given that. I mean, implementations will probably want them, I think?

Edit: this is wrong. We need the add hook to prevent invalid CSSStyleSheet objects from entering the backing list.

This is kind of suboptimal because spec authors must not modify it. Adding more traps would let us do this better.

domenic · 2020-02-19T21:59:16Z

With the no-holes prohibition in place, I agree it should be black-box identical. (Without that the backing list gets awkward and needs to have holes in it.) I had some version of this locally before abandoning it for the current one, which reuses more machinery.

I am re-creating this version now (but didn't quite finish by end of day) since if we have a backing list, it's much more convenient to let spec authors manipulate it, and this only works if we also implement the getOwnProperty / ownKeys / etc. handlers to reflect any spec-side changes to the backing list. Combined with how a version with the extra handlers sounds closer to how folks would implement this, it seems like the right direction to go.

(I have a question on whether we need get/set/hasProperty traps too if you have any idea.)

TimothyGu · 2020-02-19T23:33:19Z

Since we are defining the object in terms of Proxy, my assumption is that redundant hooks are necessary. If we don’t define a get hook, then the target’s version of [[GetOwnProperty]] will be used instead of the getOwnPropertyDescriptor hook, since the spec just calls out to target.[[Get]] directly. This is different from overriding internal methods, where any change to internal methods will be picked up by other ones automatically.

(Hence, the question about whether Integer-Indexed Exotic Objects need those overridden internal methods, is different from the question we have here.)

domenic · 2020-02-20T20:05:54Z

This is ready for another round of review. I updated to override all the traps, and store the data in the backing list, not the indexed properties of the target. This is more verbose on the IDL specification side, but has a number of advantages:

Better for specification authors using this, as now they can refer to and manipulate the backing list using any operation, and it will be reflected appropriately.
The algorithms are more obviously correct now, as they each special-case length and indexed properties, then pass along everything else. Before, when we were reusing a lot of the array infrastructure, it was less obvious what would work, since you'd have to know facts like "setting length calls [[Delete]] on the deleted properties" or "defining a property with no [[Value]] in the descriptor will eventually do nothing if we pass it through".
Closer to how implementations would likely work, if they piggybacked off of legacy platform object infrastructure. I.e. this more obviously meets @bzbarsky's idea of

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.

bzbarsky · 2020-02-20T20:22:05Z

Notably classes that use those must sync to any mirrors manually, if they want to, and some classes that use indexed setters don't seem to want such a mirror. (Example: HTMLOptionsCollection.)

Well.. HTMLOptionsCollection does maintain a mirror, in the form of the <option> kids of the <select>. I can see how there was a sort of terminology issue here...

because it's building on non-rigorous foundations. (Apparently nothing in CSS actually dictates what style sheets are applied to a page

Yup. The current rigor of the CSS specs is awful.

With the no-holes prohibition in place, I agree it should be black-box identical. (Without that the backing list gets awkward and needs to have holes in it.) I had some version of this locally before abandoning it for the current one, which reuses more machinery.

Right. To be clear, I am fine with having a clean spec abstraction, if it's non-leaky enough that actual implementation can do something observably same but possibly more efficient or reusing more existing implementation machinery.

bzbarsky · 2020-02-20T20:30:18Z

My conclusion in WICG/construct-stylesheets#118 (comment) is that at the spec level we would only need the IDL-maintained mirror list ("backing list") and wouldn't even need the add/remove hooks.

Hmm. That's assuming that something else effectively polls the list, right? That is, instead of having an explicit "this list got changed, go flag the rendering as needing updating" in the spec we'd just say in places that update the rendering or compute styles that they need to get the current values in the list.

I mean, implementations will probably want them, I think?

Right, implementations are going to want to invalidate cached information when the list mutates and whatnot.

And good catch about the "add" hook.

That said, if we maintain the mirror directly in the proxy, as a matter of what the spec looks like we could do the following:

Have the target be an array.
Have a slot with the "mirror" list (which is actually just The List).
Indexed props and "length" just read/modify the mirrored list; don't touch the array at all.
Other props, proto gets/modifications, etc go through to the array, for convenience.

This does require implementing a bunch of proxy hooks, unfortunately. But it makes it very clear what the state of the list is, and implementations can do whatever they want in terms of the list maintenance, including whatever internal callbacks they like when it mutates, and it's very clear that that's identical to the spec behavior.

Note that I have not had a chance to read the most recent updated diff yet, so maybe that's what it's already doing.

domenic · 2020-02-20T20:33:31Z

Note that I have not had a chance to read the most recent updated diff yet, so maybe that's what it's already doing.

Yep, that's the current diff (modulo bugs) ^_^. And yeah, I agree with the benefits you mention.

TimothyGu

Couldn't spot any errors in the proxy traps themselves, but some editorial comments are provided.

index.bs

bzbarsky · 2020-03-09T14:36:02Z

@EdgarChen

bzbarsky

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:

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

index.bs

bzbarsky · 2020-03-10T19:46:11Z

index.bs

+    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.

If we can think of some even-slightly-compelling reason to prevent setting the prototype, I'm happy to add it. In general I think JS engine folks desperately wish Array instances could not get their prototypes changed, as that would make the VM work easier in various ways, but at least the implementation-side reasoning doesn't apply here...

I haven't thought of a super-compelling reason so far, unfortunately.

bzbarsky · 2020-03-10T19:56:12Z

index.bs

+
+    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.

This is modeled on https://tc39.es/ecma262/#sec-arraysetlength, but I messed up by overwriting newLen, yeah.

My reading of the corresponding ES section is that -0 and +0 do not test equal. In particular both variables end up as doubles, per the defaults in https://tc39.es/ecma262/#sec-mathematical-operations.

Right, but when working with IEEE doubles, -0 == +0 tests true, in general. So the important part is that the ≠ here is a numeric comparison, not an Object.is() kind of thing. And in particular, assigning -0 to length should work fine and set the length to 0.

This is modeled on https://tc39.es/ecma262/#sec-arraysetlength

Huh. Indeed, that does the to-number conversion twice, look at that. Alright then; we should stay compatible with that but make sure we add tests for it (using a Symbol.toPrimitive with side-effects on an object that length is then set to).

index.bs

bzbarsky · 2020-03-10T20:39:17Z

And my apologies for the terrible lag here. :(

bzbarsky · 2020-03-11T19:52:02Z

@petervanderbeken

domenic · 2020-03-12T19:40:46Z

OK, updated! Both of the big issues you found were instances of me emulating an existing ES spec pattern, but messing up in some way. So I think they should be good now.

bzbarsky

Thank you, this looks great!

index.bs

TimothyGu · 2020-03-15T06:02:39Z

index.bs

+defined in this manner because the ECMAScript specification includes special treatment for
+[=Proxy exotic objects=] that have <code>Array</code> instances as their proxy target, and we want
+to ensure that [=observable array types=] are exposed to ECMAScript code with this special treatment
+intact.


I think we should be clear what this special treatment is (as far as I can tell, IsArray). Even better, I think an implementation note that this is (as @bzbarsky said) "black-box-identical" to having "an IDL object with an indexed getter and indexed setter, but returning true from IsArray and with Array.prototype on the proto chain".

I feel it is regrettable that observable arrays have to be defined this way, and I think having more context in the spec itself will help people in the future understand the decisions being made now.

"black-box-identical" to having "an IDL object with an indexed getter and indexed setter, but returning true from IsArray and with Array.prototype on the proto chain"

Now that I have read the details, that's not quite the case. The behavior of defineProperty with various interesting attributes set in the descriptor is different, length is an own property instead of a prototype one, etc. The two things are similar, and can share a lot of implementation, I suspect, but are not quite identical.

annevk · 2020-12-18T08:40:06Z

Were bugs filed for this change? (Edit: filed https://bugzilla.mozilla.org/show_bug.cgi?id=1683281 on Fx.)

rakuco · 2021-10-18T20:06:49Z

FTR, the corresponding Chromium bug is https://crbug.com/1201744. I couldn't find anything on bugs.webkit.org

domenic added 2 commits February 3, 2020 15:38

Introduce the observable array type (proxy-based)

8dda4ad

Part of #796.

Ensure there are no holes in the array

18fa303

domenic mentioned this pull request Feb 3, 2020

Use observable array for adoptedStyleSheets WICG/construct-stylesheets#117

Closed

bzbarsky reviewed Feb 13, 2020

View reviewed changes

nordzilla mentioned this pull request Feb 14, 2020

Consider disallowing duplicate stylesheets WICG/construct-stylesheets#120

Open

domenic commented Feb 19, 2020

View reviewed changes

Code review comments

a613257

domenic mentioned this pull request Feb 19, 2020

Spec requires adopted stylesheets to be included in styleSheets property but doesn't require them to be used for styling WICG/construct-stylesheets#118

Open

domenic added 2 commits February 19, 2020 16:22

Touchups

433face

Track the backing list

d3cc626

This is kind of suboptimal because spec authors must not modify it. Adding more traps would let us do this better.

Add all the traps

119cec7

"whose values are" plus index param code review comments

eb3960a

TimothyGu reviewed Feb 21, 2020

View reviewed changes

Review comments from Timothy

152372f

bzbarsky requested changes Mar 10, 2020

View reviewed changes

domenic added 3 commits March 12, 2020 15:30

Easy review comments

d8d5bec

Forgot to commit this easy review comment

77686bb

Fix defineProperty with no [[Value]] to no-op

85c0e54

bzbarsky approved these changes Mar 12, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

bzbarsky mentioned this pull request Mar 13, 2020

Figure-out what supporting Array-subclassing implies #345

Open

domenic mentioned this pull request Mar 13, 2020

Introduce the observable array type (legacy platform object based) #836

Closed

Swap the equal variables

7c3be07

TimothyGu reviewed Mar 15, 2020

View reviewed changes

mfreed7 mentioned this pull request Mar 16, 2020

Instead of assignable FrozenArray, use add / remove WICG/construct-stylesheets#45

Closed

domenic merged commit e6ae134 into master Mar 18, 2020

domenic deleted the observablearray-proxy branch March 18, 2020 21:37

saschanaz mentioned this pull request Jun 28, 2020

feat(productions/type): support ObservableArray w3c/webidl2.js#481

Merged

2 tasks

annevk mentioned this pull request Dec 18, 2020

A new approach to array-ish classes #796

Open

domenic mentioned this pull request Dec 1, 2021

ObservableArray, and its use by adoptedStyleSheets w3ctag/design-reviews#693

Closed

1 task

domenic mentioned this pull request May 3, 2022

ObservableArray types in namespace #1000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the observable array type (proxy-based) #840

Introduce the observable array type (proxy-based) #840

domenic commented Feb 3, 2020 •

edited by pr-preview bot

bzbarsky left a comment

bzbarsky Feb 12, 2020

domenic Feb 19, 2020

bzbarsky Feb 13, 2020

domenic Feb 20, 2020

bzbarsky Feb 20, 2020

domenic left a comment

domenic Feb 19, 2020

domenic commented Feb 19, 2020 •

edited

domenic commented Feb 19, 2020

TimothyGu commented Feb 19, 2020 •

edited

domenic commented Feb 20, 2020

bzbarsky commented Feb 20, 2020

bzbarsky commented Feb 20, 2020

domenic commented Feb 20, 2020

TimothyGu left a comment

bzbarsky commented Mar 9, 2020

bzbarsky left a comment

bzbarsky Mar 10, 2020

domenic Mar 12, 2020

bzbarsky Mar 12, 2020

bzbarsky Mar 10, 2020

domenic Mar 12, 2020

domenic Mar 12, 2020

bzbarsky Mar 12, 2020

bzbarsky Mar 12, 2020

bzbarsky commented Mar 10, 2020

bzbarsky commented Mar 11, 2020

domenic commented Mar 12, 2020

bzbarsky left a comment

TimothyGu Mar 15, 2020

bzbarsky Mar 15, 2020

annevk commented Dec 18, 2020 •

edited

rakuco commented Oct 18, 2021

Introduce the observable array type (proxy-based) #840

Introduce the observable array type (proxy-based) #840

Conversation

domenic commented Feb 3, 2020 • edited by pr-preview bot

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Feb 19, 2020 • edited

domenic commented Feb 19, 2020

TimothyGu commented Feb 19, 2020 • edited

domenic commented Feb 20, 2020

bzbarsky commented Feb 20, 2020

bzbarsky commented Feb 20, 2020

domenic commented Feb 20, 2020

TimothyGu left a comment

Choose a reason for hiding this comment

bzbarsky commented Mar 9, 2020

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Mar 10, 2020

bzbarsky commented Mar 11, 2020

domenic commented Mar 12, 2020

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Dec 18, 2020 • edited

rakuco commented Oct 18, 2021

domenic commented Feb 3, 2020 •

edited by pr-preview bot

domenic commented Feb 19, 2020 •

edited

TimothyGu commented Feb 19, 2020 •

edited

annevk commented Dec 18, 2020 •

edited