Define "implements" checks using internal slots #595

littledan · 2018-12-09T17:12:16Z

Define an imperative algorithm for instantiating a platform object,
which everything theoretically calls into, and which sets the primary
interface into a new internal slot [[PrimaryInterface]].
Define the "implements" algorithm as checking the [[PrimaryInterface]]
internal slot.

The algorithm here is roughly in correspondence with the V8 implementation,
where operation functions are allocated with a reference to a
FunctionTemplate object (which is in corresondence with WebIDL interfaces).
When checking the receiver of a method that came from WebIDL, the original
prototype chain is traversed by looking at the FunctionTemplate's parent.
The same FunctionTemplate is used in multiple JavaScript realms.

This PR procrastinates on defining an imperative algorithm for [Global]
interfaces, which would be a bit more text. It also doesn't define which
global object is associated with platform objects, or how this association
is represented.

Fixes #97

Preview | Diff

- Define an imperative algorithm for instantiating a platform object, which everything theoretically calls into, and which sets the primary interface into a new internal slot [[PrimaryInterface]]. - Define the "implements" algorithm as checking the [[PrimaryInterface]] internal slot. The algorithm here is roughly in correspondence with the V8 implementation, where operation functions are allocated with a reference to a FunctionTemplate object (which is in corresondence with WebIDL interfaces). When checking the receiver of a method that came from WebIDL, the original prototype chain is traversed by looking at the FunctionTemplate's parent. The same FunctionTemplate is used in multiple JavaScript realms. This PR procrastinates on defining an imperative algorithm for [Global] interfaces, which would be a bit more text. It also doesn't define which global object is associated with platform objects, or how this association is represented. Fixes whatwg#97

annevk · 2018-12-10T10:15:13Z

index.bs

+    if |value| [=is a platform object=] and one of the following is true:
+    <ul>
+        <li>|value|.\[[PrimaryInterface]] is |interface|.</li>
+        <li>The set of [=inherited interfaces=] of |value|.\[[PrimaryInterface]] contains |interface|.</li>


Theoretically it'd be better if these inherited interfaces were stored directly on the object, no?

Why's that? I thought it'd be better if the data structures were "normalized"--this makes it more clear to implementers that they don't need to denormalize either.

Because "inherited interfaces" as currently defined is global. To me it seems better to not rely on a global lookup for something like this. Note that inlining this would make this [[PlatformBrand]].

Right, this PR is trying to get at the same thing as you were proposing with [[PlatformBrand]].

I don't see the downside to the implementation or specification strategy to use shared data structures here during the actual check itself. We will have to look up the brand set from these data structures at some point one way or another, just a question of whether we shift the lookup a bit earlier.

One thing we could do is explain what an interface is in terms of internal slots, so it doesn't sound like we're searching through all the interfaces to find the set of inherited interfaces, or "cache" the set of inherited interfaces. But this seems like excessive formalism for no particular purpose--it's pretty clear how to represent these with efficient data structures.

What V8 does, if I understand the code correctly, is that hidden classes have a "constructor" slot, which has a reference to the "function template" used to make the platform object. That same function template is instantiated on each realm, so it's 1:1 with an interface declaration. The function template has a "parent", which is iterated up through during the brand check. The bindings generator creates a "signature" which is just a wrapper around a function template, and passes it to the function constructor, to produce a function which will include the check on the receiver.

I was going to write the spec using the [[PlatformBrand]] internal slot, but when I saw that Chrome implements this in a way that corresponds in an even more straightforward way to existing IDL concepts, I went with that.

To clarify, what I like about this form of specification is that it makes it clear that, even when an object is in a deoptimized hashtable mode, no more than constant space is needed to represent the set of platform brands needed for a particular instance.

How does that avoid the brand being per Realm? It seems the "function template" would have to be scoped to an Agent, no?

I think that's how it works, but I haven't found that particular part of the code.

Yeah, I guess it's cached in GetWrapperImpl isolate-wide (i.e., per agent).

annevk · 2018-12-10T12:05:04Z

If @bzbarsky has the time I'd love for him to have another look, but I think I'm okay with the general design here per the above discussion.

littledan · 2018-12-10T12:25:31Z

Added a note to clarify #97 (comment)

annevk · 2018-12-10T12:28:02Z

FWIW, I hope we can remove that note over time, it now seems to me everyone should use "implements" going forward, referencing this algorithm.

bzbarsky

This still leaves open questions about how [Constructor] and [NamedConstructor] set the right prototype in subclassing scenarios. Right now they delegate to the constructor steps, but don't pass through NewTarget or NewTarget.prototype; the constructor steps presumably invoke the new "create an X object" algorithm, but that does not take the proto to use as an argument....

This is not new, so a followup is probably OK here, except we're going from "totally undefined, use your best judgement" to "defined in a way that is wrong"...

