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

Designing mixins #363

Closed
domenic opened this issue May 15, 2017 · 28 comments
Closed

Designing mixins #363

domenic opened this issue May 15, 2017 · 28 comments

Comments

@domenic
Copy link
Member

domenic commented May 15, 2017

We've talked a lot about mixins, but I don't think we've written down our plans in one central location...

The basic idea is to stop using [NoInterfaceObject] interfaces + implements to do mixins, and give them dedicated syntax. Along the way we can drastically simplify the model; currently, since interfaces are complicated, mixins are complicated. This leads to situations like the one in the example in @tobie's latest pull request, where you have mixins that inherit from things that have mixins and so on.

We will either eliminate or repurpose implements. The only thing you can mix in (either via the implements keyword, or via a new one like mixes that takes its place) its mixins; you cannot mix in interfaces.

Once we have a clear way to talk about mixins, and a simpler model, we should be able to fix the following bugs that largely derive from confusion about how mixins interact with the rest of the system:


Concretely, the biggest simplifications we have lined up are:

  • Making it clear that mixins do not create their own interface/interface prototype object, but instead cause the appropriate members to be added to the interface/interface prototype objects of the interface they are mixed in to
  • Saying that mixins are not allowed to inherit from anything (this makes no sense as they have no proper JS prototype chain)
  • Saying mixins cannot be mixed in to, maybe? I don't think anything does this today, but we should check. It'd make things simpler to not have to recurse and flatten the mixin chain all the time.
@annevk
Copy link
Member

annevk commented May 16, 2017

Saying mixins cannot be mixed in to, maybe?

Can you do partial mixins then? You need to be able to extend WindowOrWorkerGlobalScope and such somehow.

@domenic
Copy link
Member Author

domenic commented May 16, 2017

Yes, good point, you want partial mixins. But hopefully you are not mixing the same stuff into two different mixins, so for mixins at least I hope we can say partial mixins is the extensibility avenue.

@tobie
Copy link
Collaborator

tobie commented May 16, 2017

Yeah, I feel like partials is a well understood and clearly scoped solution to the need to split things up across multiple specs. Mixing mixins into each other, on the other hand, complexifies a bunch of things.

@tobie
Copy link
Collaborator

tobie commented Aug 29, 2017

Some feedback from researching over the gecko code base:

  • mixins do not need to support inheritance.
  • mixins will probably need to support constants.
  • mixins need to support "stringifier".
  • mixins do not need to support "legacycaller", "setter", "getter", "deleter".
  • mixins neither need to support static operations nor static attributes.
  • mixins do not need to support, iterable, setlike, or maplike.

@tobie
Copy link
Collaborator

tobie commented Aug 31, 2017

table

@bzbarsky
Copy link
Collaborator

It makes sense for mixins to not support things that change the "sort" of object you're dealing with (legacycaller, getter, setter, deleter, iterable, setlike, maplike). But supporting static bits probably does makes sense.

@tobie
Copy link
Collaborator

tobie commented Aug 31, 2017

But supporting static bits probably does makes sense.

I don't disagree, but given no current mixin actually declares any, I thought it was easier to leave it out for now and add it later if necessary.

@annevk
Copy link
Member

annevk commented Aug 31, 2017

Would we ever add static bits to multiple objects though? Since static operations/attributes don't have context, it seems you'd almost always use partial for them.

@bzbarsky
Copy link
Collaborator

That's a good question. I guess there's not much call for adding the same statics to multiple things at once...

@tobie
Copy link
Collaborator

tobie commented Sep 1, 2017

As I mentioned on IRC, I feel like the mental model I'm using for mixins is that they're named interface partials that can extend more than one interface with a more limited set of members.

When a partial interface doesn't have a [Exposed] extended attribute, its exposure set is "the exposure set of the original interface or namespace definition." When it does, the partial interface's exposure set must be a subset of the interface's exposure set.

I feel like this is the behavior that we want for mixins (if my mental model is the right one).

But while this works well with partials, it completely breaks down with mixins as @bzbarsky points out in #423 (comment), as that implies that a mixin and its members would have multiple exposure sets.

(As a side note, this might also make the transition from [NoInterfaceObject] to mixins not as straightforward in some cases.)

