Allow subclassing a sealed class outside the library with an abstract class about language

[eernstg]

Agreed, @rrousselGit, breakage of the form "you need to add a case for Foo() to this switch expression" can be good breakage. I don't think an overall rule like "avoid breaking changes" should be used to justify an approach where the program compiles after the upgrade, but then crashes now and then.

[scheglov]

If we merge interface and implementation classes, we would start using sealed.
https://dart-review.googlesource.com/c/sdk/+/308402

[rrousselGit]

it would be confusing to have to cover A, B, and SealedImpl in any exhaustive switch.

Couldn't SealedImpl be made a mixin such that the only real subclasses in the implementation file are AImpl/BImpl?

Such that we'd have:

// foo.dart
sealed class Sealed {
  int get offset;
}
abstract class A implements Sealed {
  DartType get type;
}
abstract class B implements Sealed {}


// foo_impl.dart
mixin _SealedMixin { // Not using "on" clause as it's not allowed to do "on SealedClass"
  // We duplicate the properties we care about in the mixin
  int get offset;

  // The mixin can use the content of the sealed class
  void someUtils() => print(offset);
}

class _AImpl extends A with _SealedMixin {
  @override
  final int offset;
  @override
  final DartType type;
}

class _BImpl extends B with _SealedMixin {
  @override
  final int offset;
}

There's a bit of duplication, so this is not perfect. But that would enable to use of the sealed keyword without any language modification or relying on parts.

[eernstg]

we have a lot of sealed-in-spirit class hierarchies

I think it would make sense to enhance the terminology a bit in this area: Assume that a library L defines a maximal set of classes C1 .. Cn that are subtypes of a given root C, and each of these classes have the modifier sealed, and every class on every superinterface path from Cj to C is one of the classes C, C1 .. Cn. In this case we say that {C, C1 .. Cn} is a sealed hierarchy. There may be any number of subtypes of each member of the sealed hierarchy in L, but the point is that every class in the sealed hierarchy is an always-exhaustive type (and we can find a minimal exhaustive set of subtypes for every type D in C, C1 .. Cn by finding all direct subtypes of D in L).

In the example here, the intended sealed hierarchy is just the single class Sealed, as far as I can see. But I would assume that many subtype hierarchies with a sealed class S at the top would actually have a larger sealed hierarchy below S. The conceptual justification would be that every node in the sealed hierarchy plays the role as one member of a well-known set of "sibling" classes. In the example, class Sealed is known to consist of A and B; but, presumably A could be sealed and it could consist of A1 and A2 and A3, and similarly for B.

If the sealed hierarchy is indeed just one class then we should be able to create an Impl structure as follows:

// lib/foo.dart

sealed class Sealed {}
abstract class A implements Sealed {}
abstract class B implements Sealed {}

// lib/src/foo.dart <-- implementation private!

abstract class SealedImpl {} // Could be a mixin, as @rrousselGit mentioned.
class AImpl extends SealedImpl implements A {}
class BImpl extends SealedImpl implements B {}