bzbarsky · 2018-12-11T18:03:53Z

index.bs

+</div>
+
+<div class="note">
+    Specifications use various wordings to reference the "[=implements=]" algorithm,


bzbarsky · 2018-12-11T18:05:10Z

index.bs

+
+    1. If |slots| is provided, append \[[PrimaryInterface]] to |slots|.
+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. Let |proto| be the [=interface prototype object=] of |interface|.


So this algorithm doesn't do subclassing. I assume that's because it's meant to be done from specs that really want the default prototype?

Which global's interface prototype object is used here? Seems like the right global should be input to this algorithm.

What do you mean by "do subclassing"? Are there any specs that don't want the default prototype?

My guess is that the current global object would make sense, but there's a lot to test to see if this is solidly web reality...

What do you mean by "do subclassing"?

I mean class Foo extends Event in JS.

Are there any specs that don't want the default prototype?

Well, new Foo in my example does not want the default prototype. What prototype would it get and why, as things stand now?

Oh, of course, I was thinking about interfaces extending other interfaces. I didn't realize that browsers actually used new.target from platform constructors this way; changed the draft spec to reflect that.

bzbarsky · 2018-12-11T18:07:26Z

index.bs

+    1. Let |proto| be the [=interface prototype object=] of |interface|.
+    1. Let |instance| be [$ObjectCreate$](|proto|, |slots|).
+    1. Set |instance|.\[[PrimaryInterface]] to |interface|.
+    1. Let |realm| be the current [=Realm=].


What if there isn't one? Lots of objects get created when there is no current Realm around. As a simple example, documents are (and this matters, because they have unforgeable things on them). Same thing for events, actually.

This really needs to use the Realm of the passed-in global, not the current Realm.

FWIW, I've been hoping that for cases where we create something before we create a realm, we can move those around, at least specification-wise. It seems better to first create a realm and global, and then a document.

Oh, we may have a Realm fine. But it's not going to be current unless either script is running or someone takes steps to make it current.

I can see how this is an important issue, but I am not sure how to address it in this patch. If I add a realm argument which is required for cases where there is no current realm, that won't make everyone pass it.

In previous discussions we wanted to default to the relevant realm, not the current one. And indeed it should be required in some scenarios; it would be a spec bug not to pass it. #135

that won't make everyone pass it.

True, but that way we can work on fixing those places to pass it, or have some default convention for what Realm is used when calling this algorithm in various situations.

It seems like we have two modes where this algorithm will be called:

Passing a new.target value (e.g., from all constructors)

Somehow indicating a realm. I made this PR vaguely refer to the relevant realm of the relevant this value, but I'm not sure how we should formalize this or let folks pass in their own this/realm.

Could we say that, if you don't have a relevant this value, then you should explicitly pass in the new.target value? Specs could use wording like this:

Create a new object implementing the XYZ interface, with new.target of the interface prototype object of the relevant realm of abc.

Or is that too much cryptic boilerplate?

Too much cryptic boilerplate. As I mentioned above, I think the usage should be one of

Create a new X object

or, if there is no this value

Create a new X object in the relevant realm of abc

bzbarsky · 2018-12-11T18:07:49Z

index.bs

+    1. If |slots| is provided, append \[[PrimaryInterface]] to |slots|.
+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. Let |proto| be the [=interface prototype object=] of |interface|.
+    1. Let |instance| be [$ObjectCreate$](|proto|, |slots|).


This probably needs to enter the Realm of the passed-in global or something. At least to the extent that ObjectCreate cares about its Realm.

I guess it would be the operation of getting the interface prototype object that requires the realm. Not sure how to get people to pass in the realm, so i am inclined to stay vague for now (like the old spec was).

bzbarsky · 2018-12-11T18:09:17Z

index.bs

+</div>
+
+<div class="note">
+    Specifications use various wordings to reference the "[=create an object implementing


Specifications may refer to the "[=create an object implementing the interface=]" algorithm in various ways, including "a new |interface| object".

Thanks, adopted

bzbarsky · 2018-12-11T18:10:45Z

index.bs

+    the lifetime of the object.
+
+    Multiple [=interface objects=] associated with different [=Realm/global objects=]
+    will allocate [=platform objects=] which have \[[PrimaryInterface]] internal


Interface objects don't allocate anything, unless there's a [Constructor].... I think this is really trying to say that platform objects associated with different globals can have the same [[PrimaryInterface]]; this doesn't involve interface objects.

Right, fixed.

bzbarsky · 2018-12-11T18:11:51Z

index.bs

-that it implements.  The value of the \[[Prototype]] [=internal slot=]
+that [=implements=] one or more interfaces is the value of the object's
+\[[PrimaryInterface]] internal slot, which is is the most-derived [=interface=]
+that it [=implements=].  The value of the \[[Prototype]] [=internal slot=]


