dart-lang / language

Design of the Dart language
Other
2.61k stars 200 forks source link

Allow mixins in "extends" clauses #1942

Open munificent opened 2 years ago

munificent commented 2 years ago

We would like to deprecate and eventually stop supporting using class declarations as mixins. There are a couple of proposals out for how to do that: #1529 and #1643. The main sticking point for simply making it a warning or error to mix in a class is Flutter.

Flutter has a number of class declarations that are widely used as both mixins (in with clauses) and as superclasses (in extends clauses). Flutter can't change those class declarations to be explicit mixin declarations because it would break every extends use of the classes in the wild today.

We could potentially have a deprecation period where we encourage Flutter users to change all of those extends ___ to extends Object with ___, but it's not clear that that's actually a desirable ecosystem change. Most Dart users seem to be unaware of mixins so forcing them to learn about with before they can use common functionality like ChangeNotifier might be punishing for little benefit.

Here is a simpler proposal:

  1. Allow using extends ___ on mixins and treat it as syntactic sugar for extends Object with ___. Unlike the previous proposals, we don't make this a temporary language feature. Instead, we simply allow that sugar henceforth and forever.

    With this, Flutter classes like ChangeNotifier can immediately be changed to mixin instead of class without breaking or effecting any users.

  2. After Flutter and other key packages have migrated their mixin-in-able classes to use mixin, add a linter rule that warns when users mixin a type declared using class. Eventually promote that to a warning.

  3. In Dart 3.0, remove support for mixing in classes. Continue to allow mixins to appear in extends clauses.

Levi-Lesches commented 2 years ago

Allow using extends ___ on mixins and treat it as syntactic sugar for extends Object with ___.

As someone who took quite a while to understand mixins, I think this will be too confusing for several reasons:

IMO, it might just be simpler to attack the root of the problem: the user is trying to share functionality with a mixin but thinks extends is the right word, when really it's with. So Dart can include that in the error message. Here's the current error message for trying to extend a mixin:

Classes can only extend other classes. Try specifying a different superclass, or removing the extends clause.

How about a similar error when mixing in a class?

Classes cannot extend mixins. Try specifying a class, or mixing-in the mixin using `with` instead of `extends`. 

This way, the compiler explains to the user

As for the timeline, I think that with dart fix, this can probably be one of those changes that gets done all at once in Dart 3.0. Flutter and others convert their classes to mixins, announce support for Dart 3.0, and then dart fix converts extends to with in user code.

munificent commented 2 years ago
  • it implies you can actually extend a mixin, further complicating the user's understanding of mixins

This depends a little on what you mean by "extend". When you have a "with" clause, the mixin does end up as the superclass of the class you're declaring. So, in some sense, mixins are always extended by the subclass. It's just a question of what goes in the superclass's superclass slot.

  • it wouldn't allow mixing in and extending, something that's done especially commonly with ChangeNotifier (class UserModel extends DataModel with ChangeNotifier)

It would. It would allow exactly this. Flutter can change ChangeNotifier to use mixin and then users can use it in both extends and with clauses.

  • It wouldn't allow for mixing in multiple mixins, something that's often done on State (SingleTickerProviderStateMixin and a custom mixin)

It would still allow that by using a with clause as usual. The proposal doesn't remove any syntax. It literally just allows extends Foo where Foo is a mixin to be syntactic sugar for extends Object with Foo.

  • It doesn't show the user the "real" way, using with (although that can be a lint)

I agree that it does obscure the semantics of mixins, which is why I wasn't in favor of this approach initially. But... the reality is that a lot of users just don't understand mixins and a compile error saying "write with instead of extends here" likely won't solve that.

Classes cannot extend mixins. Try specifying a class, or mixing-in the mixin using `with` instead of `extends`. 

This was my initial thinking too. But it does seem cruel to have the compiler say, "I know exactly what you meant to do here, but I'm going to yell at you and make you change your code anyway."

There is a more pragmatic angle to this too. If we allow mixins to appear in extends clauses, then it makes it a non-breaking API change to turn a class declaration into a mixin declaration (assuming there are no other changes needed in the class). That in turn can make mixins more common which will help encourage users to learn about them.

Levi-Lesches commented 2 years ago

My criticisms have to do with the fact that this workaround is meant specifically for users who don't know much about mixins and using extends instinctively, and that allowing both would cause confusion to those just learning the difference.

