Skip to content

Add "mixin class" to the "base-interface-final" proposal. #2674

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

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

munificent
Copy link
Member

I went ahead and took the more aggressive step of also saying that you can't mixin a class unless it has the "mixin" modifier. However, that rule is gated by a language version so that it's not breaking.

Since "mixin class" allows you to preserve the previous behavior of class+mixin, I think we can get away with doing this change in one step. Basically, whenever you upgrade your library to the latest version, you add "mixin" to whatever classes you want to support being used as mixins and you're done.

If you think that's too aggressive, let me know and I can change this. If we do that, though, it ends up meaning that the "mixin" modifier doesn't actually do anything (except yell at you if you apply it to a class with a generative constructor or non-Object superclass).

Lasse, I really like your suggestion for "mixin class". I think this lets us close the loose end of ChangeNotifier henceforth and forever without a lengthy migration and lets us support a rare but potentially useful combination of capabilities.

@mit-mit @eernstg @stereotype441 @jakemac53 @natebosch @kallentu

I went ahead and took the more aggressive step of also saying that you
can't mixin a class unless it has the "mixin" modifier. However, that
rule is gated by a language version so that it's not breaking.

Since "mixin class" allows you to preserve the previous behavior of
class+mixin, I think we can get away with doing this change in one step.
Basically, whenever you upgrade your library to the latest version, you
add "mixin" to whatever classes you want to support being used as mixins
and you're done.

If you think that's too aggressive, let me know and I can change this.
If we do that, though, it ends up meaning that the "mixin" modifier
doesn't actually *do* anything (except yell at you if you apply it to a
class with a generative constructor or non-Object superclass).
@natebosch
Copy link
Member

Do we expect that authors of classes used as mixins are reliably aware of that usage?

I think this is worth the risk as long as it's not breaking to add mixin to an existing class so that we can safely republish if we find there is a critical use case.

@lrhn
Copy link
Member

lrhn commented Dec 1, 2022

I expect authors of classes that are intended to be used as mixins to be aware of that (because they have documented it on the class, otherwise they probably didn't intend it).

It should not be breaking to add mixin to a class declaration. Adding mixin puts restrictions on the class itself (no generative constructors declared, no superclass other than Object), but if the class was used as a mixin before, it already satisfied those constraints. From the outside, adding mixin just allows you to mix in the class, it doesn't prevent anything.

Adding base and not mixin may break code that depended on mixing in the class, because they no longer can.
That means that forgetting to add the mixin is the breaking change. If the class is intended as a mixin, your own tests should catch that.

Changing a class that was not intended as a mixin into a class that cannot be used as a mixin might break users who used it as a mixin anyway.
Preventing such unintended use is the point of restricting you to mixing in classes that are declared as mixin class, so it'd be working as intended in breaking those classes.

@eernstg
Copy link
Member

eernstg commented Dec 1, 2022

I hope we can do this! However, it could be helpful to get some input from the Flutter team: Which and how many classes do they expect to turn into mixin classes? Do they know, or would they be worried about finding them? Luckily, lots of classes just become an error if mixin is added, which means that they can be ruled out immediately (that is, they aren't in use as mixins).

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, ship it!

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested a couple of typo-level changes

@munificent
Copy link
Member Author

However, it could be helpful to get some input from the Flutter team: Which and how many classes do they expect to turn into mixin classes?

Excellent point. I'll reach out to them.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 2, 2022
… enabled.

From the changes in dart-lang/language#2674 that specify from a certain language version, we would like classes to not be used as mixins unless specified as a 'mixin class'.

Currently, this behaviour is under the sealed-class flag. May be subject to change as the other modifiers are added, but I'd at least like to make sure this works for sealed classes.

Change-Id: I5754b383327dde06d49175fe2d05c8ba7462145f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/273082
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
@munificent munificent merged commit b7edc0b into master Dec 5, 2022
@munificent munificent deleted the mixin-class branch December 5, 2022 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants