Skip to content

Can we have mixin class? #31

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
yjbanov opened this issue Sep 26, 2018 · 11 comments
Closed

Can we have mixin class? #31

yjbanov opened this issue Sep 26, 2018 · 11 comments
Labels
request Requests to resolve a particular developer problem

Comments

@yjbanov
Copy link

yjbanov commented Sep 26, 2018

A mixin class is a non-super-mixin that allows extension. This is already supported in Dart (you just use the class keyword). The problem is that such mixins do not prevent unintentional breaking changes. For example:

  • you can change it to extend another class
  • you can add a constructor or a factory to the mixin
  • you can instantiate the mixin directly (if it is concrete)
  • you can add a super call to the class

These are all valid changes to normal Dart classes, but are breaking to a mixin. A dedicated class mixin declaration could prevent these issues by restricting what you can do with the class, similar to the restrictions placed on the plain mixin (which is allowed to be a super-mixin).

Strawman proposal:

mixin class Foo extends Bar {
//              ^
//              ERROR: cannot extends other classes

  Foo();
// ^
// ERROR: cannot have constructors

  void foo() {
    super.hashCode;
// ^
// ERROR: cannot call super
  }
}

main() {
  Foo();  // ERROR: cannot instantiate mixins
}

One example of such a mixin in Flutter is WidgetsBindingObserver.

prefer_mixin lint

The current situation makes prefer_mixin lint unusable because the lint shows up at the usage site (the with clause), not at the mixin declaration site. IOW, the lint does not encourage the author of the mixin to change the declaration of the mixin, but asks the user of the mixin to use something else, which may not be possible.

Alternatively: remove this capability

If we don't like this idea in general, we should be clearer about the removal of this capability so we can start cleaning up our code now. Currently the spec only says that it may potentially be removed. However, that means our users will have to live with these quirks for a long time. Flutter will declare API stability soon and it will be hard to change this in the future.

@Hixie @lrhn @leafpetersen @eernstg

@bwilkerson
Copy link
Member

... the lint shows up at the usage site (the with clause), not at the mixin declaration site.

Flagging this at the declaration site would require whole-program analysis, which analyzer doesn't currently do, and a closed-world assumption, which analyzer doesn't make.

We might be able to relax the lint so that classes that do not use super are not flagged. Let me know if you decide that that's the right approach and we can look into whether it's actually feasible.

@yjbanov
Copy link
Author

yjbanov commented Sep 27, 2018

We might be able to relax the lint so that classes that do not use super are not flagged.

That's partially why I filed the issue. We want to make sure that this lint is useful when Flutter goes stable. However, what to do about it depends on the language direction w.r.t. non-super-mixins.

@lrhn
Copy link
Member

lrhn commented Sep 27, 2018

I would love to remove the ability to derive mixins from classes entirely, for all the reasons you mention.

The relation between a mixin application and the mixin class is tighter than the one between a sub-class and its super-class. If you use a class as a mixin, any number of otherwise innocuous changes can break the mixin application, so using any class as a mixin, which is not intended as a mixin, is highly fragile and discouraged.

Any class in the SDK that is not named *Mixin is not intended as a mixin. We have tried to prevent usage as a mixin by adding generative constructors, but I'm sure we haven't gotten all the classes.
(Another option would be to have class _NotAMixin extends Object {} and extend _NotAMixin instead of Object on all the non-mixin classes. Still highly annoying).

There is no good solution that doesn't remove the ability to derive a mixin from a class.

Marking classes as non-mixinable requires the author to think about the issue at all, which is putting the responsibility in the wrong place. Having to mark classes as mixin-able is better handled by just converting the class to a mixin declaration directly. There is no reason to have a mixin class Foo over just having mixin Foo (except if someone is also extending the class, they should convert that to with Foo instead of extends Foo when we get that implemented).

The obvious problem with removing class-derived mixins is migration. We can't migrate all existing code. We can't detect whether a class is intended as a mixin, we certainly can't detect whether it's used as a mixin (unless we know all Dart code in the world).