This depends a little on what you mean by "extend".

I mean the behavior you get when using the extends keyword -- inheriting constructors and being part of a hierarchy, and any other differences between superclasses and mixins.

It would allow exactly this. Flutter can change ChangeNotifier to use mixin and then users can use it in both extends and with clauses.

But the point is new users would be using extends ChangeNotifier, instead of with ChangeNotifier, so they can't extend both ChangeNotifier and DataModel unless they learned to use with.

It would still allow that by using a with clause as usual.

Right, but again, there would be no way for users to know they should be using with if their initial intuition of extends works "properly" (that is, until later).

But... the reality is that a lot of users just don't understand mixins and a compile error saying "write with instead of extends here" likely won't solve that.

Sort of did for me! The mixins that beginners use like ChangeNotifier are designed to be pretty much identical to superclasses in practice, and then when things get more complicated, users will recognize that it's because they're using this new "with" mechanism that they should look into rather than a confused notion of "a magical type of class that can be extended and mixed in".

But it does seem cruel to have the compiler say, "I know exactly what you meant to do here, but I'm going to yell at you and make you change your code anyway."

I guess it would make sense if this were a lint or warning then, because those pretty much cover the lines of "you're probably wrong about this, and you probably meant to do this instead". I proposed an error because I was afraid someone would try using a class as a mixin and it wouldn't work (because it has a constructor or superclass), then they'd wonder why it ever worked before and realize they were doing it wrong. A warning is "your code works but this is likely not what you want", but an error is "you're doing this wrong". Either is fine in this case.

If we allow mixins to appear in extends clauses, then it makes it a non-breaking API change to turn a class declaration into a mixin declaration (assuming there are no other changes needed in the class). That in turn can make mixins more common which will help encourage users to learn about them.

Like I said before, I don't think this is that big of an issue with versioning and dart fix. As @lrhn has pointed out here and elsewhere, making your class a mixin is intentionally self-limiting, and changes to that should be breaking, to allow consumers of that API to audit their code. (Also, while searching for that comment, I found #32, where @yjbanov proposed this workaround before, albeit as a temporary solution).

munificent commented 2 years ago

But the point is new users would be using extends ChangeNotifier, instead of with ChangeNotifier, so they can't extend both ChangeNotifier and DataModel unless they learned to use with.

It would still allow that by using a with clause as usual.

Right, but again, there would be no way for users to know they should be using with if their initial intuition of extends works "properly" (that is, until later).

Yes, but that's arguably a feature. In general, it's nice if users don't have to learn every corner of the language at once. We provide both for-in loops and C-style for(;;) loops so that users who only know the latter can still be up and running.

If a user wants to use ChangeNotifier and only knows about extends, allowing that will let them make progress in cases where they don't need to apply another mixin too. They can defer learning about with until they actually need to.

But it does seem cruel to have the compiler say, "I know exactly what you meant to do here, but I'm going to yell at you and make you change your code anyway."

I guess it would make sense if this were a lint or warning then, because those pretty much cover the lines of "you're probably wrong about this, and you probably meant to do this instead".

I think we could also add a lint that suggests users use with Foo instead of extends Foo when Foo is a mixin. That would gently guide users towards learning about mixins without stopping them from making progress.

If we allow mixins to appear in extends clauses, then it makes it a non-breaking API change to turn a class declaration into a mixin declaration (assuming there are no other changes needed in the class). That in turn can make mixins more common which will help encourage users to learn about them.

Like I said before, I don't think this is that big of an issue with versioning and dart fix. As @lrhn has pointed out here and elsewhere, making your class a mixin is intentionally self-limiting, and changes to that should be breaking,

I'm talking about change going the other way. Because mixins are not widely known in Dart, there are a lot of abstract classes out there that extend Object and have a default constructor. Many of those classes could become mixins and the author might be happy to turn them into mixins (using an explicit mixin declaration) once they realize that's an option. But they can't make that change now because it would break any existing user that has the class in an extends clause.

Levi-Lesches commented 2 years ago

I think we could also add a lint that suggests users use with Foo instead of extends Foo when Foo is a mixin. That would gently guide users towards learning about mixins without stopping them from making progress.

All of the issues I listed are about what the "default" choice is -- extends or with. If there are lints or warnings telling users there's a better way to do things, then I don't think it's so bad to let them do it "wrong". As far as I can tell, nothing will go wrong by having an extends mindset when working with a mixin other than what I mentioned above that can't be done without explicitly writing with, but that's what the warning's for.

