Skip to content

opt-out for old-school mixin-like classes from prefer_mixin #45343

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

Closed
goderbauer opened this issue Mar 16, 2021 · 6 comments
Closed

opt-out for old-school mixin-like classes from prefer_mixin #45343

goderbauer opened this issue Mar 16, 2021 · 6 comments
Labels
customer-flutter devexp-pkg-meta Issues related to package:meta legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on

Comments

@goderbauer
Copy link
Contributor

Flutter has some classes (like ChangeNotifier and WidgetsBindingObserver) where we tell developers that those can be extended, implemented, or mixed-in. However, the prefer_mixin lint is not happy when they are used as a mixin in a with clause. Since those classes pre-date the introduction of actual mixins in Dart, it would be nice if they can be opted out from the lint, e.g. by annotating them with something like @legacyMixin.

The Dart SDK also has some of these legacy mixins that would benefit from such an annotation (example: IterableMixin) to avoid the prefer_mixin warning in the following code snippet:

/// info: Prefer using mixins. (prefer_mixin)
abstract class Foo with IterableMixin<int> {
  // ...
}

/cc @pq @Hixie @mit-mit @munificent @devoncarew

@goderbauer goderbauer changed the title opt out old-school mixin-like classes from prefer_mixin opt out for old-school mixin-like classes from prefer_mixin Mar 16, 2021
@goderbauer goderbauer changed the title opt out for old-school mixin-like classes from prefer_mixin opt-out for old-school mixin-like classes from prefer_mixin Mar 16, 2021
@bwilkerson
Copy link
Member

If the annotation were to be used in the SDK it would need to be defined in the SDK, so I'll cc a couple more people: @leafpetersen @lrhn.

@pq pq transferred this issue from dart-archive/linter Mar 16, 2021
@pq pq added devexp-linter Issues with the analyzer's support for the linter package pkg-meta devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. customer-flutter labels Mar 16, 2021
@leafpetersen
Copy link
Member

@lrhn should we consider making a plan to migrate SDK class mixins to mixin mixins?

@lrhn
Copy link
Member

lrhn commented Mar 17, 2021

@leaf Absolutely.

It's breaking because someone might be extending the existing class, and you can't extends Mixin.

The best solution we've come up with for that is to allow extends Mixin to be equivalent to extends Object with Mixin. That would make it non-breaking to change existing classes-used-as-mixins to actual mixins, and bring us one step closer to simply disallowing classes as mixins entirely (which was always the goal).
See: dart-lang/language#1068 (comment)

We just hoped we could avoid the extends Mixin because it might cause more confusion about what a mixin really is, and how it differs from a class, but after a significant time, there has been zero progress on actually migrating people off class-mixins.

So, let's do it. Allow extends M to refer to a mixin declaration, in which case it's equivalent to extends Object with M (or just with M , which is the recommended syntax that is also equivalent to extends Object with M).
It's not purely syntactic replacing extends with with in case there is a following with, so extends M1 with M2 becomes with M1, M2, and we need to consider the class C = M with M2; syntax and also allow a mixin in the M position there, meaning class C = Object with M, M2.

With that in place, it should be a non-breaking change to switch a class declaration which is intended as a mixin, to be a mixin declaration. Then we can start bugging people about using classes as mixins (effectively deprecating that feature, so we can remove it eventually, like in Dart 3.0), and about using extends Mixin instead of with Mixin, but their code will keep running.

I'll file an issue: dart-lang/language#1529
(Pretty sure I've suggested this before, but couldn't find an issue).

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Mar 18, 2021
@bwilkerson
Copy link
Member

See also #47437

@bwilkerson bwilkerson added devexp-pkg-meta Issues related to package:meta and removed devexp-warning Issues with the analyzer's Warning codes devexp-linter Issues with the analyzer's support for the linter package pkg-meta labels Jul 18, 2022
@parlough
Copy link
Member

Thanks everyone for the discussion.

Now that the mixin class modifier now exists, the SDK migrated to use it ands so did Flutter (which was also able to enable prefer_mixin).

As a result, I don't think an opt-out would be worth it at this point. Any new code needing this functionality can use mixin class, so the only situation that remains is using with LegacyMixin in a Dart 3+ file where LegacyMixin is declared in a pre-Dart 3 file as a class. That non-Dart 3 file can relatively easily migrate to a language version of Dart 3 or later to switch to a mixin class.

Any concerns about closing this? :)

@lrhn
Copy link
Member

lrhn commented Dec 17, 2023

No concerns.

The mixin class declaration was introduced specifically to cover use-cases like this, so we could migrate away from classes being incident mixin -able without it being breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-flutter devexp-pkg-meta Issues related to package:meta legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

7 participants