SealedImpl can provide whatever is needed in terms of member implementations, and the fact that we got them all (in order to satisfy implements Sealed, which we can't actually write any more) is checked indirectly: Any missing implementations in SealedImpl will just translate into missing implementations in AImpl and/or BImpl.

In other words, there is no problem as long as the sealed hierarchy is a single class.

So let's assume that the sealed hierarchy is (at least) Sealed, A, and B.

// lib/foo.dart

sealed class Sealed {}
sealed class A implements Sealed {}
sealed class B implements Sealed {}
class A1 implements A {}
class A2 implements A {}
class B1 implements B {}
class B2 implements B {}

// lib/src/foo.dart <-- implementation private!

// Create a shadow of the sealed hierarchy. We don't get to declare subtype relationships,
// because that's an error, but we do provide implementations of various members.
abstract class SealedImpl /*would implement Sealed*/ {}
abstract class AImpl extends SealedImpl /*would implement A*/ {}
abstract class BImpl extends SealedImpl /*would implement B*/ {}

// Just below the sealed hierarchy we can have concrete classes,
// and at this point they can have the "correct" type.
class A1Impl extends AImpl implements A1 {}
class A2Impl extends AImpl implements A2 {}
class B1Impl extends BImpl implements B1 {}
class B2Impl extends BImpl implements B2 {}

It's inconvenient that we cannot declare the subtype relationships from SealedImpl to Sealed, from AImpl to A, and so on, but there is no real danger in that: The fact that A1Impl actually implements A1 will provide all the needed subtype relationships, and it will give rise to compile-time errors if any members do not have an implementation, or they have a signature which is not a correct override of the member signature in the sealed hierarchy.

It may not be pretty, but I do think that it would work.

[srawlins]

Sorry, I believe it is useful (one might say imperative) to have SealedImpl implements Sealed. To flesh it out a bit more:

// lib/foo.dart

abstract class Sealed {
  Iterable<SyntacticEntity> get childEntities;
}

// lib/src/foo.dart <-- implementation private!

abstract class SealedImpl implements Sealed {
  @override
  Iterable<SyntacticEntity> get childEntities => _childEntities.syntacticEntities;
}

The implements Sealed provides all the usual guards, that members have the right types, the right names, etc.

WRT to my proposed subclassing rules, would they be technically difficult? Hinging on finding the sealed family when only given abstract class C2 implements C1 and a sealed C1?

[eernstg]

@srawlins wrote:

The implements Sealed provides all the usual guards

Indeed, but those checks will be performed anyway when AImpl is checked because AImpl extends SealedImpl and implements A, and A implements Sealed (and similarly for A2Impl, A2, etc. in the other example).

However, it gets much more interesting!:

WRT to my proposed subclassing rules, would they be technically difficult?

I think I understand it now (but a couple of things were confusing along the way). Here is the rule:

• Given a sealed class C1 declared in library L1, and given a class C2 declared in library L2, which has C1 as a supertype (however indirectly), and given L1 != L2:
  • If C2 is abstract (either sealed or abstract), we do not have a compile-time error.
  • If C2 does not directly subclass any of the classes in the sealed family of C1, we have a compile-time error.

I think the second rule ought to be more flexible (it's much stricter than the current rules because it requires, e.g., that concrete subtypes of a sealed type must be direct subclasses of the direct subtypes of the sealed class, IIUC).

Anyway, the fundamental idea, as I understand it, is that we allow the same out-of-library subtypes of a sealed class as we do today, and then we also allow some extra out-of-library abstract direct subtypes of the sealed class.

Then we add an extra constraint, in order to ensure that soundness isn't violated. The point is that no actual object will ever have one of those new abstract types without also having one of the types that we're currently considering to be the exhaustive set (that is, the direct subtypes of the sealed class C1 that are declared in L1).

The current rule in the feature specification is as follows:

It's a compile-time error if [..] a declaration D from library L has a direct superdeclaration S marked sealed [..] in a library different from L.

We would replace that rule by this one:

It's a compile-time error if [..] a concrete declaration D from a library L has a (direct or indirect) superdeclaration S marked sealed [..] in a library different from L, and D does not have a (direct or indirect) superdeclaration U from the library L such that S is a direct superdeclaration of U.

A concrete declaration is a class or mixin class declaration which isn't abstract.

We'd need to scrutinize this rule a lot, of course, and it is not obvious to me how much more work it is to check this rule in tools, compared to the current rule (which is O(numberOfSuperinterfaces)). But I do think that it would work!

[lrhn]

We would replace that rule by this one:

It's a compile-time error if [..] a concrete declaration D from a library L has a (direct or indirect) superdeclaration S marked sealed [..] in a library different from L, and D does not have a (direct or indirect) superdeclaration U from the library L such that S is a direct superdeclaration of U.

A concrete declaration is a class or mixin class declaration which isn't abstract.

(or enum declaration.)

This basically boils down to:

• All concrete subclasses of a sealed class S must be subtypes of one of the immediate subclasses of S declared in the same library as S.

That's a very direct specification of the requirement necessary for ensuring exhaustiveness, so it pretty certainly would work. Every other rule we've made is trying to enforce that restriction by disallowing some patterns (and preferably in a way where it's directly visible on a class declaration which constraints it imposes on its subclasses).

So when we say that you cannot subclass a sealed class from another library, not even with an abstract declaration, it's because would not be directly visible on that abstract subclass that there is a transitive constraint. You won't notice until you try to make a concrete subclass of the abstract class, possibly multiple classes down, at which point you're told "This concrete class must implement one of these classes: SealedSubclass1, SealedSubclass2, ..., SealedSubclassN, because it implements the sealed class Sealed".

(And then, if any of those immediate subclasses are themselves sealed, we may want to find the non-sealed frontier below the sealed classes. And maybe omit final declarations and enums, since you can't implement those anyway. The class cannot be in the same library, because then it is one of the immediate subclasses, and it implements itself, so no problem.)

We could do that, it would be sound for exhaustiveness, but it causes a lower direct readability of the classes.

(It would actually mean that declaring an abstract subclass in a separate library gives you an ability you don't have in the same library, that of creating an immediate (abstract) subclass which doesn't count towards exhaustiveness. Maybe we should introduce a way to get that without needing to be in a different library: A non-exhaustiveness-participating can directly subclass a sealed class, or another non-exhaustiveness-participating declaration, must be abstract, and any non-abstract subclass of one of those must then implement on of the actually exhaustiveness-participating subclasses of the original sealed superclass.)

This would also cover one of my original pet ideas: allowing a mixin in another library to have the sealed type as on type,. Then it had to be base to prevent implementation, but the mixing in would necessarily have to be on a subtype when you couldn't extend the sealed class directly.

[munificent]

This suggestion makes a lot of sense. Like Lasse says, I think we could do it.

At the same time, the base transitivity stuff already in class modifiers ended up becoming a spiral of complexity that really got out of hand, and we're already hearing worry from users about the increasing complexity of the language. I'm excited about all of the new features and I know a lot of users are too but... oof... the language is getting big.

Sorry, I believe it is useful (one might say imperative) to have SealedImpl implements Sealed. To flesh it out a bit more:

...

The implements Sealed provides all the usual guards, that members have the right types, the right names, etc.

Is that really so valuable? You'll already get that checking implicitly inside all of the subtypes of SealedImpl if they fail to implement the stuff inherited from Sealed in their respective subtype.

[scheglov]

FWIW, I don't want to make any API classes sealed.
This puts too much obligations on me.
So, no, I don't think that this is very valuable.

I find it very useful to use sealed classes for (so far) local, library private, classes.
https://dart-review.googlesource.com/c/sdk/+/306160/15/pkg/analysis_server/lib/src/services/refactoring/framework/write_invocation_arguments.dart#85
The theme: just because I can! :-P

So, I appreciate local type safety.
Maybe we could allow something better inside the package / module / etc.
But I'm not going to promise anything to my clients.

[srawlins]

Thanks a lot for all of the input! I like that it seems sound. I don't know of other cases that use our style of public-interface and private-impl (I mean, it's classic, I just don't think I've seen it outside our code base in a Dart package).

[rrousselGit]

FWIW, I don't want to make any API classes sealed. This puts too much obligations on me. So, no, I don't think that this is very valuable.

@scheglov You mean that you don't want analyzer to expose sealed classes in its interface?
If so, why?

As an avid user of analyzer, it sounds very useful. I already wrote a bunch of analyzer switch statements which are begging to become exhaustive

[jakemac53]

As an avid user of analyzer, it sounds very useful. I already wrote a bunch of analyzer switch statements which are begging to become exhaustive

Adding a new subtype is then breaking, which means even more breaking versions of analyzer. But you could view that as a plus as well - you will know exactly the places in your code that need to update and which cases need to be handled.

(you could also I think make a reasonable claim that it is breaking either way, but one will make code not compile at all while the other might be ok for a lot of code, and only code using new features etc would be broken).

[rrousselGit]

Adding a new subtype is then breaking, which means even more breaking versions of analyzer. But you could view that as a plus as well - you will know exactly the places in your code that need to update and which cases need to be handled.

It depends on the kind of "breaking".
I've been thinking about this for a while, and I think a good case could be made that adding new subtypes is still a 0.x.0 bump instead of x.0.0 bump.

Because that's the same kind of breaking as when adding methods to an implementable class.
Say a new method is added to AstVisitor and someone implements AstVisitor, then their code would "break" too. But we don't consider that a breaking change for the analyzer package.

I view adding new sealed types the same way.

---

In that sense, I'm arguing that someone writing an exhaustive switch voluntarily made a choice similar to that of implementing a class. They are aware that their code will need updating when a new type is added. And if they don't want this, they can opt out by making a nonexhaustive switch (using _).

[bwilkerson]

Say a new method is added to AstVisitor and someone implements AstVisitor, then their code would "break" too. But we don't consider that a breaking change for the analyzer package.

Actually, we do consider that to be a breaking change, which I why the documentation states:

Clients may not extend, implement or mix-in this class.

[scheglov]

Yes, I agree, that adding a method to AstVisitor, if we allowed implementing it to the clients, would have been a breaking change. In a sense visitors and sealed classes are similar. We don't allow implementing AstVisitor, just by saying this, and don't use any language syntax to enforce it. Instead, we say that clients should extend SimpleAstVisitor for example. This keeps the code compile-error free, when we add a method. But maybe not working as expected, if the analyzer adds a new node that replaces some other node, and now another method of the visitor is invoked. The client will not know about this.

So, there might be an argument for using sealed classes, and implementing AstVisitor.

But I'm curious about your specific cases that you noticed for exhaustive switches.

[rrousselGit]

By that standard, then pretty much everything is a breaking change.
Like how adding a new type could break apps, because the app already has a class with that name, so now they have a name conflict. Or how adding a new property to a final class is still breaking because it could shadow extension methods.

Semantic versioning makes no sense with that considered.

[rrousselGit]

But I'm curious about your specific cases that you noticed for exhaustive switches.

I've found myself quite often needing to iterable over all the possible subclasses of a given node.

I generally click on the definition of a given type. It contains a list of all the possible subtypes. And I switch over them all.
But if new types are added, I have to go back through my entire codebase to find them.

It's exactly the sort of things why sealed exist IMO.

[scheglov]

Sure, I understand sealed in general.
Do you mean that you would want sealed to tell you what to update?
I initially thought that you found cases where an exhaustive switch would be just more convenient, not only more correct.
Correctness is not a small thing, just if you have more arguments - bring them :-)

[rrousselGit]

Sure, I understand sealed in general.
Do you mean that you would want sealed to tell you what to update?
I initially thought that you found cases where an exhaustive switch would be just more convenient, not only more correct.
Correctness is not a small thing, just if you have more arguments - bring them :-)

I'll admit I don't understand your question and your usage of convenient vs correct. I'm not sure what they mean in this context. Happy to help by explaining more my point of view. But I don't quite understand your comment yet.

But anyway, for me, the Ast/Element tree is the textbook definition of what sealed classes are for.
While patiently waiting for sealed classes all these years, I've always been eagerly waiting for analyzer to use them. So this discussion breaks my heart a bit, ha

[lrhn]

I don't know of other cases that use our style of public-interface and private-impl (I mean, it's classic, I just don't think I've seen it outside our code base in a Dart package).