I suppose it would be weird having a supported language feature immediately throw up a warning, but there are other cases like using ! on a non-nullable variable that also do, and they're considered normal.

lrhn commented 2 years ago

I agree that it would be better to have a clean separation of mixins and classes, where you can only extends a class and only with a mixin, and, honestly, my personal goal is still to get there if possible. I just don't necessarily think it's possible, or even worth the effort. And, pragmatically, I've wanted this since the introduction of mixin declarations in 2018, and haven't been able to make any progress so far.

The primary goal is to disallow mixing in classes. That's just fundamentally problematic because there is no way to declare that a class is or is-not intended to be used as a mixin, it's derived from secondary characteristics which can accidentally be true of a class that is not intending it, and can be changed by what would otherwise be non-breaking changes to a class. That's just bad.

What's currently blocking us from disallowing mixing in classes is that people are still using it. We could then try to force those classes to be declared as mixins instead, but some of the classes are used as both superclasses and mixins. Even if we could convince Flutter to change ChangeNotifier to being a mixin, that would still break every piece of client code currently doing extends ChangeNotifier. We can't force all that code to change to with ChangeNotifier (well, we can for that one class, but in general ...) because we can't see which classes are really intended to be used as mixins. And we can't see all code to begin with, so we don't know where to change, so changing ChangeNotifier would be a breaking change. We need a migration plan which allows step-wise migration.

So, by allowing you to extend mixins, we increase our chance to make Flutter change ChangeNotifier (and all similar classes by all other people) to being declared as a mixin. That brings us one step close to being able to disallow mixing in classes: Changing all mixed in classes to be mixins as a non-breaking change. (And them, as @munificent said, we can add lints to encourage you to make that transition, and we can even add lints urging you to use with over extends, which your projects can opt in to or out of.)

And also remember that you can only extends Mixin if Mixin can be applied to Object, so you must still use with on most mixins. It's only the ones which truly has no super-dependencies that can be treated like they define a class directly.

If we get to Dart 3.0, and nobody (or almost nobody) has any extends Mixin anywhere, we can then still choose to remove the ability. Even if we don't, and only remove the ability to mix in classes, it's still much better than what we have now.

That's the goal.

This is a first step towards that goal.

Removing the ability to extend a mixin again can be a stretch goal, which we only do if it ends up making sense and everybody being happy about it. I'll be happy even if we don't get there.

(Heck, maybe everybody will then love being able to do extends Mixin instead of with Mixin, and will be happy to keep it.)

mateusfccp commented 2 years ago

Yeah, I like this gradual changes proposal. I only would change the last step to completely disallow "extending" mixins, as we will be going to Dart 3.0, where breaking changes makes sense.

If we get to Dart 3.0, and nobody (or almost nobody) has any extends Mixin anywhere, we can then still choose to remove the ability.

IMHO, I think in Dart 3.0 we should enforce it eve if there are people extending mixins... Obviously, we should gently guide them in the meantime (with lints, recommendations etc). If they don't follow the recommendations, it's their fault to have breaking changes in their system.

Hixie commented 2 years ago

FWIW, if the goal here is to allow Flutter to migrate ChangeNotifier from a class to a mixin, I don't think the proposal here helps that.

lrhn commented 2 years ago

@Hixie Why not? If it becomes valid to extend a mixin declaration, it will be a non-breaking change to change the ChangeNotifier class declaration to a mixin declaration. That should allow you to do so.

Our end goal is to disallow mixing in class declarations, and this should provide a non-breaking migration path for existing classes that are being mixed in, as well as ensure that there is no incentive to declare any new classes that are also intended to be mixed in (you should just declare those as mixins from the start).

Hixie commented 2 years ago

ChangeNotifier is two steps down an inheritance hierarchy. I don't think it makes sense for it to be a mixin.

Hixie commented 2 years ago