So if we want to go with this solution, it sounds like we need to make members' exposure sets function of the interface they're specified or included on, and thus no longer constant.

Does that seem reasonable?

@tobie tobie mentioned this issue Sep 1, 2017
7 tasks
@domenic
Copy link
Member Author

domenic commented Sep 2, 2017

I'm not sure how related the following is to your question, @tobie, but it seems somewhat related. I came here to state it after doing a skim of #433.

In general I think everything would be greatly simplified if we could just treat mixins as some kind of "macro" that "ahead of time" modifies the given interface. That is, instead of constantly talking about the mixin and the places it's mixed in to, or talking about an interface's members and its mixins' members, we could just talk about the interface's members, and by definition that will also include those of any mixins.

Do you think that direction is feasible to go in? I see some changes in #433 that just go from "A and A's consequential interfaces" to "A and A's mixins" and that seems like a bit of a shame.

I guess this does impact your question, because in this framework, you don't have to worry about the mixin's members having different exposure sets depending on where they're mixed in to. Because there are no members of the mixin; there are just N separate "copies" of those members, living inside each of the interfaces they're mixed in to. That seems nice.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Sep 2, 2017

That seems like a reasonable approach. The one drawback with it is that if you have an entire mixin that should all have on exposure set, which doesn't match the exposure set of the whole interface, you have to annotate each thing in the mixin individually.

@tobie
Copy link
Collaborator

tobie commented Sep 2, 2017

Thinking out loud, here, but what if instead of adding mixing we modified partial interfaces so they could do:

partial interface Foo, Bar, Baz { };

to mean roughly the same as:

mixin M { };
Foo includes M;
Bar includes M;
Baz includes M;

We'd lose the indirection provided by the ability to add members to the mixin itself, but would that really be an issue?

Either way, it seems that what we want is something conceptually close to partial interfaces.

@domenic
Copy link
Member Author

domenic commented Sep 2, 2017

That would require centralizing the partial interface definition.

I think we should separate the concept from the syntax/usage. The syntax for mixins seems good. It's just a refined version of the syntax we're already using, with mixin replacing [NIO] interface and includes replacing implements. But we should indeed revise the underlying system to be more like partial interfaces.

@annevk
Copy link
Member

annevk commented Sep 2, 2017

The one drawback with it is that if you have an entire mixin that should all have on exposure set, which doesn't match the exposure set of the whole interface, you have to annotate each thing in the mixin individually.

This only ever matters if the mixin is exposed on a subset of the interface, right? I'm not sure if that is common, but if it is we might want to allow [Exposed] on mixins as a shorthand for annotating all individual members of the mixin.