Except when subclassing is involved.... I'm not sure there's much of a point to this sentence about [[Prototype]] now. It's neither normative nor very informative.

Good point, deleted.

annevk · 2018-12-12T06:08:00Z

#540 also has some discussion on subclassing and globals btw.

- The new.target value is used when available (verified in Chromium that usage of new.target is implemented). - When new.target is not available, the relevant Realm of the this value is used. It's not clear how the this value should be passed in; this patch uses some sloppy wording there.

bzbarsky · 2018-12-12T18:14:03Z

index.bs

+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. If |NewTarget| is provided, let |instance| be ? [$OrdinaryCreateFromConstructor$](|newTarget|, "%ObjectPrototype%")
+    1. Otherwise,
+        1. Let |realm| be the [=relevant Realm=] of the |this| value that led to the execution of this algorithm.


This may not exist either. Simple example: the HTML parser. It's creating objects without any |this| value involved, or any script anywhere in sight.

I think |realm| should be a parameter, which callers are allowed to omit if they are in a context that contains such a this value.

bzbarsky · 2018-12-12T18:14:55Z

index.bs

+
+    1. If |slots| is provided, append \[[PrimaryInterface]] to |slots|.
+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. If |NewTarget| is provided, let |instance| be ? [$OrdinaryCreateFromConstructor$](|newTarget|, "%ObjectPrototype%")


I don't think this does the right thing, because not all web platform objects are ordinary objects (due to things like legacygetters and whatnot), but OrdinaryCreateFromConstructor explicitly creates an ordinary object. Not sure how best to deal with that.