(I also don't really understand the benefit of allowing class Foo extends Mixin instead of class Foo with Mixin. It just seems to muddle things further. The migration path for the latter seems straightforward with dart fix.)

bwilkerson commented 2 years ago

We don't currently have a fix for that, but we certainly could.

munificent commented 2 years ago

ChangeNotifier is two steps down an inheritance hierarchy. I don't think it makes sense for it to be a mixin.

I'm a little confused. The docs don't show it having any superclass other than Object. I see lots with ChangeNotifier in a corpus of packages, so it is definitely being used as a mixin.

What am I missing here?

Hixie commented 2 years ago

Sorry I was thinking ValueNotifier, ChangeNotifier is just one step down. The inheritance hierarchy is:

Listenable
   |
ChangeNotifier 
  |
ValueNotifier

(The top link is done via implements rather than extends, in practice, because Listenable is just an interface.)

I don't really understand what problem we're trying to solve here, though. As far as I can tell, everything works fine today. The only issue is that prefer_mixin can't be silenced when a class is intended to be used as a mixin as well as a superclass.

munificent commented 2 years ago

Sorry I was thinking ValueNotifier, ChangeNotifier is just one step down. The inheritance hierarchy is:

Listenable
   |
ChangeNotifier 
  |
ValueNotifier

(The top link is done via implements rather than extends, in practice, because Listenable is just an interface.)

That distinction matters here. If ChangeNotifier extended Listenable, then users wouldn't be able to use it as a mixin (unless they manually implemented the Listenable interface themselves).

I don't really understand what problem we're trying to solve here, though.

  1. It's a source of confusion for library consumers and a maintenance burden for library authors that any class can be used a mixin if it happens to meet certain restrictions, even if the class author doesn't intend it to be used as one. We'd like to change the language so that you can only mix in types declared to support that by using mixin instead of class. We would change the language to make it an error to mix in a type declared using class. Anything you mixin would have to be declared using mixin.

  2. That language change would be breaking to Flutter and others because there are types like ChangeNotifier that are intended to be used as mixins but are written using class.

  3. Changing ChangeNotifier and friends to use mixin instead of class would be breaking to Flutter users since there are many existing uses of extends ChangeNotifier in their code.

To fix 3, we can simply allow mixins in extends clauses. (It's harmless sugar for extends Object with ___.) Doing this unblocks 2 so that ChangeNotifier and friends can be changed from class to mixin. That unblocks 1 so we can make it an error to mix in classes. The end result is a simpler language where it's easier for type authors and consumers to know how the type should be used.

when a class is intended to be used as a mixin as well as a superclass.

There's no material benefit to allowing a type to be used as both a mixin and superclass. They both result in a new subclass whose superclass is the mixin/class thing. The only capability you get is being able to use it in an extends clause, which we can just allow directly for mixins. Then it's clear to anyone looking at the type that it is a mixin and that it must continue to fit within the restrictions of being a mixin.

Hixie commented 2 years ago
  1. It's a source of confusion for library consumers and a maintenance burden for library authors that any class can be used a mixin if it happens to meet certain restrictions, even if the class author doesn't intend it to be used as one.

Is it? It doesn't seem like that much of a burden. Do we have data supporting this?

This really does not seem like a high priority change to the language. There are far bigger fires for us to deal with, IMHO.

Levi-Lesches commented 2 years ago

The only capability you get is being able to use it in an extends clause, which we can just allow directly for mixins.

I've been wondering about this: Can you make a mixin that shares functionality with another mixin? The closest I could get to this was:

mixin A { void a() => print("a"); }
/// Trying to implement `B extends A`, such that `A` is an implementation detail
mixin B on A { void b() => a(); }

/// But A is not an implementation detail, C has to explicitly mix it in
class C with A, B { }  

void main() => C().b();  // "a"
munificent commented 2 years ago
  1. It's a source of confusion for library consumers and a maintenance burden for library authors that any class can be used a mixin if it happens to meet certain restrictions, even if the class author doesn't intend it to be used as one.

Is it? It doesn't seem like that much of a burden. Do we have data supporting this?

I don't know if we have numeric data, but we've heard from API maintainers over the years that Dart's flexibility around how classes can be used makes it harder to evolve APIs without breaking them.

This really does not seem like a high priority change to the language.

Sure, but it's also a tiny change.

leafpetersen commented 2 years ago

Is it? It doesn't seem like that much of a burden. Do we have data supporting this?

@Hixie It is absolutely a burden for library maintainers, and we have data to support it in the form of extensive experience as library maintainers. The current best practice in numerous Dart team maintained libraries is to document classes in with comments indicating that a class is not intended to be mixed in. Example. But the problem with comments is that if someone ignores them, then making a change that breaks a use as mixin still breaks the downstream customer, and so we are still on the hook to either fix the issue on a roll (internally) or to deal with the breakage (externally).

lrhn commented 2 years ago

@munificent

That distinction matters here. If ChangeNotifier extended Listenable, then users wouldn't be able to use it as a mixin (unless they manually implemented the Listenable interface themselves).

No, not at all. You can't use a class as a mixin if it has a superclass other than Object.

So, if your class has an extends not followed by Object, or a with, then you cannot derive a mixin from it.

lrhn commented 2 years ago

The problem we are trying to fix is that you can mix in classes that are not intended to be mixed in.

Allowing that is a problem. If anyone uses the class as a mixin, it becomes a breaking change to add a constructor or a superclass. Obviously, you can just say "I don't care" and add a supertype or constructor anyway. Even our own breaking-change policy does not consider programs doing mixin application of classes not intended as mixins as valid programs. (We're a little too lenient for my taste: "must not mixin a class clearly documented as not intended to be used as a mixin", where it really should be "not clearly documented as intended to be used as a mixin").

In practice, it would be better to not allow you to mix in those classes at all, instead of allowing the foot-gun.

So, we need to either introduce new syntax to declare classes that can be used as mixins, and disallow mixing in any other class, or to just disallow mixing in classes entirely, and have a migration path for the classes which are currently being used as mixins, so they can be come mixins.

The former would be a declaration like mixin class Foo { ... } which can't have superclasses, on-types or generative constructors (all the restrictions of class and mixin declarations). That's a lot of work for not a lot of benefit (there aren't that many mixin-classes), but it's consistent. As with other breaking language changes, we'd have cross-version compatibility which allows old code to mix in new mixin-classes, and new code to mix in old classes, but new code can only mix in new mixins and mixin-classes.

The latter would just make it an error to mix in a class. Whoever wrote the class that was intended as a mixin would have to change it to a mixin declaration. That's much simpler, and what we are proposing to do. The problem is that it breaks existing code which extends that declaration, which makes it harder to push through a change. Not impossible. Again, language versioning can work: We can allow old code to extend new mixins, and new code must change that extends to with to be valid. Or we can just allow extends Mixin, and not require any code to be migrated except the declaration. That makes migration easier, and if that's what it takes to get there, I'm fine.

So, different proposals: In a new language version:

  1. Allow declaring mixin class which is like the current class that can be mixed in, but disallow mixing in any other new-code class. You can still mix in old-code classes, old code cannot mix in new-code plain classes.
  2. Disallow mixing in a class. You can change the class declaration to a mixin declaration to keep mixin-ability, but then you can't extend it (you have to use with). You can still mix old-code classes. Old code classes can extend new-code mixin declarations if they allow Object as supertype.
  3. Disallow mixing in a class. You can change the class declaration to a mixin declaration to keep mixin-ability. You can still mix old-code classes. Any class can extend new-code mixin declarations if they allow Object as supertype.

Assume we choose one of these, and do it in 2.16, which one would you prefer?

(In either case, you'd have to change ChangeNotifier to be a mixin to keep the ability to mix it in.)

Levi-Lesches commented 2 years ago

It seems like one of the questions we circle back to (and if I'm reading it right, the difference between options 2 and 3), is whether you should be allowed to extend a mixin, barring migration conveniences.

Using the example of Flutter's ChangeNotifier, users should be using with, but the question is what would another type of ChangeNotifier, like ValueNotifier, use? There's an overlap here between wanting to include the functionality of a mixin by using with, and wanting to offer similar functionality of a class using extends.

munificent commented 2 years ago

but the question is what would another type of ChangeNotifier, like ValueNotifier, use?

Since ValueNotifier doesn't extend Object (it extends ChangeNotifier), it can't be used as a mixin and can only be used as a superclass.

Levi-Lesches commented 2 years ago

Right, my comment was addressing the earlier question of whether ValueNotifier should be using with ChangeNotifier or extends ChangeNotifier, and why.

lrhn commented 1 month ago

Seems to be handled by the language-versioned switch in Dart 3.0 to needing mixin class for declarations that can be both extended and mixed in.

We could still allow extends Mixin, meaning extends Object with Mixin, instead of having to write with Mixin, but I don't think there is any real benefit left. If anything, it'll just be confusing that you extends Mixin, but it doesn't have a generative constructor.