So, this sounds like a migration problem that requires opt-in. If we get opt-in language features, then opting into "declared-mixins-only" would prevent any class in the opted-in code from being used as a mixin, and would prevent that code from using any class as a mixin. Opting in would be a breaking change, in case someone actually uses the class as a mixin.

@lrhn lrhn added the request Requests to resolve a particular developer problem label Sep 27, 2018
@Hixie
Copy link

Hixie commented Sep 27, 2018

FWIW WidgetsBindingObserver is explicitly intended to be used as both a mixin and a superclass (it's not a supermixin).

@yjbanov
Copy link
Author

yjbanov commented Sep 27, 2018

@Hixie in flutter/plugins#797 (comment) you mention that WidgetsBindingObserver is meant to be used as a super-class too. What properties of extends are we looking for that are not provided by with?

@Hixie
Copy link

Hixie commented Sep 27, 2018

Simplicity.

@yjbanov
Copy link
Author

yjbanov commented Sep 27, 2018

Just chatted offline with @Hixie. We are not looking for any extra properties of extends that with doesn't give us. We just do not want to break our users who already use extends for some of the mixins.

This is good! I think this allows us to remove the ability to derive a mixin without having to update user code. To do that we will have to overload what extends means.

Imagine this declaration:

class Foo extends Bar

The compiler first checks whether Bar is a mixin or a class.

If Bar is a mixin, then the compiler desugars the above declaration into:

class Foo extends Object with Bar

More generally, class Foo extends M1 with M2, ..., Mk desugars into class Foo extends Object with M1, M2, ..., Mk.

If Bar is a class, then it's a normal derivation. The lint prefer_mixin then can be used to help authors find classes used as mixins and convert them to mixin syntax without breaking their users.

This approach will allow Flutter to convert its classes-meant-to-be-mixins from class to mixin and not break existing user code.

Linting

Another issue that came up in the conversation is that while we are OK with converting our classes-meant-to-be-mixins to the mixin syntax, we do not want to cause analyzer warnings for classes our users defined and use as mixins in their code. We only want to warn about classes that we defined that are users use as mixins. If our users so choose, they can enable the lint for their project and see warnings about their code. Simply upgrading from Flutter RC2 to Flutter 1.0 should not cause analysis warnings.

So instead, could we somehow have the lint only check for mis-uses of our classes? Example:

// material.dart

class FlatButton { ... }

// snail_racing_app.dart

class SnailButton extends Widget with FlatButton { ... }
//                                    ^
//                                   ERROR: FlatButton is a class, not a mixin

class Foo { ... }

// This is fine. It's their own shenanigans.
class Bar extends Object with Foo { ... }

IOW, we'd like users to deal with their own code migrations on their own. We prefer that we not use Flutter as a mixin police.

@bwilkerson
Copy link
Member

... we do not want to cause analyzer warnings for classes our users defined and use as mixins in their code.

Lints are only run over the code in a package if the package opts in to the lint in the analysis options file. Users that have a package that depends on 'flutter' will not get any lint warnings about using classes in a with clause unless they explicitly enable the lint.

So, you can't cause lint warnings for your users.

We only want to warn about classes that we defined that are users use as mixins.

For the same reason, you can't do that, either. You can't enable a lint in packages that depend on 'flutter'.

But I think that the behavior you describe is only one way of accomplishing your real goal, which is to control how users can use the API you provide. The best solution I know of would involve language support that we don't have.

The only other alternative I can think of at the moment is to implement a hint that is enabled everywhere and that is specific to code that depends on 'flutter'. That seems like a horrid hack to me, but I don't have a better suggestion at this point.

@Hixie
Copy link

Hixie commented Sep 27, 2018

We select the default lints for our users, so we actually can cause them whatever trouble we want to cause them. :-)

@yjbanov
Copy link
Author

yjbanov commented Sep 27, 2018

Yeah, flutter analyze and Flutter IDEs use this file for our users' Flutter code: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/analysis_options_user.yaml

@yjbanov
Copy link
Author

yjbanov commented Sep 28, 2018

I'm closing this issue. I have an alternative proposal: #32 (based on #31 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

4 participants