The reason for that could be that Dart doesn't need it. In Java, you'll want to define an interface, and an implementation of that interface, because otherwise you don't have an interface at all.
In Dart, with open classes, you can just write the implementation, and then derive an interface from it anyway.

The only real reason to have a public interface and private implementation is to have public members in the implementation, which are not part of the interface, and consider them "pseudo hidden" that way. It's just rarely done, because you can just make private members instead, and not worry about them getting exposed at all.

[bwilkerson]

The only real reason to have a public interface and private implementation is to have public members in the implementation, which are not part of the interface, and consider them "pseudo hidden" that way. It's just rarely done, because you can just make private members instead, and not worry about them getting exposed at all.

Yep. But in our case we want those "pseudo hidden" APIs to be usable outside the library, but within our package.

[scheglov]

Hm... Looking at it some more.

sealed class ExtendsClause implements AstNode { ... }

final class ExtendsClauseImpl extends AstNodeImpl implements ExtendsClause { ... }

ExtendsClause is sealed, so we can prove that any ExtendsClause is also ExtendsClauseImpl.

Currently, in the analyzer resolution visitors we have to rely on covariant, and write visitExtendsClause(covariant ExtendsClauseImpl node). I wonder if it would have been good to allow to write just visitExtendsClause(ExtendsClauseImpl node), i.e. without covariant to say: I want to handle this node as ExtendsClauseImpl here because I'm going to write some of its properties. Well, maybe this is too analyzer specific, and does not have wide use outside.

