Skip to content
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

ObservableArray types in namespace #1000

Closed
EdgarChen opened this issue Jul 28, 2021 · 21 comments · Fixed by #1001
Closed

ObservableArray types in namespace #1000

EdgarChen opened this issue Jul 28, 2021 · 21 comments · Fixed by #1001
Assignees

Comments

@EdgarChen
Copy link
Member

per https://heycam.github.io/webidl/#idl-observable-array,

Observable array types must only be used as the type of regular attributes.

and readonly regular attributes can be a namespace member, so it seems

namespace A {
  readonly attribute ObservableArray<boolean> foo;
};

is allowed.

But in https://heycam.github.io/webidl/#dfn-attribute-getter, we check if the target is interface

  1. if target is an interface, and attribute is a regular attribute:
    ....
    4. If attribute’s type is an observable array type, then return esValue’s backing observable array exotic object for attribute.

which makes ObservableArray type in namespace doesn't work.

cc @domenic

@domenic
Copy link
Member

domenic commented Jul 28, 2021

This should be disallowed until we have a use case I think. I'm happy to work on a patch tomorrow-ish.

@bathos
Copy link
Contributor

bathos commented Jul 28, 2021

@EdgarChen there's also an assert in the algorithm to get the backing list of a platform object that suggests the intention was to forbid it:

Assert: obj implements an interface with the regular attribute attribute.

(feeling self-conscious realizing this was my fifth comment in a row across different repos following one of yours @domenic - I swear it's just chance, I'm not deliberately looking for em :)

@EdgarChen EdgarChen self-assigned this Jul 29, 2021
@EdgarChen
Copy link
Member Author

I could provide a PR for this. Thanks!

@tabatkins
Copy link
Contributor

I'm looking into copying some of the spec text for OAs for use in maplike/setlike (to solve #254) and ran into this question - OAs are excluded from a bunch of places that maplike/setlike interfaces are not, and it would be really awkward to add the restriction, but it's really strange to have them be treated differently.

I'm thinking this issue was resolved the wrong way around - it was originally raised just because the OA text was contradictory due to being overly specific in one part, and it got resolved by applying the more-specific condition everywhere. An OA is just... a Proxy object, tho. There's nothing about it that should actually make its use in any of these places problematic.

Would y'all mind if I PR'd to reverse this and do it the other way around instead, allowing OAs in all the places any other interface object is allowed?

@domenic
Copy link
Member

domenic commented Apr 29, 2022

I think it'd be best to stay conservative, even if it makes things less consistent... if you want to make things more consistent by disallowing maplikes/setlikes on namespaces though, that seems reasonable?

@tabatkins
Copy link
Contributor

