Skip to content
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

Allow sealed base class, because it is useful #3121

Open
jakemac53 opened this issue May 31, 2023 · 18 comments
Open

Allow sealed base class, because it is useful #3121

jakemac53 opened this issue May 31, 2023 · 18 comments
Labels
class-modifiers-later Issues related to "base", "final", "interface", and "sealed" on classes and mixins, tbd later request Requests to resolve a particular developer problem

Comments

@jakemac53
Copy link
Contributor

In general I think our logic for what the valid combinations are was a bit flawed because we used the table for what is allowed in other libraries, but should have used a table based on what is allowed in any library (including the current one). There may be other valid use cases that are not allowed, other than this one, from within the same library.

Use case

You have a sealed class, and you want to ensure all possible implementations of that class are transitive extensions of it.

You can achieve this today by making all the subtypes in your library be themselves base classes, but it could be easy to miss one on accident, in particular when adding new ones. Allowing sealed base would allow you to enforce this property, since base is transitive even within the same library.

Concretely, this is coming up for the Code class in macros, I want every piece of Code to ultimately extend the base sealed class, but they must go through some more specific subtype to get there (DeclarationCode, ExpressionCode, etc).

It is OK for people to build their own subtypes of those, but only through extension. We want to be able to put in some form of validation in their constructors, and ensure that those constraints are met.

@jakemac53 jakemac53 added the request Requests to resolve a particular developer problem label May 31, 2023
@jakemac53 jakemac53 added the class-modifiers-later Issues related to "base", "final", "interface", and "sealed" on classes and mixins, tbd later label May 31, 2023
@rrousselGit
Copy link

Could we do so for sealed mixin class too?

@lrhn
Copy link
Member

lrhn commented May 31, 2023