Also, this does not match existing browser behavior, which I think is the right behavior, for the cases when a prototype cannot be derived from newTarget. This would create the object with proto set to %ObjectPrototype%, but we want to create it with proto set to the default prototype for the given interface in the correct Realm (the current Realm, which I believe would in fact always exist when newTarget exists.

As a minor nit, is it newTarget or NewTarget? Should be consistent.

Relatedly, I can never think of a case when a web platform specification would provide a newTarget value to this algorithm. I guess from the definition of a Web IDL constructor, but that's rare. Can we create a wrapper that does not allow passing new.target?

That makes sense. What I saw was some text in HTML to explain constructors which say that they create a new instance of the interface. I like your idea that we could just implicitly take in new.target in this particular case; I will think about what wording will make sense.

Hmm yeah it's true that currently most web platform spec constructors are done "return override" style, i.e. they have a step that says "create a new X object" which Web IDL calls as part of https://heycam.github.io/webidl/#create-an-interface-object. Maybe we need to rethink that. I can see either "create a new instance of the interface being constructed" (which will do the right new.target thing for you) or making it implicit, by e.g. Web IDL allocating "this" for you before calling the constructor steps.

I think we should figure out which direction we want before landing this, but switching over specs to use it can be done as part of the larger project to get specs to use these new dfns, so it's OK if we create something novel that doesn't mesh with existing practice. As long as we're aligned on the right direction.

Those directions both seem plausible. The second (WebIDL creates the instance) feels simpler and more JS-like at first glance. The only downside I can think of is that it doesn't map to current spec text very clearly.

Agreed. Since we'd need to change things anyway, I'd be happy to go with the second.

bzbarsky · 2018-12-12T18:19:06Z

index.bs

+    1. Otherwise,
+        1. Let |realm| be the [=relevant Realm=] of the |this| value that led to the execution of this algorithm.
+        1. Let |proto| be the [=interface prototype object=] of |interface| in |realm|.
+        1. Let |instance| be [$ObjectCreate$](|proto|, |slots|).


Again, this always creates ordinary objects...

The ES spec really doesn't have great hooks for creating exotic objects, does it? :(

domenic

I think this could be finished without much extra work; in particular there was talk of blocking this on defining this but I don't think that's necessary. It'd be really exciting to see this land and get this aspect of object creation and branding fully defined.

index.bs

domenic · 2018-12-21T21:35:03Z

index.bs

+
+    1. If |slots| is provided, append \[[PrimaryInterface]] to |slots|.
+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. If |NewTarget| is provided, let |instance| be ? [$OrdinaryCreateFromConstructor$](|newTarget|, "%ObjectPrototype%")


Relatedly, I can never think of a case when a web platform specification would provide a newTarget value to this algorithm. I guess from the definition of a Web IDL constructor, but that's rare. Can we create a wrapper that does not allow passing new.target?

domenic · 2018-12-21T21:35:50Z

index.bs

+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. If |NewTarget| is provided, let |instance| be ? [$OrdinaryCreateFromConstructor$](|newTarget|, "%ObjectPrototype%")
+    1. Otherwise,
+        1. Let |realm| be the [=relevant Realm=] of the |this| value that led to the execution of this algorithm.


I think |realm| should be a parameter, which callers are allowed to omit if they are in a context that contains such a this value.

domenic · 2018-12-21T21:36:04Z

index.bs

+    1. Otherwise, let |slots| be &laquo; \[[PrimaryInterface]] &raquo;.
+    1. If |NewTarget| is provided, let |instance| be ? [$OrdinaryCreateFromConstructor$](|newTarget|, "%ObjectPrototype%")
+    1. Otherwise,
+        1. Let |realm| be the [=relevant Realm=] of the |this| value that led to the execution of this algorithm.


We seem to use <emu-val>this</emu-val> value through the rest of the spec.

domenic · 2018-12-21T21:38:17Z

index.bs

+    1. Let |proto| be the [=interface prototype object=] of |interface|.
+    1. Let |instance| be [$ObjectCreate$](|proto|, |slots|).
+    1. Set |instance|.\[[PrimaryInterface]] to |interface|.
+    1. Let |realm| be the current [=Realm=].


Too much cryptic boilerplate. As I mentioned above, I think the usage should be one of

Create a new X object

or, if there is no this value

Create a new X object in the relevant realm of abc

domenic · 2018-12-21T21:38:34Z

index.bs

+        1. Let |proto| be the [=interface prototype object=] of |interface| in |realm|.
+        1. Let |instance| be [$ObjectCreate$](|proto|, |slots|).
+    1. Set |instance|.\[[PrimaryInterface]] to |interface|.
+    1. Let |realm| be the current [=Realm=].


The mismatch where you use relevant realm of this above, and current realm here, seems bad.

Thanks, will fix.

domenic · 2018-12-21T21:41:36Z

index.bs

 Every [=platform object=] is associated with a global environment, just
 as the [=initial objects=] are.
 It is the responsibility of specifications using Web IDL to state
 which global environment (or, by proxy, which global object) each platform
 object is associated with.

+<div algorithm>
+    To <dfn>create an object implementing the interface</dfn> |interface|, with optional
+    intenal slots |slots| and |new.target| value |newTarget|, for an interface which is not


I would omit allowing passing internal slots until or unless we figure out a story for #258. It'd be bad if we had a few specifications start using "create a new X with slots [[Y]], [[Z]], [[W]]" and then later told them all to transition to putting their slot declarations in their IDL blocks, or similar.

Co-Authored-By: littledan <[email protected]>

domenic · 2019-01-30T20:22:39Z

@littledan or @Ms2ger, do you think you'll be able to push this over the finish line?

Ms2ger · 2019-01-31T07:55:07Z

Yeah, it's on my todo list.

littledan · 2019-02-07T22:14:14Z

Subsumed by various patches by @Ms2ger , including #635

annevk reviewed Dec 10, 2018

View reviewed changes

Editorial: Clarify other wordings to reference "implements"

cc74839

bzbarsky requested changes Dec 11, 2018

View reviewed changes

littledan force-pushed the platform-brand branch from fbd5df3 to 568d311 Compare December 12, 2018 16:58

littledan added 2 commits December 12, 2018 18:00

Editorial: Remove/clean up some informative text

dceb267

littledan force-pushed the platform-brand branch from 568d311 to dceb267 Compare December 12, 2018 17:00

bzbarsky reviewed Dec 12, 2018

View reviewed changes

littledan mentioned this pull request Dec 12, 2018

Specify that returned Promises are created in the relevant realm #598

Closed

domenic requested changes Dec 21, 2018

View reviewed changes

domenic and others added 2 commits December 21, 2018 22:59

Clean up create definition

8e57f76

Co-Authored-By: littledan <[email protected]>

Update index.bs

15f81cd

Co-Authored-By: littledan <[email protected]>

Ms2ger mentioned this pull request Jan 31, 2019

Add an algorithm to create a platform object. #635

Merged

littledan closed this Feb 7, 2019

Ms2ger mentioned this pull request Feb 25, 2019

Define "implements" checks using internal slots #654

Merged

Define "implements" checks using internal slots #595

Define "implements" checks using internal slots #595

Conversation

littledan commented Dec 9, 2018 • edited by pr-preview bot Loading

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

littledan Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Dec 10, 2018

littledan commented Dec 10, 2018

annevk commented Dec 10, 2018

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

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

littledan Dec 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

annevk commented Dec 12, 2018

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

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

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 commented Jan 30, 2019

Ms2ger commented Jan 31, 2019

littledan commented Feb 7, 2019

littledan commented Dec 9, 2018 •

edited by pr-preview bot

Loading

littledan Dec 10, 2018 •

edited

Loading

littledan Dec 12, 2018 •

edited

Loading