[jakemac53]

In that sense, I'm arguing that someone writing an exhaustive switch voluntarily made a choice similar to that of implementing a class. They are aware that their code will need updating when a new type is added. And if they don't want this, they can opt out by making a nonexhaustive switch (using _).

This issue with this approach, is that all your users get broken when a new type is added and they pub upgrade. To avoid that you would have to maintain a tight version constraint on analyzer, and have a contract around that (lets say new subtypes are only added in minor versions).

But we don't have any good linting to ensure people use this type of constraint today, or even good patterns established and agreed upon.

[jakemac53]

Fwiw, I would love it if pub had a diagnostic that would warn if you don't constrain yourself to minor release ranges, whenever you do a variety of things in this category, such as:

• switch on a sealed class you don't own
• switch on an enum you don't own
• implement a class you don't own
• override a method from a class you don't own

Or possibly this should actually be an analyzer lint? We do have some similar-ish lints.

[rrousselGit]

+1 on the lint.

I've used tigher version constraints before. I voluntarily "implement" AstVisitor on purpose in custom_lint. Because I do want a compilation error if a method is not implemented. And I use tighter constraints for this.

I think a lint rule and considering adding new sealed subtypes as minor bumps would be ideal.

I find it very counter productive if package authors voluntarily avoid using sealed for the sake of versioning.

[scheglov]

Why a minor version bump?
If a change is de-facto breaking, it should be a major version bump.
The clients will adapt to the new major version, one way or another.

[lrhn]

Whether a change is breaking, in the semver perspective, it's not necessarily as simple as "can any code possibly stop working?"

It's more like "will all intended uses keep working?", where the critical point is how well you've signaled which uses are intended.

Breaking an unintended, and therefore unsupported, use is not your problem.
If you never promised that it should work, not working is within specification.

Changing the API of a class that was never intended to be implemented or extended may break someone doing it anyway, but it's not semver-breaking if it doesn't break the supported API usage that's actually being versioned. The publisher decides what that is. (The rest is a documentation problem.)

In practice it's rarely as clear cut, not if your part of a large organization that tells you who you can break out not, but it is a perspective that a single, third-party developer can choose to take with their package.

That is, "de facto breaking" is only something you should care about if you've promised not to cause any possible breaking, and at that point it's actually "de jura" breaking too.

[scheglov]

When we make a class sealed, we promise: here are all its subtypes, you can use exhaustive switches.
Adding a new subtype will result in compile-time errors for these switches.

[jakemac53]

Why a minor version bump?

The reason is that depending on the use case, it might not be breaking for most users. Every time there is a new version of a package, it causes churn for all packages that depend on that package, which is a maintenance burden for package authors. So we want to strike a good balance between safety and breaking changes.

This is why most classes today don't consider adding a new member to be a breaking change, even though it is potentially breaking (if anybody is implementing your class). Or adding an optional named parameter to a method (breaking if somebody is implementing or extending and overriding that method).

Semantic versioning is just a tool, the point of which is to avoid breaking people. I don't think we should get too caught up in trying to be technically "correct" with our semantic versioning. We should use it in the way that best benefits our users. Because that is ultimately the point.

From my perspective, it would at least be interesting to consider a version of semver, where these specific types of breakages are done only as minor version bumps, and the smaller subset of packages that do these particular things are encouraged to have a constraint based on the next minor version instead of major version. This does cause more churn for the package with that dependency, they have to update their dependency for each minor version, but it means the rest of the things that depend on that package in a more "typical" way, do not have as much churn.

I do think it is a bit unfortunate to treat switching on a sealed type the same way as implementing a class from another package, but ultimately I think its very likely that people who own sealed types will add more of them, and accidentally not do breaking change releases when they do.

[rrousselGit]

Or adding an optional named parameter to a method (breaking if somebody is implementing or extending and overriding that method).

Heck, even for global functions, that's breaking too if you consider things like type inference or if (fn.runtimeType == void Function() or toString.
There are way too many ways something could "break". 😝

insert mandatory xkcd reference

---

In the end, I think it's about what makes the most sense for users.

If semantic versioning is going to prevent packages from using sealed because that's too inconvenient to make "breaking changes", then I'd rather stop using semver.
Flutter and Dart don't use semantic versioning after all.

Ultimately, with exhaustive pattern matching, the key point for me is that it's a conscious decision made by the package users.

Folks have the choice to write either:

switch (Base()) {
  case Impl1():  print('');
  case Impl2(): print('');
}

Or:

switch (Base()) {
  case Impl1():  print('');
  case _: print('');
}

Folks who opted for the exhaustive check did so on purpose. They want the "breaking change".
Folks who don't like the idea can use a non-exhaustive switch.
Users have the choice and made their decision.

If packages refuse to offer exhaustive checks for the sake of versioning, what's the point of having the sealed keyword in Dart?

[rrousselGit]

Just passing by to say thanks for making various Ast classes "sealed" ❤️