We have no modifiers for controlling what is allowed in the current library, because they don't work anyway.
Even if you could declare a sealed base class, you can still implement it inside the same library, the base doesn't affect you.
The only thing it does is to force you to put base on every subclass (at least until #3108).

Which is precisely what you want, to help you from forgetting to add a base to a subclass, but it's not a lot of benefit for allowing a new syntax combination.

Workaround:

abstract base class _RealSuperclass { ... }
sealed PublicSuperclass extends _RealSuperclass {}
// all subclasses must be `base`-or-more.

Or even

abstract base class _Based {}
sealed Superclass implements _Based { ... }

(But that is getting a little silly, might as well allow the sealed base if that's what we'll recommend doing instead.)

(I'm much more sympathetic to sealed mixin and sealed mixin class, which can be worked around too, but not in any pleasant way.)

@jakemac53
Copy link
Contributor Author

jakemac53 commented May 31, 2023

It just seems very arbitrary to me to block this particular combination - I don't think it is a crazy situation or anything. It is in fact the very first sealed class I have ever tried to write and I ran into it.

Yes there are workarounds, but I don't really see why they are necessary, given we already have the modifiers.

It was my understanding that we chose to exclude the combinations of modifiers that we thought were useless, but in doing so we used the wrong table, and should add back the ones that are in fact useful, when the correct table is used (the one that includes capabilities within the current library).

@lrhn
Copy link
Member

lrhn commented May 31, 2023

Can you elaborate what you mean with "capabilities within the current library"?
(Because you always have all the capabilities inside the same library, without doing anything.)

The reasons sealed base was excluded was not because we removed it from a table. It was never in a table because we treated base and sealed as mutually exclusive modifiers which would go into the same position in the declaration.
@eernstg had a proposal to make sealed orthogonal to base/interface/final (can't find that now), but we didn't do that.
So sealed base wasn't even in/on the table to begin with.

Should it have been? (Well, Erik thought so. He didn't manage to convince everybody else.)

We did focus mainly on the effect of the modifiers on other libraries because the modifiers doesn't restrict the current library anyway. If you can express all the effects you want on other libraries, you can write a library which satisfies that.
Sometimes it means adding base to all the subclasses of a sealed class. But there is nothing you can do by writing sealed base that you can't also achieve without it, by editing only the same library.

(Not saying I'm against allowing sealed base mixin class declarations. Just saying why we didn't consider them necessary originally. I'm usually in the "allow all the combinations" camp, although sealed base wasn't something I originally thought necessary.)

@eernstg
Copy link
Member

eernstg commented Jun 1, 2023

I think we're in a perfectly good position to allow additional combinations of keywords (following the compositional semantics that we already have). As @lrhn mentioned, I argued already a while ago that the ability to mark a sealed class as base as well could be useful, cf. #2755 (comment), section 'Sealedness'.

We might want to wait a bit and see how the usage evolves, but if we decide at some point that the introduction of keyword sequences like sealed base class is a good idea then I don't see any technical difficulties or ambiguity issues with that.

.. we treated base and sealed as mutually exclusive modifiers which would go into the same position in the declaration

This might have been a visual preference, I don't see any technical reason to consider them mutually exclusive.

@lrhn
Copy link
Member

lrhn commented Jun 1, 2023

It's quite possible that sealed base is the outlier here, because that base is only aimed at the current library.

The original argument was that sealed was mutually exclusive with interface, final and base because it already restricted other libraries as much as any of them, so adding sealed to another modifier wouldn't change anything.
That's not entirely true after we made base be contagious, a sealed base has a different effect than just sealed inside the same library too. Not an effect you can't get anyway, by manually putting base on all subclasses, but nevertheless a difference.
Still, the extra modifier only affects the current library, and we generally considered modifiers as relevant to other libraries.

I'd be very surprised to ever see sealed final class being considered useful, because a final aimed only at the current library makes no sense for a class that must be subclassed to be useful. There is no difference between sealed base and sealed final in effect (both currently requires subclasses to be base-or-more, and does nothing more inside the same library, and the sealed prevents the declaration from being used from outside of the library.)

And sealed interface is nothing but documentation, it has no effect whatsoever. Being interface doesn't restrict direct subclasses in the same library in any way, or transitive subclasses at all, and sealed prevents direct subclasses in other libraries, so the interface modifier has zero effect on anything.

That is: sealed base or sealed final has one effect: It forces immediate subclasses in the same library to be base. We only need one, and base is the more reasonable one since there will be subclasses. So the only combination which can reasonably be argued for is sealed base.

I can see sealed base class and sealed interface class being allowed as documentation, but not really for effect.
(sealed mixin and sealed mixin class are more directly useful.)

The argument for allowing these anyway would be orthogonality, so that if an author chooses to write it it, we can say that it works as they (hopefully) think it does, and then recommend that they remove the unnecessary words.

@rrousselGit
Copy link

For reference, I personally have a usecase for sealed mixin class.

I have a code generator which wants the annotated class to be declared as "mixin" because the generated code uses "with". But I also want to enable the class to be marked as sealed.

@jakemac53
Copy link
Contributor Author

(Because you always have all the capabilities inside the same library, without doing anything.)

Well that isn't entirely true, because base is still contagious within the current library. If everything else can be violated in the same library without any explicit action that seems unfortunate tbh, but the ship has sailed.

I understand we are also considering dropping the contagiousness of base in the current library but I hope we don't do that, or at least require some explicit marker to do it instead of implicitly allowing it.

@munificent
Copy link
Member

munificent commented Jun 9, 2023

We made a deliberate choice to not have class modifiers that let you control capabilities within a library. It's entirely reasonable and meaningful to have them, but it means even more modifiers and more combinations and more complexity. Since a library is all "your code" anyway, I don't think modifiers or combinations of modifiers that are only helpful within a library carry their weight.

I think of a library as the "unit of maintenance". If your library is so complex that you need to start using language features to enforce invariants within it in order to maintain it, that's a hint that you might want to split it into multiple libraries. (Though I realize sealed does interfere with that.)

For sealed base in particular, I think it isn't worth it because it adds so much confusion to users outside of the library. If they see a class marked base, we want them to think "I can extend this". But in this case, the sealed would cancel out the meaning of the other modifier. This is the same reason we disallow sealed mixin. That combination has entirely valid semantics that are even potentially useful, but it's just really confusing outside of the library because it says mixin but you can't mix it in.

@rrousselGit
Copy link

rrousselGit commented Jun 9, 2023

Note that my personal use-case for sealed mixin is code-generators.

I expect the input of my code-generators to be "mixin" classes. This is necessary because I have some multiple-inheritance scenarios, where "extending" the annotated classes won't be possible.
But I also want to support sealed class at the same time.

I don't see how I could do that without sealed mixin.

@rrousselGit
Copy link

In code, this means I want folks to be able to write:

@freezed
sealed mixin class Example implements _$Example {
  factory Example.a() = A;
  factory Example.b() = B;

  void doSomething()  => print('Hello');
}

@freezed
sealed mixin class Another implements _$Another {
  factory Another.a() = A;

  void anotherMethod() {}
}

where A/B are code-generated and ends-up being:

class A with Example, Another {}

clas B with Example {}

which enables:

switch (Example.a()) {
  case A(): print('first');
  case B(): print('second');
}
...
A()
  ..doSomething()
  ..anotherMethod();

@jakemac53
Copy link
Contributor Author

Since a library is all "your code" anyway, don't think modifiers or combinations of modifiers that are only helpful within a library carry their weight.

I disagree wholeheartedly with this assertion. Libraries are not written by a single person who maintains them forever. And even if they were, when you come back to your own code years later you won't remember anything about it anyways.

The more assumptions/intentions about my code that I can express statically as opposed to in a comment that nobody will read, the better.

I think of a library as the "unit of maintenance". If your library is so complex that you need to start using language features to enforce invariants within it in order to maintain it, that's a hint that you might want to split it into multiple libraries. (Though I realize sealed does interfere with that.)

I don't think the language should be designed in such a way that it assumes all libraries are small. There are valid reasons to have larger libraries, in particular sealed is a good example, we essentially force large libraries.

For sealed base in particular, I think it isn't worth it because it adds so much confusion to users outside of the library. If they see a class marked base, we want them to think "I can extend this". But in this case, the sealed would cancel out the meaning of the other modifier.

I really don't think this is confusing, base doesn't mean "can be extended" - all classes can be extended by default. All modifiers other than mixin impose restrictions on usage. Those restrictions compose together totally sensibly in this case: base means it can't be implemented and sealed means all direct subtypes are statically known (and defined in the same library). What this tells you is all the sealed subtypes can themselves only be extended due to base transitivity.

@munificent
Copy link
Member

Since a library is all "your code" anyway, don't think modifiers or combinations of modifiers that are only helpful within a library carry their weight.

I disagree wholeheartedly with this assertion. Libraries are not written by a single person who maintains them forever. And even if they were, when you come back to your own code years later you won't remember anything about it anyways.

The more assumptions/intentions about my code that I can express statically as opposed to in a comment that nobody will read, the better.

Sure, but there is a cost in language complexity for adding more ways to express those. For example, we currently only have library privacy. We could add class privacy and even instance privacy and that might help intra-library maintenance. But is the complexity or more privacy levels worth it?

For sealed base in particular, I think it isn't worth it because it adds so much confusion to users outside of the library. If they see a class marked base, we want them to think "I can extend this". But in this case, the sealed would cancel out the meaning of the other modifier.

I really don't think this is confusing, base doesn't mean "can be extended" - all classes can be extended by default. All modifiers other than mixin impose restrictions on usage. Those restrictions compose together totally sensibly in this case: base means it can't be implemented and sealed means all direct subtypes are statically known (and defined in the same library). What this tells you is all the sealed subtypes can themselves only be extended due to base transitivity.

We have discussed this at length in #2723 and #2595 (and probably elsewhere). You can reason precisely about the capability modifiers in terms of subtracting capabilities (which is what they actually do in terms of semantics). But the words we chose for them were deliberately chosen to enable users to reason about them in terms the positive capabilities they allow.

If we wanted them to think about negative capabilities, then interface is a bad name because it has absolutely nothing to with extension, which is the capability it removes.

It's definitely not a perfect design but it's the best set of words and semantics that we could come up with after beating on it for a very long time.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Jun 12, 2023

Sure, but there is a cost in language complexity for adding more ways to express those. For example, we currently only have library privacy. We could add class privacy and even instance privacy and that might help intra-library maintenance. But is the complexity or more privacy levels worth it?

In this case we already have the modifiers, and the existing meaning of the modifier is consistent with what I think it should mean, so the complexity should be minimal, otherwise I would agree. I wouldn't be surprised if it actually made the code less complex because not allowing it probably introduces more implementation complexity? The main complexity would probably be in language tests.

In terms of user complexity, I think it is definitely more complex to have certain combinations that are not allowed, and seem arbitrarily banned from a user perspective. There is no real reason as a user to expect this combination of modifiers to be banned so it's a surprising thing you have to learn.

But the words we chose for them were deliberately chosen to enable users to reason about them in terms the positive capabilities they allow.

I still don't think sealed base is confusing even if reasoned about in terms of the user perceptible value they provide? In effect it is just a sealed class whose subtypes you must extend if you want to implement them.

@lrhn
Copy link
Member

lrhn commented Jun 12, 2023

If we wanted them to think about negative capabilities, then interface is a bad name because it has absolutely nothing to with extension, which is the capability it removes.

I'll admit that I read the "positive" modifiers negatively too, reading interface as "only as an interface, so only implement, not anything else".
That's the most consistent way I've found to think about the modifiers, when no modifier is completely permissive. Every modifier restricts away from the base of allowing everything.

I still like the positive modifier names, but that's because they do say what you are limited to, rather than saying what you can't. Then you'd have to think what's left. The final works as negative, because there is nothing left.

But it's still a restriction, and if I see two restrictions, I have no problem combining them and applying both. In most cases, I don't even need that, because the most restrictive one comes first. If I see sealed base, I just see sealed first, and think "not for me". I never get to the base.

(That's also why I personally have no problem with interface mixin class or final mixin either, the most restrictive modifiers - applied to code from other libraries - come first, so I never need to look at the rest. Those other modifiers are not for me. And mixin is not a modifier, it's a declaration kind 😉.)

@eernstg
Copy link
Member

eernstg commented Jun 13, 2023

@jakemac53 wrote, responding to @munificent:

I still don't think sealed base is confusing

Agreed.

@lrhn wrote:

I'm usually in the "allow all the combinations" camp

and also:

abstract base class _Based {}
sealed Superclass implements _Based { ... }

(But that is getting a little silly, might as well allow the sealed base if that's what we'll recommend doing instead.)

So let's allow all the combinations! 😄

@munificent
Copy link
Member

In this case we already have the modifiers, and the existing meaning of the modifier is consistent with what I think it should mean, so the complexity should be minimal, otherwise I would agree. I wouldn't be surprised if it actually made the code less complex because not allowing it probably introduces more implementation complexity? The main complexity would probably be in language tests.

I'm not particularly worried about the implementation complexity. I'm much more worried about user confusion when looking at an API. If they're reading the docs for some API and they see a class marked base, it currently sends an unequivocal signal "Ah, I'm allowed to extend this." If we allow sealed base, then they have to now understand that base no longer means that. It's an implementation detail of the library that has leaked out to its declaration signature.

I'd rather have a @internallyBase annotation and a linter rule to enforce what you're looking for here because then the checking is encapsulated within the library where it matters.

But this is also not a hill I will die on. If you think this is really useful... I guess it's OK?

@lrhn
Copy link
Member

lrhn commented Jun 15, 2023

a class marked base, it currently sends an unequivocal signal "Ah, I'm allowed to extend this." If we allow sealed base, then they have to now understand that base no longer means that. It's an implementation detail of the library that has leaked out to its declaration signature.

True, but it would then allow having that implementation detail to begin with, which we prevent today.

We think can (and should) work on the leaking, even with the features we have today.
Even if it's only in the generated DartDoc, that still counts for something.

Showing sealed class Foo is already over-sharing, because you don't need to know that it's a class, since you can't extend it. If we ever allow (which there are somewhat reasonable reasons for) a sealed mixin or sealed mixin class, which is needed in order for the local library to use it as a mixin, then it's still unnecessary to tell anyone whether it's a class, mixin or mixin class. It's a sealed type, which is all other libraries need to know.

So if DartDoc doesn't shown anything except sealed Foo for abstract sealed base mixin class Foo, then it's giving all the necessary information, and nothing more.

If a user looks directly at the declaration, then sealed is the most important word. It's also the first modifier.
They can stop reading when they get to that. Context matters. Seeing base mixin class only matters if they apply to you, and they don't if there is a sealed in front.

Or if there is a final in front. Or an interface in front of a mixin class.

If you can't extend or implement, you don't need to know whether something is a class or mixin to begin with.

So, what if DartDoc shows (plus abstract if applicable):

  • class Foo for a no-modifier class.
  • mixin class Foo for a no-modifier mixin class.
  • mixin Foo for a no-modifier mixin.
  • base class for a base class.
  • base mixin class for a base mixin class.
  • base mixin for a base mixin.
  • interface Foo for any interface declaration (whether class, mixin class or mixin).
  • final Foo for any final declaration.
  • sealed Foo for any sealed declaration.

That should cover all the available operations in other libraries. Any more information is leaking implementation details.

And people reading the code can also stop at the first sealed, final or interface, because that uniquely defines what you can do with the declaration (nothing, nothing and implement).

It's only if you see no modifier or base that you should read further, because then you need to see whether you can extend (class) and/or mix in (mixin).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-modifiers-later Issues related to "base", "final", "interface", and "sealed" on classes and mixins, tbd later request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

5 participants