Skip to content

[Class Modifiers] Should final classes from other libraries be usable as on types? #2933

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
leafpetersen opened this issue Mar 21, 2023 · 9 comments
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.

Comments

@leafpetersen
Copy link
Member

Currently, I believe the specification allows the following:

import "lib.dart" show FinalClass; // FinalClass is declared as `final class FinalClass {}`
base mixin M on FinalClass {}

Do we want to allow this?

It's more or less harmless (I think), in that the only way to use M is to apply it to FinalClass or a subtype thereof. The former can only be done inside of the library that declares FinalClass. The latter could potentially be done outside of the library that declares FinalClass if the subtype re-opens the class.

But it also feels surprising, and not especially useful. We've chosen to disallow this for sealed classes.

cc @dart-lang/language-team

@leafpetersen leafpetersen added the class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins. label Mar 21, 2023
@eernstg
Copy link
Member

eernstg commented Mar 21, 2023

I think we do achieve a small amount of additional expressive power if we can ensure that this kind of declaration is allowed.

However, I tend to think that the simplicity achieved by making it an error may be more valuable.

In particular, this means that we don't have any special exceptions for on edges in the superinterface graph, we simply have a rule which states that a class/mixin/mixin_class cannot have a final type declared in a different library as a direct superinterface, no matter whether the name of the edge is extends/with, implements, or on.

Let's see if we can come up with an example where we'd really want to do it, anyway:

// --- Library 'lib.dart'.

final class FinalClass {}

@reopen base class Sub1 implements FinalClass {}
... // Lots of classes Sub2 ... SubN-1
@reopen base class SubN implements FinalClass {}

// --- Library 'other.dart'.
import "lib.dart" show FinalClass;

base mixin M on FinalClass { void foo() {} }

// Yes!!! We can add `foo` to any of those `Sub...` classes as we please!
base class MySubJ extends SubJ with M {}

The desirable property of this set up would be that M can refer to FinalClass once and for all (and it's allowed to depend on the interface of FinalClass), and this will open the door for applications of M to all the Sub... classes. We could also just remove on FinalClass and then we'd still be able to apply M to each of the Sub... classes, but then we wouldn't have the ability to perform superinvocations or regular invocations on this using members of FinalClass in the body of M.

However, if we explicitly do not wish to provide any exhaustiveness guarantees based on a final class (that job is done by sealed), it seems plausible that FinalClass could just have been declared as a base class, and then I can't really spot what is lost for the author of 'lib.dart'.

@leafpetersen
Copy link
Member Author

I realized last night that if we allow this, then it's still technically a breaking change to add a method to a final class, which seems bad.

@mit-mit
Copy link
Member

mit-mit commented Mar 21, 2023

then it's still technically a breaking change to add a method to a final class, which seems bad.

Ouch, I think that alone is enough reason that we should disallow it.

@lrhn
Copy link
Member

lrhn commented Mar 21, 2023

I don't think it's worth disallowing.

If someone writes mixin Foo on MyFinalClass there is only one rational reason for it: To apply the mixin to base-declared subclasses of MyFinalClass.

Disallowing that will prevent something that's potentially useful.
Adding a member to that final class will also add it to the base subclasses, and will be just as breaking to potential subclasses whether there is a mixin involved or not.

The alternative is that you're making a mixin that you can't use for anything.
The mixin can't ever be applied if there is no visible base-marked subclass of the final class, and a mixin which cannot be applied is just a namespace with extra steps, and an extra ability to be broken without any benefit in return.
And if you're broken, you've added an instance method to it that cannot possibly be invoked ever. You're clearly not testing that method.

I don't think you should worry about "breaking" useless code, it won't exist in code-bases that are worth caring about. And therefore I don't think we should consider it a problem to worry about.
Preventing it is a net loss, because there is no real benefit.

@leafpetersen
Copy link
Member Author

If someone writes mixin Foo on MyFinalClass there is only one rational reason for it: To apply the mixin to base-declared subclasses of MyFinalClass.

Why would you do this? Do we have any real examples? And specifically where declaring MyFinalClass as base wouldn't work just as well? I get that one can do it. I question whether anyone ever will.

Preventing it is a net loss, because there is no real benefit.

I'm not convinced there is any real loss here. And conceptually at least, I think it's a win. When I write a final class, I expect there to be no subtypes that I don't know about. It's weird that there can be some other class out there, which is a direct subtype of my final class.

@natebosch
Copy link
Member

We'd have the option to allow this in a later version if someone discovers a valid use case. I do think users who try writing this will be better served by an immediate error than by having to discover the limitations in other ways.

@lrhn
Copy link
Member

lrhn commented Mar 22, 2023

The arguments here suggests that we intend a final class to guarantee that adding a member is non-breaking.

That's not true if it has non-final (therefore base) sub-classes, but that's a property of the type hierarchy, not the final class itself.

Whether it's worth having a mixin on FinalClass is also a property of the type hierarchy.
It can be useful.

It's correct that having a (completely useless) on FinalClass with no base subclasses makes adding a member hypothetically breaking. I'd just say that adding a member to such a class is not considered breaking, so
you should just not have useless mixins.

(It is also true that having a base SuperClass instead of final Superclass doesn't have to expose any new functionality, you can just not have any public generative constructors, then people still cannot extend it. So no real loss if you want to allow people to make mixins on a supertype.)

But it's yet another non-orthogonal restriction, and I don't think it will ever solve a real problem.

@lrhn
Copy link
Member

lrhn commented Mar 22, 2023

Decided: We disallow it for now.

I actually came up with a good reason for disallowing. It makes a final type be completely opaque, which makes it possible to change it to, e.g., an inline class in a future change.
A type which can be uses as an on type must be an "interface type", so a use as an on type can also prevent such a change.

(We should make sure that a final inline class will also have the property that it can be changed to another kind of type without changing visible behavior. Well, unless someone casts it to its representation type. Should we disallow casting of a final type entirely? 😉)

@eernstg
Copy link
Member

eernstg commented Mar 23, 2023

I think we should close this issue now that we have a decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.
Projects
None yet
Development

No branches or pull requests

5 participants