-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce the observable array type (proxy-based) #840
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
* <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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
|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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
|
||
* <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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.) |
Since we are defining the object in terms of Proxy, my assumption is that redundant hooks are necessary. If we don’t define a (Hence, the question about whether Integer-Indexed Exotic Objects need those overridden internal methods, is different from the question we have here.) |
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:
|
Well.. HTMLOptionsCollection does maintain a mirror, in the form of the
Yup. The current rigor of the CSS specs is awful.
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. |
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.
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:
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. |
Yep, that's the current diff (modulo bugs) ^_^. And yeah, I agree with the benefits you mention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't spot any errors in the proxy traps themselves, but some editorial comments are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
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|). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought of a super-compelling reason so far, unfortunately.
index.bs
Outdated
|
||
1. Set |newLen| to [=?=] [$ToUint32$](|newLen|). | ||
1. Let |numberLen| be [=?=] [$ToNumber$](|newLen|). | ||
1. If |newLen| ≠ |numberLen|, then throw a {{RangeError}} exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is modeled on https://tc39.es/ecma262/#sec-arraysetlength, but I messed up by overwriting newLen, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
And my apologies for the terrible lag here. :( |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks great!
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Were bugs filed for this change? (Edit: filed https://bugzilla.mozilla.org/show_bug.cgi?id=1683281 on Fx.) |
FTR, the corresponding Chromium bug is https://crbug.com/1201744. I couldn't find anything on bugs.webkit.org |
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