If it actually cost impl effort I can potentially agree, but I don't see how that's the case. (The restriction doesn't cost impl effort either, except in their internal binding generator that parses IDL, but it costs spec effort to remember these odd inconsistencies, and results in oddly-designed APIs when someone blindly designs around the restriction rather than revisiting the original question.)

And looking into this more, it actually seems like the OP accidentally discovered a different error actually - the attribute getter algo runs some steps if called specifically on an interface and with a regular attribute, but those steps should apply to namespaces as well, which can also have (readonly) regular attributes. As currently written, it looks like namespace getters would bypass the security check, for example. Fixing that would have resolved the OP's contradiction as well.

Am I reading this wrong?

@domenic
Copy link
Member

domenic commented Apr 29, 2022

I feel like we're talking past each other. We're trying to prevent the spread of unusual patterns in the web spec ecosystem, such as a namespace (supposed to be a collection of functions, maybe with static data) with an observable array property. We should not be loosening those restrictions; if anything we should be tightening them.

@tabatkins
Copy link
Contributor

Ah, that's a reasonable restriction, tho not enforced generally (any other interface value is still allowed as the type of a namespace property). I still think that might mean we have a bug in this algo, which I'll raise as a separate issue.

Putting aside the specific question of OAs in namespaces, I was actually brought to this issue by the commit immediately following the one in #1001 - a352edb - which prevents OAs from being nullable or put into another OA. I couldn't find an issue explaining why this change was made, unfortunately. @EdgarChen, do you remember why you addeed this restriction?

@bathos
Copy link
Contributor

bathos commented Apr 29, 2022

prevents OAs from being nullable

A nullable OA meant you’d hit undefined behavior in the the getter/setter steps for OA attributes (which would seemingly need to do something novel with the backing list?). Unless there were motivations besides fixing the bug, I suspect specifying the missing behavior would permit lifting the constraint.

or put into another OA

It seems like there’s a whole lot that prevents OA<OA> from being coherent currently. Instances are 1:1 with platform objects, allegedly created during define the attributes (but see below*). The getter/setter behaviors of the attributes depend on the backing list / “backing observable array exotic object” (@_@) associations and that accessor behavior would seemingly need to become the outer OA’s own behavior for index-key properties.

* Step 8 in define the attributes creates and “installs” the “backing observable array exotic object”. But when this algorithm runs with the OA-typed regular attributes, the target ... is a prototype! Currently, each prototype apparently gets a unique OA and instances get nuthin.

@domenic
Copy link
Member

domenic commented Apr 29, 2022

Currently, each prototype apparently gets a unique OA and instances get nuthin.

Well, oops. I think the correct fix is to move (the equivalent of) step 8 into https://webidl.spec.whatwg.org/#internally-create-a-new-object-implementing-the-interface so that ES value instances get the BOAEO.

@EdgarChen
Copy link
Member Author

prevents OAs from being nullable

A nullable OA meant you’d hit undefined behavior in the the getter/setter steps for OA attributes (which would seemingly need to do something novel with the backing list?).

The attribute setter steps covers the new value, V, to sequence<T>, see step 4.5.10.1 of https://webidl.spec.whatwg.org/#dfn-attribute-setter. If a null is passed, https://webidl.spec.whatwg.org/#es-sequence throws a TypeError.

or put into another OA

It seems like there’s a whole lot that prevents OA from being coherent currently. Instances are 1:1 with platform objects, allegedly created during define the attributes (but see below*). The getter/setter behaviors of the attributes depend on the backing list / “backing observable array exotic object” (@_@) associations and that accessor behavior would seemingly need to become the outer OA’s own behavior for index-key properties.

Yeah, BOAEO is created 1:1 with OA attribute, what should accessing the inner OA of OA behave is undefined.

@tabatkins
Copy link
Contributor

Hmmmm, if an OA can't hold another OA, that suggests there is something deeply wrong with how OAs are specified. It's not like they can only hold plain JS values; they can already hold arbitrarily-complex interface objects. I'll investigate this when I'm done with my current train of thought.

@bathos
Copy link
Contributor

bathos commented May 3, 2022

I'll investigate this when I'm done with my current train of thought.

😍 Excited to hear more about what you find. I find several things about OA behaviors and their underlying model pretty surprising. In particular I have some thoughts to share about the way they currently implement indexed properties with custom accessor semantics (like a NodeList) instead of with custom own data property semantics (like an Array).

Perhaps the rest of this will be useful input for your investigation at least as an angle to consider. Given it’s a departure from the original topic, I’m nestling it in a <details>
let sheet = new CSSStyleSheet();
sheet.replaceSync("* { background-color: hotpink; }");
({ __proto__: document.adoptedStyleSheets })[0] = sheet

That defines a "0" property on the prototype, not the receiver. To fix this, the [[Set]] steps should be checking if receiver === proxy and only moving into the “like an accessor, but only when out-of-bounds*” %TypedArray%-like behavior in that case specifically. But better yet, kill [[Set]] altogether! Array exotic objects themselves don’t override it for a reason: they exclusively use own-property semantics, not accessor semantics. Accessors are what [[Get]] and [[Set]] “explain”, so unless that special out-of-bounds behavior is really important, [[DefineOwnProperty]] is normally sufficient and even [[GetOwnProperty]] can usually be left alone in cases like this.

(It’s unfortunate that the two most specialized traps have the two most attractive names while the real generic ones have the most arcane names. “What’s a receiver? shrug” seems to be the most common mistake in Proxies out in the wild.)

* The out-of-bounds index scenario is the only scenario where it doesn’t just end up behaving the same as if there were no custom [[Set]], provided the missing receiver check is fixed. TypedArrays had very good reasons to introduce this behavior despite its novelty, but it’s not clear to me why an OA merits it given the complexity it adds, especially if the premise is to be more like a “normal” array.

I forget where now but I recall finding prior discussion of this where Domenic had a suspicion that this was true, that customizing these OIMs was unnecessary. He was right! However folks subsequently concluded that [[Get]] and [[Set]] were necessary. This seemed to stem from the expectation that because proxies without custom [[Set]] delegate to target.[[Set]](P, V, Receiver), the custom [[DOP]] would be “skipped” if it did not have a custom implementation. This is untrue because Receiver there can never be the target object if the target object isn’t exposed anywhere — it can only be an object external code can access and is almost always the proxy. It’s Receiver.[[DOP]] that gets called in OrdinarySet if no unwritable / accessor properties are hit on the way down, and likewise it’s the receiver’s [[GOP]] in OrdinaryGet. The receiver is the value having a property ... gotten or uhh sotten, not the object implementing these methods (unless they’re one and the same, but if own-properties are the expected case data property semantics are the norm, just like for AEOs).

A user-controlled custom “typed” array only requires [[DOP]], like AEOs themselves, while a “controlled” typed array that intercepts (& possibly rejects) all mutations but preserves its own right to mutate at will additionally requires [[Delete]], [[PreventExtensions]], and [[SetPrototypeOf]] (SetImmutablePrototype style) ... at least if the target object is leveraged for its usual role in the proxy model. That’s the happy path for proxies, but here it gets bypassed for reasons I haven’t been able to work out. OAs implement custom [[HasProperty]], [[Get]], [[GetOwnProperty]], [[Set]], and [[OwnKeys]], but OrdinaryX semantics for all of these would provide sound behavior for free if the forever-configurable properties were defined on the target object.

It’s super possible that I’m ignorant of / mistaken about key details & motivating design considerations that would explain the choice of accessor semantics & the reimplementation of behaviors the target object would provide out of the box if it weren’t being shunned. While I think these things look like mistakes, I appreciate that this is a super difficult thing to spec and that a lot of thought’s already gone into it, so forgive me if I’m way off with this assessment.

@domenic
Copy link
Member

domenic commented May 3, 2022

It's hard for me to load all the context of observable arrays back into my head. However I can give some broad stroke responses:

  • In general I find Array's data-property-like semantics bizarre and undesirable, even for Array itself. The way in which changing array[numericProp] will also impact array.length feels much more like a setter and the way it's defined by just shoving the setter behavior into [[DefineOwnProperty]] seems like bad language layering. I haven't done a deep analysis of the alternatives but this is just a gut feeling that guided my design.
  • This is especially true for something as dynamic as observable arrays which have a backing store they consult (i.e., a getter that feels nontrivial) and which run arbitrary spec code and type conversion on sets (i.e., a setter that is definitely nontrivial).
  • The special out-of-bounds behavior seems pretty important to me.
  • It's very possible we've made a mistake in some design of the hooks, e.g. by not checking receivers correctly. Any test cases and spec fixes to accompany them would probably be welcome.
  • However if the motivating "use case" is assigning to ({ __proto__: document.adoptedStyleSheets })[0] then I'm not sure how much I care. I'd hope for a more realistic example of something a web developer might actually write that could cause unexpected behavior.

@domenic
Copy link
Member

domenic commented May 3, 2022

Regarding the specific question as to why the backing list is the source of truth instead of the wrapped Array object itself, it's because we don't want spec authors (or C++ code authors) to have to mess with with raw ES Array objects. There is some discussion of this (if you squint) in the initial few comments of the original PR: #840

@tabatkins
Copy link
Contributor

Yeah I think the surface-level design of OAs is right, at least at first blush. There's just something wrong with the details of the guts, if something is preventing an OA from holding another OA. The details of the backing list and getting/setting from it should be tied to the OA object, not to the interface that happens to have an OA attribute. If nesting is illegal it implies that OAs can't live by themselves as, say, something that a method can return (in particular, it implies that you can't make an interface that extends from OA).

But I haven't dug into these details yet so I'm not sure what, if anything, is actually problematic, or if it's just something that needs a quick tweak to fix an accidental error in one of the algos.

@bathos
Copy link
Contributor

bathos commented May 3, 2022

However if the motivating "use case" is assigning to ({ proto: document.adoptedStyleSheets })[0]

It’s not — it’s a hiccup that tends to show up when [[Set]] is used as a hook for extending own-data-property-related behaviors because it’s easy to miss that a receiver test is needed to keep it from becoming spooky. It happened to TypedArray too. It’s fixable with a tiny change & no alterations to the intended behavior, so I’d be surprised if it were controversial to do so.

@domenic
Copy link
Member

domenic commented May 3, 2022

What is the motivating use case for the proposed change then?

@bathos
Copy link
Contributor

bathos commented May 3, 2022

What is the motivating use case for the proposed change then?

My previous comment only regarded the quoted bit. The change in question would be the addition of something like “and the receiver is the proxy” after “is an array index”. If this was already apparent though, I’m not sure I can provide an answer that you’d be satisfied with — a [[Set]] algorithm that mutates the wrong argument seems self-evidently like a bug to me. In typical usage, the wrong argument happens to have the same value as the right one, it’s true ... but bug fixes usually aren’t gated on use cases? Maybe I’ve misunderstood something about what you’re asking? (Or offended? Really hope not — I very much respect the work that’s been done by you & everybody else here.)

@domenic
Copy link
Member

domenic commented May 3, 2022

I just don't understand why it's a bug when what it's "fixing" is weird esoteric code like that, where I at least have no expectations of what correct vs. incorrect behavior is. I was hoping for some example of reasonable code developers might write that shows how your proposed behavior is more-expected than the current spec's behavior.

(To be clear, no offense!)

@bathos
Copy link
Contributor

bathos commented May 4, 2022

I think a.b = c defining a "b" property on a would be more-expected than if it defined a new "b" property on d, an object somewhere in a’s prototype chain, especially if they report themselves as data properties.

I have no reason to think folks are going to be invoking AO’s [[Set]] with receivers other than the AOs themselves (other than me, apparently) but didn’t think this would bear on which argument is the assignee (usually literally the LHS of an assignment expression).

I was hoping for some example of reasonable code developers might write

Hear this a lot yeah 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants