prefer_mixin lacks nuance

[Hixie]

The prefer_mixin lint points out uses of with that mix in classes rather than mixins.

Unfortunately, sometimes a class is intended to be used as both a mixin and a class, and there's currently no way to indicate that a particular class is therefore ok for use with with.

Examples in Flutter's framework include WidgetsBindingObserver and ChangeNotifier.

[pq]

We make some exceptions for a few SDK classes

• IterableMixin
• ListMixin
• MapMixin
• SetMixin
• StringConversionSinkMixin (dart:convert)

as a holdover until changes on the SDK-side let us handle this in a more principled way. It's not an awesome solution but we could consider adding WidgetsBindingObserver and ChangeNotifier to the allow-list?

@lrhn: I wonder if there are any updates relevant to this from the SDK or core lib perspective?

[Hixie]

I definitely would not want anything Flutter-related to be hard-coded into the linter in that way, because we want it to be just as possible for people to copy our framework and create the same product, and that won't work if the tools are hard-coded to a particular instance of the framework.

My ideal solution here would be to just introduce some sort of annotation that we can add to all the classes mentioned above that just tells the lint to cool it on with clauses that use those classes.

[pq]

Sounds good. (If package:meta or a copy of some kind was available in the SDK, this would be an interesting approach there too.)

Here's a tracking issue on package:meta where this work will start: #47437.

[bwilkerson]

Unfortunately, sometimes a class is intended to be used as both a mixin and a class, and there's currently no way to indicate that a particular class is therefore ok for use with with.

It's a little more verbose, but you could define both a mixin and a class that uses that mixin. Doing so would cover both of the use cases without too much extra code. It would also negate the need for an annotation. Perhaps something like

mixin ChangeNotifier implements Listenable {
  // ...
}

class SimpleChangeNotifier with ChangeNotifier {}

I'm not objecting to an annotation, just making sure the alternatives have been considered.

... was available in the SDK ...

It might be worth asking whether this is something that we'd want to use (and hence define) in the SDK.

@lrhn For visibility.

[Hixie]

We've considered it, but I think having two classes makes the API harder to understand. In practice it would be six classes -- Listenable (the interface), ChangeNotifier (the class), ChangeNotifierMixin (the mixin), ValueListenable (the interface), ValueNotifier (the class), ValueNotifierMixin (the mixin), and that's before we start talking about Animation and so on. It's already pretty confusing as it is without adding more...

[lrhn]

I stand by dart-lang/language#33 (comment)

The problem is migration. If someone currently extends a class we want to convert to a mixin, then it's breaking to change it.
We could add an annotation meaning "this is intended as a mixin", and make it a warning to extend such a class (you'd have to write with instead). Then eventually we could make the change.
I suggested that in dart-lang/language#1529.

I'm more inclined towards a language version based migration, like dart-lang/language#1643.

[pq]

Thanks @lrhn!

@Hixie: to confirm, you said above that

sometimes a class is intended to be used as both a mixin and a class,

Do you anticipate a migration path away from this dual use or is this the end state for the API?

Either way, an annotation (#47437) feels like the best next step.

[Hixie]

I'm not anticipating us changing this API currently, but I'm open to the possibility that I'm just missing something important about how mixins work and that @lrhn is right that we should migrate.

[lrhn]

sometimes a class is intended to be used as both a mixin and a class,

Do you anticipate a migration path away from this dual use or is this the end state for the API?

I want to migrate that to being a mixin declaration, and disallow deriving mixins from class declarations entirely. It's too fragile to allow without explicit permission.

You can still use that as a mixin. You can still "extend" it by doing extends Object with MyMixin or just with MyMixin (which implicitly extends Object).

Maybe, just maybe, we should consider allowing extends MyMixin to implicitly mean extends Object with Mixin, but I'm wary about that because it confuses the story we tell users: You extend classes, you mix in mixins. (Except when you extend some mixins, which you only can if they apply to Object).

Or we could allow a declaration like mixin class MyThing which must satisfy all the requirements of both class and mixin, but can be used as both. Not sure it's worth it.

[lrhn]

Short term (possibly even until Dart 3.0), the language team are fine with introducing an annotation like @mixin and a lint warning for using non-@mixin-annotated classes as mixins — and perhaps even for using @mixin-annotated classes as superclasses, urging people to move towards with instead.
We can then consider moving such lints into the lints/ package as scoring or even recommended lints when we are ready.

That could move us towards the point where existing classes used as mixins are actually made into mixin declarations, which might again allow us to make the breaking change to disallow mixing in classes before Dart 3.0.

(Read: We do want this, but it's not urgent, and if we can stage the migration over time, using lints, it's easier than making a breaking language change and having to migrate "lots" of existing code.)

We'll then have to figure out how to use annotations in the platform libraries too, since we also have some legacy mixins-declared-as-classes that we haven't dared migrating yet, but that's a separate issue.

[pq]

See also, dart-lang/language#1942.

[robert-j-webb]

Hey ran into this when using ChangeNotifier and WidgetsBindingObserver

Is there some kind of canonical solution to this? We should probably update the linter message to help users resolve this.

I had a class like this

class SomeModel extends ChangeNotifier with WidgetsBindingObserver {

Which gave me the error: Prefer using mixins. See https://dart-lang.github.io/linter/lints/prefer_mixin.html

and I converted it to this:

class SomeModel with WidgetsBindingObserver, ChangeNotifier {

but that still triggered the lint? Sorry I'm not a dart wizard so I don't really know what the correct solution should be here. I'm happy to just ignore the lint but it doesn't feel right.

[pq]

The issue here is that WidgetsBindingObserver is a class that you're using as a mixin. It looks like it is designed to be potentially used that way so this is not really a problem of yours but reflects our lack of support for classes that are kind of exceptionally mixin-able.

Reading the docs for WidgetsBindingObserver am I right that you're only getting stubbed methods? If that's the case maybe you could just implement it instead (and provide your own implementations)?

class SomeModel extends ChangeNotifier implements WidgetsBindingObserver

@goderbauer: any advice here?

[goderbauer]

I don't have great advice, unfortunately. That lint doesn't work well with some of the concepts in the flutter framework that pre-dates mixins, see also dart-lang/core#743.

However, it is totally fine to implement the WidgetsBindingObserver as @pq suggested. You'd probably just have to copy a lot of the empty stubs from the base classes to fully implement the interface. Or you can just ignore the lint with // ignore: ....

To really fix this, we'd need some changes to the dart languages, as discussed in the issues linked from ttps://github.com/dart-lang/lints/issues/26.

[goderbauer]

With the mixin modifier, this is likely no longer an issue and can be closed. We are also now able to enable this lint in flutter: flutter/flutter#123159.

[pq]

Good deal. Thanks @goderbauer!