(And yeah, I don't think we want centralized definitions. Pretty sure we use all flexibility provided by the current system.)

@tobie
Copy link
Collaborator

tobie commented Sep 2, 2017

@domenic wrote:

That would require centralizing the partial interface definition.

Well, not necessarily. You could have multiple partial interfaces targeting the same set of interfaces.

I think we should separate the concept from the syntax/usage. The syntax for mixins seems good. It's just a refined version of the syntax we're already using, with mixin replacing [NIO] interface and includes replacing implements. But we should indeed revise the underlying system to be more like partial interfaces.

But yes, I agree with the second part of your comment. The key is to shift the thinking into considering mixins as spec editing devices just like partials and not language constructs such as interfaces.

@tobie
Copy link
Collaborator

tobie commented Sep 2, 2017

@annevk wrote:

(And yeah, I don't think we want centralized definitions. Pretty sure we use all flexibility provided by the current system.)

Well, they wouldn't need to be centralized, they just wouldn't have their own identifier. So instead of having mixin WindowOrWorkerGlobalScope { }; in HTML and then having multiple specs do:

partial mixin WindowOrWorkerGlobalScope { };

You wouldn't have the WindowOrWorkerGlobalScope mixin at all and would just have multiple specs doing:

partial interface Window, WorkerGlobalScope { };

It's conceptually simpler than learning a new IDL construct, and makes things a lot more explicit. But it implies a bit more churn to move to and you loose the ability to suddenly expend WindowOrWorkerGlobalScope to also be included e.g. in Worklets.

@tobie
Copy link
Collaborator

tobie commented Sep 2, 2017

@annevk wrote:

@bzbarsky wrote:

The one drawback with it is that if you have an entire mixin that should all have on exposure set, which doesn't match the exposure set of the whole interface, you have to annotate each thing in the mixin individually.
This only ever matters if the mixin is exposed on a subset of the interface, right? I'm not sure if that is common, but if it is we might want to allow [Exposed] on mixins as a shorthand for annotating all individual members of the mixin.

Yes, the idea was not to prevent mixins from being annontated with [Exposed]. It was to make the default behavior of missing an [Exposed] annotation the same as it is for partial interfaces. That is, to have mixin members exposed the same way as the (non-annotated) interface members of the interface the mixin was included in. Similarly, annotating a mixin with the [Exposed] extended attribute would be shorthand for annotating all of its members. Again, just like it works for partial interfaces.

@domenic
Copy link
Member Author

domenic commented Sep 2, 2017

you loose the ability to suddenly expend WindowOrWorkerGlobalScope to also be included e.g. in Worklets.

Right. You centralize control over who mixes-in the mixin with the mixin's author, taking it away from the consumers. This is not great; we want to be able to define mixins in one place and have other specs use them.

Looking through your gist that flexibility seems to be used rarely; most mixins are used within the same spec. But it seems important to me...

@tobie
Copy link
Collaborator

tobie commented Sep 2, 2017

Right. You centralize control over who mixes-in the mixin with the mixin's author, taking it away from the consumers. This is not great; we want to be able to define mixins in one place and have other specs use them.

My understanding from previous conversations was that we used implements statements (instead of interface Foo { include M; }; precisely because the common scenario required control at the mixin author level rather than at the consumer level.

In #195 we need to add dictionary mixins. We already need interface mixin partials. We'll probably end up with dictionary mixin partials too. All in all we'll be end up with the following extension mechanisms in the near future:

interface mixin M1 { };
partial interface mixin M1 { };
partial interface I { };
interface I { };
interface J { };
I includes M1;
J includes M1;

dictionary mixin DM { };
partial dictionary D { };
dictionary D { };
dictionary E { };
D includes DM;
E includes DM;

Where we could have:

partial interface I, J { };
partial interface I, J { }; // again, no need for partial mixins
partial interface I { };
interface I { };
interface J { };

partial dictionary D, E { };
dictionary D { };
dictionary E { };

Which seems a much lighter cognitive load. Of course both are tradeoffs.

I just want to make sure we're choosing the right tradeoffs. (I'm just exposing those tradeoffs, btw, not pushing towards a specific direction myself.)

@annevk
Copy link
Member

annevk commented Sep 2, 2017

I guess it would be good to study precedent. Perhaps both is what we want.

I do think there's value to having a mixin named DocumentOrShadowRoot and being able to extend that. It conveys the concept of shared functionality between Document and ShadowRoot much more clearly than a bunch of partial interface Document, ShadowRoot would. The latter seems more accidental somehow. Similar for other mixins in DOM, such as ParentNode.

@tobie
Copy link
Collaborator

tobie commented Sep 25, 2017

Okay, so I let this rest quite a bit. I looked at some of the specs using mixins today, and it seems clear that there's a strong appetite for having mixin constructs in WebIDL.

I'd like to account for #195 (dictionary mixins) right away, so I suggest adding support for the following syntax to the spec:

interface mixin identifier { /* ... */ };
partial interface mixin identifier { /* ... */ };

// we'd only add those when handling #195
dictionary mixin identifier { /* ... */ }; 
partial dictionary mixin identifier { /* ... */ };

The alternative is to do, instead:

mixin identifier { /* ... */ };
partial mixin identifier { /* ... */ };

// we'd only add those when handling #195
dictionary mixin identifier { /* ... */ }; 
partial dictionary mixin identifier { /* ... */ };

which seems less consistent, but simpler to write.

Any preference or alternative suggestions?

@annevk
Copy link
Member

annevk commented Sep 26, 2017

I'd prefer just mixin over "interface mixin". Do we keep using "implements" to couple them to interfaces?

@domenic
Copy link
Member Author

domenic commented Sep 26, 2017

I'm slightly in favor of "interface mixin" for symmetry and because it helps ensure that we're not ambiguous with any future ECMAScript-native concept of mixin.

@tobie
Copy link
Collaborator

tobie commented Sep 26, 2017

I'd prefer just mixin over "interface mixin".

WFM.

Do we keep using "implements" to couple them to interfaces?

I moved to "includes" instead as I thought it would make it easier to figure out what was upgraded and what wasn't.

@tobie
Copy link
Collaborator

tobie commented Sep 26, 2017

I'm slightly in favor of "interface mixin" for symmetry and because it helps ensure that we're not ambiguous with any future ECMAScript-native concept of mixin.

That argument ways a little more than aesthetics, imho.

@annevk
Copy link
Member

annevk commented Sep 27, 2017

I'm fine with that, I mostly argued for mixins since that's how I refer to them in prose.

@tobie
Copy link
Collaborator

tobie commented Sep 27, 2017

I'm fine with that, I mostly argued for mixins since that's how I refer to them in prose.

Defining them as <dfn for=interface>mixin</dfn> should mostly let you do that, no?

tobie added a commit to tobie/webidl that referenced this issue Oct 11, 2017
* Obsolete use of [NoInterfaceObject] extended attribute as mixins.
* Add new interface mixin and partial interface mixin constructs.
* Replace implements statement by includes statement which only accepts
  mixins on its rhs.
* Remove supplemental interface and related concepts altogether.
* Add generic members dfn.
* Add table to clarify which members each construct accepts.
* Refactor default toJSON operation, and [Exposed] and [SecureContext]
  algorithms accordingly. Closes whatwg#118.
* Prevent operation overloading across mixins and interfaces. Closes whatwg#261.

Closes whatwg#363.
Closes whatwg#164.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.
tobie added a commit to tobie/webidl that referenced this issue Oct 11, 2017
* Obsolete use of [NoInterfaceObject] extended attribute as mixins.
* Add new interface mixin and partial interface mixin constructs.
* Replace implements statement by includes statement which only accepts
  mixins on its rhs.
* Remove supplemental interface and related concepts altogether.
* Add generic members dfn.
* Add table to clarify which members each construct accepts.
* Refactor default toJSON operation, and [Exposed] and [SecureContext]
  algorithms accordingly. Closes whatwg#118.
* Prevent operation overloading across mixins and interfaces. Closes whatwg#261.

Closes whatwg#363.
Closes whatwg#164.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.
tobie added a commit to tobie/webidl that referenced this issue Oct 11, 2017
* Obsolete use of [NoInterfaceObject] extended attribute as mixins.
* Add new interface mixin and partial interface mixin constructs.
* Replace implements statement by includes statement which only accepts
  mixins on its rhs.
* Remove supplemental interface and related concepts altogether.
* Add generic members dfn.
* Add table to clarify which members each construct accepts.
* Refactor default toJSON operation, and [Exposed] and [SecureContext]
  algorithms accordingly. Closes whatwg#118.
* Prevent operation overloading across mixins and interfaces. Closes whatwg#261.

Closes whatwg#363.
Closes whatwg#164.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.
tobie added a commit to tobie/webidl that referenced this issue Oct 11, 2017
* Obsolete use of [NoInterfaceObject] extended attribute as mixins.
* Add new interface mixin and partial interface mixin constructs.
* Replace implements statement by includes statement which only accepts
  mixins on its rhs.
* Remove supplemental interface and related concepts altogether.
* Add generic members dfn.
* Add table to clarify which members each construct accepts.
* Refactor default toJSON operation, and [Exposed] and [SecureContext]
  algorithms accordingly. Closes whatwg#118.
* Prevent operation overloading across mixins and interfaces. Closes whatwg#261.

Closes whatwg#363.
Closes whatwg#164.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.
tobie added a commit to tobie/webidl that referenced this issue Oct 11, 2017
* Obsolete use of [NoInterfaceObject] extended attribute as mixins.
* Add new interface mixin and partial interface mixin constructs.
* Replace implements statement by includes statement which only accepts
  mixins on its rhs.
* Remove supplemental interface and related concepts altogether.
* Add generic members dfn.
* Add table to clarify which members each construct accepts.
* Refactor default toJSON operation, and [Exposed] and [SecureContext]
  algorithms accordingly. Closes whatwg#118.
* Prevent operation overloading across mixins and interfaces. Closes whatwg#261.

Closes whatwg#363.
Closes whatwg#164.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.
@tobie tobie closed this as completed in 45e8173 Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants