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

What keyword should we use for exhaustive class hierarchies? #2594

Closed
munificent opened this issue Oct 28, 2022 · 20 comments
Closed

What keyword should we use for exhaustive class hierarchies? #2594

munificent opened this issue Oct 28, 2022 · 20 comments
Labels
patterns Issues related to pattern matching.

Comments

@munificent
Copy link
Member

As part of pattern matching, we want to be able to mark a class or mixin as having a known exhaustive set of direct subtypes. That way, if you match all of those subtypes, you know the supertype is covered. We've gone back and forth over the keyword for this. My initial pitch was switch. In the more recent PR, I proposed sealed.

For reference, here's what some other languages do:

  • In C#, sealed on a class means the class can't be extended. (It's like final in Java.) C# has no notion of exhaustive subtypes.

  • In Java, a class or interface can be marked sealed and then there is a separate permits clause that lists the allowed subtypes, which are then used for exhaustiveness. The subtypes of a sealed type must themselves be marked either sealed or non-sealed to indicate whether they allow further extension.

  • Kotlin uses sealed to mark a class as having a known set of subtypes for exhaustiveness.

  • Scala also uses sealed for the same purpose.

  • TypeScript doesn't have any notion of exhaustiveness checking right now.

  • Swift uses separate enum types for exhaustiveness and doesn't build it on top of subtyping.

So, basically, except for C#, every language that uses sealed uses it to mean what we would have it mean for Dart: That the type has a known set of subtypes used for exhaustiveness checking.

As @mit-mit has pointed out, even C#'s use is not too far off, because a type marked sealed in Dart is also prevented from being extended (or implemented) outside of the library where it's defined.

So, while I think switch is a neat idea, I believe sealed will be easiest for users to learn and understand. But I'm filing this issue so we can discuss it more.

@munificent munificent added the patterns Issues related to pattern matching. label Oct 28, 2022
@kallentu
Copy link
Member

I'll put my two cents in for the keyword naming.

When I first heard of the feature, I was introduced to it as sealed classes and I think most people I've talked to refer to it as such.

However, the initial strawman calls it switch classes and I had a hard time wrapping my head around that.
I understand that switch points to when we actually use these types and the exhaustiveness checking we get from the modifier, but I was pretty confused and I imagine for someone first using this feature, they would be too.

And with sealed, it's a little better in that you can kinda interpret what it means by looking at it. I think there's more intuition of what it does. Albeit, I admit that when I heard about sealed classes, I assumed it was some sort of transitively sealed hierarchy and later realized my understanding of what sealed is was wrong (being exhaustiveness checking on the explicitly sealed class' subtypes).

This is a shot in the dark and not that this keyword is better than what we already have, but why did we not use exhaustive or something similar? It is a tad longer, but every time we explain sealed or switch, we really just talk about the exhaustive checking properties. (I will admit that other languages use sealed so I can see how we would want to stick with what our developers are familiar with)

TLDR;; Why not exhaustive? Otherwise, I like sealed much better than switch because the keyword more intuitively describes the behaviour of the class rather than the usage of the class, and it seems commonly used by other languages that have a similar feature.

@lrhn
Copy link
Member

lrhn commented Oct 28, 2022

One problem with sealed is that Dart already has @sealed, which is the transitive cannot-implement-or-extend restriction, not the one-level one used for exhaustiveness.
When I read sealed, that's what I think of, and that's how we have used the word "sealed" in other discussions too (#11, #704, etc.).

I fear we're going to confuse our users (and ourselves) if "sealed" starts meaning something else.
If we think we can sway the meaning towards exhaustiveness, then the word itself is not inherently bad. Slightly misleading, IMO, since it's not transitive, and I wouldn't expect that you can be just a little sealed. The word doesn't actually describe what it does, since it can equally well describe the transitive sealing. It's just a word. I'd probably like "closed" better, for no rational reason.

@leafpetersen
Copy link
Member

I added a counter-proposal here which uses closed for exhaustiveness.

@munificent
Copy link
Member Author

One problem with sealed is that Dart already has @sealed, which is the transitive cannot-implement-or-extend restriction, not the one-level one used for exhaustiveness.
When I read sealed, that's what I think of, and that's how we have used the word "sealed" in other discussions too (#11, #704, etc.).

I did a search of ~50k files in Pub packages, and I found 207 matches across 166 files, so it doesn't seem to be super widely used. If familiarity is the goal, we will probably get more mileage using sealed since that keyword exists in Kotlin, Java, and Scala with the same meaning I propose here (including not being transitive).

Slightly misleading, IMO, since it's not transitive, and I wouldn't expect that you can be just a little sealed.

It's not transitive in Kotlin, Scala, or Java either. It would be more misleading if we used sealed and made it transitive, since it isn't in the other languages that use that work for exhaustiveness checking.

Honestly, I think most of what's going on here is that folks are feeling surprised that the using-sealed-subtypes-for-exhaustiveness feature is not transitive. I was surprised when I first realized the other languages work that way too. But that is how they work, and once I thought about it, I realized it makes sense. Once I got used to the idea, using sealed and not being transitive makes sense to me.

@leafpetersen
Copy link
Member

It's not transitive in Kotlin, Scala, or Java either.

I think this is quite misleading, at least WRT Kotlin, because all classes in Kotlin are not available for subclassing or implementation unless otherwise specified. So while it is true that the exhaustiveness checking part is not transitive (which I don't think anyone is proposing it should be), the sealed for subclassing part is of course transitive, simply because that's always the default.

@munificent
Copy link
Member Author

I think this is quite misleading, at least WRT Kotlin, because all classes in Kotlin are not available for subclassing or implementation unless otherwise specified.

Yes, but that's orthogonal to exhaustiveness checking, so I'm not sure how it's relevant.

So while it is true that the exhaustiveness checking part is not transitive (which I don't think anyone is proposing it should be),

I thought that was exactly what @lrhn meant when he said "I think people will expect sealed to mean that nobody can create instances implementing the type, not just that nobody can create immediate subtypes.".

the sealed for subclassing part is of course transitive, simply because that's always the default.

I think transitivity is independent of the default. Kotlin defaults to non-extensible but that doesn't mean that the extensibility of a class is a transitive property of it. As I understand it, subclasses don't inherit the extensibility of their superclass. So if you extend a class marked open, your class still defaults to closed. If extensibility was transitive, I would expect that to mean that inheriting from an open class makes your class open too.

I think maybe we're talking past each other? I intended this issue to be about the exhaustiveness-checking-thing, not the preventing-subclassing-thing.

@munificent
Copy link
Member Author

This is a shot in the dark and not that this keyword is better than what we already have, but why did we not use exhaustive or something similar?

I don't know if we ever really thought about that. It is kind of long (and hard to spell), but you have a good point that it's pretty unambiguous.

@leafpetersen
Copy link
Member

leafpetersen commented Oct 29, 2022

Yes, but that's orthogonal to exhaustiveness checking, so I'm not sure how it's relevant.

In your proposal, sealed does two things: it does the things necessary to make exhaustiveness checking sound (closed set of direct subtypes), and it enables exhaustiveness checking. The latter is important because you also allow doing all of the things necessary to make exhaustiveness checking sound (abstract closed base class) without enabling exhaustiveness checking.

I don't think anyone is arguing (unless I'm very confused) that the enables exhaustiveness checking part should be transitive. The discussion is around whether the "no subtypes outside of the library" part should be transitive. And my point is that saying "no it shouldn't because Kotlin" is not a reasonable counter-argument. In Kotlin, "no subtypes outside of the library" is what you get, unless you explicitly choose otherwise, so there's no need to make it transitive - the behavior that @lrhn would prefer to be transitive is the behavior you already get (so it's trivially transitive).

I thought that was exactly what @lrhn meant when he said "I think people will expect sealed to mean that nobody can create instances implementing the type, not just that nobody can create immediate subtypes.".

I can't speak for him, but again, my interpretation is that he is speaking to the "no subtypes" bit, not the "enable exhaustiveness bit".

I think transitivity is independent of the default. Kotlin defaults to non-extensible but that doesn't mean that the extensibility of a class is a transitive property of it. As I understand it, subclasses don't inherit the extensibility of their superclass.

I don't think transitivity is the key point. The key point is that:

  • In Kotlin, the subclass of a sealed class is non-extensible (unless opened)
  • In this proposal, the subclass of a sealed class is extensible.

To be clear, I'm not sure I'm opposed to your approach. But I think the arguments that @lrhn is making here are perfectly well-formed, and I also think they reflect a real confusion we're going to run into. It is surprising to me that subclasses of sealed classes are open. This doesn't reflect the behavior that (e.g.) Kotlin provides, not sure about the other languages. This may just be a terminology thing, hence my counter-proposal to not use sealed for this, but to instead use sealed for "transitively not allowed to subclass or implement", and a different word for "turn on exhaustiveness checking".

@munificent
Copy link
Member Author

munificent commented Oct 29, 2022

In your proposal, sealed does two things: it does the things necessary to make exhaustiveness checking sound (closed set of direct subtypes), and it enables exhaustiveness checking. The latter is important because you also allow doing all of the things necessary to make exhaustiveness checking sound (abstract closed base class) without enabling exhaustiveness checking.

Right. The way I'd describe it is sealed (or whatever keyword we pick) is how a user expresses that they intend to use this type as a supertype for exhaustive checking. Then the language applies the minimum set of restrictions necessary for that to be sound. Since exhaustiveness checking doesn't care if the direct subtypes are themselves extended or implemented, sealed doesn't prohibit it.

The high level principle is to support what the user asks for soundly while being otherwise as permissive as possible (which I think is generally in line with Dart's ethos). I assume that same reasoning is how Kotlin, Java, and Scala also ended up allowing exhaustive subtypes to themselves be extended or implemented.

There's no need to prohibit it for soundness, and if we interpret the user writing sealed to only mean they are asking for exhaustiveness checks, then adding any other prohibition seems wrong to me, since they didn't ask for it. This seems especially clear to me since sealed is what you put on the supertype. If the user has some intention about what you can do with one or more of the subtypes, they should be stating that intention on those types. (And, in particular, they may have different intentions for the different subtypes, so inheriting a set of prohibitions broadly from the supertype feels arbitrary and not necessarily helpful.)

Of course, if we interpret sealed to mean the the user is intending to turn off all the capabilities for reasons unrelated to exhaustiveness, then there's a separate discussion to have about whether that makes sense to be transitive. But for the issue here, it's about the keyword users write to mean "I want exhaustiveness checks please".

@leafpetersen
Copy link
Member

leafpetersen commented Oct 29, 2022

The high level principle is to support what the user asks for soundly while being otherwise as permissive as possible (which I think is generally in line with Dart's ethos). I assume that same reasoning is how Kotlin, Java, and Scala also ended up allowing exhaustive subtypes to themselves be extended or implemented.

I'm really, really baffled here. Kotlin does not do what you describe. It simply, flatly, does not. If you try to extend a subclass of a sealed class, you will get an error. You can, of course opt into the behavior you describe by post-hoc opening the otherwise non-extensible subclass, but it is flat out wrong to say that Kotlin is choosing to be as permissive as possible here.

[Update] I looked into Scala and Java. Scala does do what you describe. Java does not - it requires you to explicitly choose a modifier for subclasses of sealed classes.

@leafpetersen
Copy link
Member

There's no need to prohibit it for soundness, and if we interpret the user writing sealed to only mean they are asking for exhaustiveness checks, then adding any other prohibition seems wrong to me, since they didn't ask for it.

Stepping back a bit, I'm broadly ok with this semantics. I don't think it's as clear to me that extensibility should be opt out rather than opt in as it is to you, but I'm basically fine with it. But I do have a lot of concerns about the choice of keyword here. The semantics you propose matches the Scala semantics, but they do not match Kotlin and Java, nor do they really line up with the use of sealed in C#. I really have a lot of concerns about the potential for user confusion here.

@leafpetersen
Copy link
Member

In looking over this, I feel like perhaps where we're talking past each other is on the question of "what is possible to do" vs "what are the defaults"? So let me try to lay this out as precisely as possible. I will use the term "enumerated family of types" for a set of types which is known to be the complete set of subtypes of a single supertype.

The first question is, should it be possible to have a an enumerated family of types, for which members of the family themselves have subtypes (outside of the current library/package). The answer to this, if we go by other related languages, is a resounding yes as far as I can tell. Kotlin, Java, and Scala all provide some affordance for doing this. I can't speak for @lrhn , but I personally have no concerns whatsoever with providing this affordance.

The second question is, what should be the default behavior when you try to add a subtype of a member of a family of enumerated types. On this question (which I have been assuming was the fundamental question that we were asking, but perhaps this is where the confusion arises?) there is no consensus among the languages under consideration. Specifically, here is my understanding, in table form (using "EF" as a short form for "Enumerated Families", and "FM" as a short form for "Family Member", meaning one of the enumerated subtypes of the base type of the enumerated family).

Language syntax for EF default extensibility of FM syntax for "closing" an FM syntax for "opening" an FM
Scala sealed class extensible final class N/A
Kotlin sealed class non-extensible N/A open class
Java sealed class None (must specify) final class non-sealed class
Dart (proposed) sealed class extensible closed base class N/A

My take away from this is that WRT defaults, there's no clear signal from other languages unless we weight some languages more heavily than others (which, given the relative usage of Java + Kotlin, perhaps we should?). All of the languages use the keyword sealed, but each one takes a different position on FM extensibility. Scala makes it the default, Kotlin makes non-extensible the default, and Java has no default and requires the developer to choose.

This is a troublesome chart for me.

  • The fact that there is clear consensus towards using sealed points towards using sealed.
  • The fact that in only one other language (Scala, which is fairly niche) is class FamilyMember extends Family both valid syntax and something that means "FamilyMember is extensible" concerns me a lot.

The question I am struggling with is, how likely is it that the casual reader of code will draw the right conclusions.

In the Kotlin case, I'm fairly confident that they will guess correctly. If they see sealed class Family followed by class FamilyMember they will correctly guess that FamilyMember is not extensible. If they see open class FamilyMember instead, I would imagine they will think something like "Oh, how interesting, I didn't know that was possible".

In the Java case, likewise, the user will definitely guess correctly (at the expense of verbosity). The subclass will just tell them "I am not sealed" or "I am final".

The Scala (and proposed Dart) case seems to me the most likely to lead the user astray. They see sealed class Family followed by class FamilyMember and perhaps they think "Ah, as in Kotlin, we have a sealed family with a non-extensible subclass". Or perhaps they think "Ah, I don't know what this means, but I know that in C# sealed means no subclasses so... something"? I don't know where the user ends up here. And this pushes me somewhat in the direction of saying "no, let's just use a different word here entirely, to avoid a false cognate". But this is a weak preference, I don't have high confidence here.

There is a somewhat separate issue from the choice of syntax, which is whether or not Dart/Scala is correct in choosing to make "extensible" the right default for the FM or not. You seem to feel fairly strongly that it is based on the general permissiveness of Dart, which I hear - it's a valid "style" point. I am a bit surprised by this though, given the empirical data you have from corpus analysis is that the vast, vast majority of classes are not extended or implemented. I think it's worth thinking hard about whether "extensible with opt out" (the Scala approach) is really the correct default over "non-extensible with opt-in" (the Kotlin approach).

@munificent
Copy link
Member Author

I really appreciate your comment. I think I understand what you're saying better now.

Kotlin does not do what you describe. It simply, flatly, does not. If you try to extend a subclass of a sealed class, you will get an error.

Yes, but that has nothing to do with sealed. If you try to extend a subclass of a non-sealed class, you also get an error. You get an error because classes can't be extended unless they are marked open. If Kotlin didn't even have sealed and exhaustiveness checks, you would get an error if you tried to extend a class. It's an orthogonal feature in terms of semantics.

It may be that in terms of user experience they designed sealed and open to work in concert such that they default to non-extensible partially because they feel that's the right default when combined with sealed. I don't know. My point was just that adding sealed to a class does not make it an error to extend it. Failing to add open is what makes it an error.

My take away from this is that WRT defaults, there's no clear signal from other languages unless we weight some languages more heavily than others (which, given the relative usage of Java + Kotlin, perhaps we should?).

Maybe, but another way we could write that table is:

Language syntax for EF default extensibility of FM
Scala sealed class same as default extensibility of class without FM
Kotlin sealed class same as default extensibility of class without FM
Java sealed class None (must specify)
Dart (proposed) sealed class same as default extensibility of class without FM

As far I can tell, except for Java, other languages don't change a class's extensibility from what it would be if you hadn't put sealed. It's just that Kotlin picks a different general default for extensibility. Java is an interesting one where it seems they were particularly worried about users intuiting a wrong default so they force users to explicitly pick.

I am a bit surprised by this though, given the empirical data you have from corpus analysis is that the vast, vast majority of classes are not extended or implemented.

Yes, but when we asked (a relatively small, biased sample of) users, they generally preferred Dart's permissive defaults in spite of that. And we have ~a decade of empirical evidence that it can't be that bad of a default because flipping it has never risen high enough in priority relative to other language features to make us doing anything about it. This is in contrast to other flips we have done like a sound type system, implicit downcasts, and nullability where we did flip the default (with all of the painful migration that entails) because it was clear from users that the default was wrong.

Permissive extensibility and implementability seems to be a pretty good default for Dart, at least when exhaustiveness checking is not involved. It may be that once a class is sealed then we should flip the default at that point for everything under that type on the hierarchy. I'll discuss that more over on #2595.

@leafpetersen
Copy link
Member

As far I can tell, except for Java, other languages don't change a class's extensibility from what it would be if you hadn't put sealed. It's just that Kotlin picks a different general default for extensibility.

Yes, this is a reasonable point, but presenting the table that way disguises my basic concern with the choice of keyword here, which is that for Kotlin, the general default for extensibility lines up with the common intuition of what sealed means. For Scala, Java and Dart, the general default for extensibility does not line up with the common intuition of what sealed means. Hence, I suspect, Java's choice to make the user be explicit. And hence my thought that perhaps we should consider a different keyword, so that the cognitive dissonance between the implication of the keyword and the actual semantics.

Again, I'm not strongly opposed to using the sealed keyword here. I just want to be sure we're really thinking through the choice here as it manifests itself in Dart.

@munificent
Copy link
Member Author

which is that for Kotlin, the general default for extensibility lines up with the common intuition of what sealed means.

Yes, that's fair. I can see how sealed could be confusing. Though note that even in Kotlin, the whole point of putting sealed on a type is to subtype it, just in the same library. So it's not like it means "this type right here that I put sealed on is totally locked down" like, say, final in Java (or sealed in C#, confusingly).

So, uh... How about switch or exhaustive? :)

@leafpetersen
Copy link
Member

Though note that even in Kotlin, the whole point of putting sealed on a type is to subtype it, just in the same library.

Yes, this is true, and one of the reasons that I hesitate here. If you're coming from C#, you're going to be confused no matter what. And if you're coming from Kotlin/Java/Scala, then we're trading off confusion of learning a new keyword against possible confusion around the semantics of the subclasses. Maybe it's better to leverage the familiarity of the keyword?

Another point is that it's probably not the end of the world if users do get confused. Most users probably don't care. And if they do actually care, well, then they need to learn to mark each subclass as closed base. Or with #2595 they could mark the superclass as final sealed.

So, uh... How about switch or exhaustive? :)

I think I prefer sealed to either of these. I'm on the fence about whether I like closed better than sealed. I also somewhat like enumerated but it's probably also confusing because of enum.

@leafpetersen
Copy link
Member

leafpetersen commented Nov 1, 2022

So, uh... How about switch or exhaustive? :)

I think I prefer sealed to either of these. I'm on the fence about whether I like closed better than sealed. I also somewhat like enumerated but it's probably also confusing because of enum.

What do you think about variant class? It's very direct about what capability it's providing, and there is prior art. It's a common name for tagged unions.

variant class Option<S>;
class Some<S> implements Option<S> {
  S value;
  Some(this.value);
}
class None implements Option<Never> {
  const None._();
  factory None() => const None._();
}

@munificent
Copy link
Member Author

"Variant" isn't doing anything for me. It feels like it emphasizes the wrong thing. Every type can have varying subtypes in Dart. The special thing about these ones is that they vary less than other ones.

@Wdestroier
Copy link

Wdestroier commented Nov 5, 2022

I don't see any advantage to add the "closed" or "final" class modifier to close a FM. The reasons why someone wants to implement a class may not be known beforehand.
Edit: I have read about making sure a class has a specific behavior that can't be changed by others. A valid use case I may imagine that can be achieved by implementing the class is to log an information and (delegate) call the original method. Or mock/fake a base class when unit testing.

@munificent
Copy link
Member Author

Closing this issue since we've pretty well settled on sealed.

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

5 participants