Skip to content

Make it an error to use a sealed class as a mixin #2651

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
munificent opened this issue Nov 23, 2022 · 4 comments
Closed

Make it an error to use a sealed class as a mixin #2651

munificent opened this issue Nov 23, 2022 · 4 comments
Labels
patterns Issues related to pattern matching.

Comments

@munificent
Copy link
Member

We eventually would like to move away from using classes as mixins. The base/interface/final proposal takes a step in that direction by preventing you from mixing in a class that has any of those modifiers. Since no classes have those modifiers today, we can do that since it's non-breaking.

We should do the same restriction for classes with the sealed modifier. If you want to mix in a sealed thing, that thing should be a sealed mixin, not a sealed class.

@munificent munificent added the patterns Issues related to pattern matching. label Nov 23, 2022
@lrhn
Copy link
Member

lrhn commented Nov 23, 2022

We have the recurring issue of some people having declarations that they want to use as both a superclass and a mixin, with both extends and with.
And existing classes doing so.

I've wanted to get away from that for a long time, and have failed spectacularly to even move the needle, because making existing code invalid is a hard sell. (#1942, etc.)

If we make a sealed/base class not mix-in-able, we prevent that existing class from using either of those, even if it probably wants to be a base declaration, but we still do not provide a migration path that doesn't break existing code that uses the class as a superclass and/or mixin.

That suggests using language versioning to allow old code to still mix in a base class, or even extend a base mixin, so migration can happen in two steps. That's still a required migration, though. And it requires having separate semantics for new and old code interacting with new sealed/base declarations, instead of using the same behavior everywhere.

We could embrace the current use case instead, and allow you to declare a mixin class which can be both. (And which has all the restrictions of both class and mixin declarations, so no way to write an on or extends clause - so not way to have a superclass other than Object or an on type other than Object - and no way to write a generative constructor.)

Alternatively we can allow you to write extends MixinName as shorthand for extends Object with MixinName (#1942). Then it's safe to migrate a class to being a mixin.

I think I prefer the mixin class approach.

@munificent
Copy link
Member Author

We have the recurring issue of some people having declarations that they want to use as both a superclass and a mixin, with both extends and with. And existing classes doing so.

I know we have a couple of existing classes that do this, but do you see users deliberately wanting to create new classes that are used both as mixins and classes? I can't recall any, and it's hard to see a compelling reason to support it. As I understand it, literally the only benefit you get from being able to use a mixin thing as a class too is that you can write extends Thing instead of with Thing?

That doesn't seem like enough value to me to justify supporting mixin class and teaching users about it.

I'd rather just do #1942. In fact, #1942 might actually help users understand how mixins work because it starts to reinforce that a mixin is a superclass.

If we make a sealed/base class not mix-in-able, we prevent that existing class from using either of those, even if it probably wants to be a base declaration, but we still do not provide a migration path that doesn't break existing code that uses the class as a superclass and/or mixin.

That's a fair point, but I can live with that. I think the set of people who maintain a class where:

  1. It is intended to be used as both a class and a mixin.
  2. They want to continue to support that capability.
  3. They also want to use sealed/base/final to remove other capabilities from that same class.

Is likely to be very small and possibly empty.

Whatever we do, I don't want to get stuck on this point such that we're unable to make progress on any other modifiers. I really want base, interface, final. I think it will feel very irregular if we give them sealed but don't give them any other way to decompose the bundle of restrictions that sealed implies.

So I think it would be fine to do what this issue says and make it an error to mixin a class marked sealed, base, etc. in anticipation of us later doing #1942 and eventually removing the ability to mixin classes. I think it would also be OK to not make that an error now. Instead, we let users mix in classes with those modifiers and possibly dig ourselves incrementally farther into the hole for when we later remove support for mixing in classes.

I could go either way. I just don't want to get stuck.

@munificent
Copy link
Member Author

I have a PR out (#2674) that addresses this by adding support for explicit mixin class declarations when you want to be able to use a class as a mixin.

@munificent
Copy link
Member Author

Closing now that #2674 has merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patterns Issues related to pattern matching.
Projects
None yet
Development

No branches or pull requests

2 participants