Experimental @sealed annotation

[matanlurey]

(I actually wish this was a language feature, but we can start somewhere...)

All over even the Dart SDK codebase I see code like:

/// ...
///
/// May not be subclassed or implemented.
class FooAst {}

There is nothing actually preventing users from doing this.

I've started more aggressively using factory constructors to prevent subclassing:

class DoNotSub {
  factory DoNotSub() => new DoNotSub._sealed();

  DoNotSub._sealed();
}

But this still doesn't prevent implements, which, IMO is a serious problem for:

Data models/structures that are not meant to be mocked:

class Point {
  final int x;
  final int y;

  Point(this.x, this.y)
}

// Should never be necessary... Just use 'new Point'
class MockPoint extends Mock implements Point {}

Classes that implement hashCode and equals comparing private state:

class SnickersBar {
  int _howManyPeanuts;
  bool _includesCaramel;

  SnickersBar();

  @override
  bool operator==(Object o) => 
      o is SnickersBar && 
      o._howManyPeanuts == _howManyPeanuts &&
      o._includesCaramel == _includesCaramel;
}

cc @yjbanov

[zoechi]

#25462 (comment) seems related

[lrhn]

Preventing anyone from implementing your class' interface isn't solving any problems. It might be preventing problems if the class isn't expecting other implementations, and it assumes that all instances of the class that it sees will extend itself. Maybe because the class has private members that it accesses on instances other than this (#25462). That means that you can't pretend to be that class to the class itself (or its library), but why prevent anyone from pretending to be that class to other libraries, e.g., for testing?

Preventing anybody from extending or implementing Point will prevent the ColorPoint problem, but it will also prevent anybody from making a PolarPoint that stores its data as angle and magnitude instead of Cartesian coordinates. That would be a perfectly good Point implementation that is backwards compatible and even equality will work correctly.
The more generic a class's concept is, the more reasonable it is to use it as an interface. I probably don't want to use DateTimePickerMenuEntryWidget as an interface, but Point is perfect for it.
I don't see "preventing mocking" as a goal to strive for - if someone else wants to do it, why should you concern yourself about that.

If possible, I do recommend basing hash code and equals on publicly available data of a class - or just using identity, if the class isn't really defined by its visible state (or is mutable). Obviously that's not always possible, and in that case, you will have a class where you cannot make an a subclass with objects equal to the original class, but that's hardly a problem. Do let other programmers mock your class - if they do it, they probably have a problem that they solve that way. If you prevent them, they will curse at you for preventing them from solving their problem for no apparent reason (to them, even if it might be apparent to you).

TL;DR: You won't know how someone else wants or needs to use your class, so preemptively removing their choices probably won't make anybody happy. It isn't really solving any problems for yourself - you can already not extend or implement your class if you don' t want to. You might be creating problems for others, when they decided that they do need to make a wrapper for your class, or mock it for testing, or something else that neither of us have even imagined yet.

Also, you may like the syntax:

   factory DoNotSub() = DoNotSub._sealed;

[bwilkerson]

@matanlurey

A couple of questions: Do you want a single annotation that prevents classes from extending, mixing in or implementing a class, or do you want more granular control? Do you want this to apply to all code, or only code outside your package?

@lrhn

I can tell you my motivation for wanting something like this, and you can tell me if there's a better way to accomplish my goal. What I really want is better control over what changes will and won't be a breaking change. If I have a class and allow it to be extended, but don't allow it to be mixed in or implemented, then I think I can add a new concrete method to that class without breaking clients. But if I allow it to be implemented, then any client that implements the class is broken by the addition of a method they don't implement.

[lrhn]

True, to some extent. If you add to the interface of a class, other implementations of that interface will now be incomplete. That's not necessarily breaking immediately - it's only if someone starts using the new feature that it becomes a problem. It's not a problem that can be avoided in general. The same problem happens if you have a bona fide interface and add a member to it.

There shouldn't be any problem with allowing the class to be mixed in. There are many other problems with using a class as a mixin if it isn't designed for it, but if you add a member to a mixin class, using it as a mixin should also get you that member. It may conflict with something in the superclass it's mixed onto, but that's a different problem.

All in all, I don't think it's a problem worth solving by disabling the the interface of the class. I find that cure to be worse than the disease.

[yjbanov]

Another use-case for @sealed is when designing a class optimized to be monomorphic to support inlining and JIT-ing. If Point is involved in some heavy math, allowing the possibility of passing PolarPoint to the algorithm could hurt performance.

[eernstg]

@bwilkerson

For a given class C, even adding a concrete method m to a class would be a breaking change, even in the case where no other class is implementing C --- in all the client code, including libraries written by third parties "out there" that we do not know about. There could still be a subclass of C which contains a getter named m, and that class would now be broken. So we'd certainly need to constrain both implements and extends in order to get any absolute guarantees against breaking changes. Even in that case there could be tricky issues if there is a non-trivial noSuchMethod on C.

If we want to promise that any particular set of changes will not break existing usages then we would certainly need to aim for a better foundation for knowing that this is true, and in which sense. I suspect that developers would have very little wiggle room if that notion is made strict and precise, so we would probably keep on being somewhat loose on this topic.

However, the original issue was actually focusing on preventing both implements and extends, so let's return to that.

@matanlurey

I think it would be quite easy to implement support for a @sealed annotation or a sealed modifier, so the real discussion is whether that will make a lot of developers overly constrain their code, thus hurting the overall Dart community because Dart code in general will tend to prevent reuse in too many cases where it had not been necessary. It's a matter of deciding when a given constraint is justified based on (perceived?) performance benefits, and when it's unjustified because extensibility turns out to be more important in the long run.

In any case, it makes sense to try to see whether we can work around the current lack of sealedness.

There may be cases where you can make the class that you want to protect private. Let's call it _C. Other libraries cannot denote such a class, so that'll enforce that no class (outside that library) can implement or extend it. Of course, you may need to establish a connection like _C implements C, such that clients in other libraries could use C in their type annotations. This makes it possible for clients to create mock versions of C, and it makes it possible for the code in your library to tolerate those (potential) mock objects in some situations, and specifically insist on having _C instances in other cases (e.g., when calling private methods on _C). Documentation might state to which extent mocking will work, possibly by saying that it won't ever work (i.e., the implementation does if (x is! _C) throw 'Illegal argument!'; whenever an object has been provided from "outside").

The missing bit here is that (1) clients can still get in trouble because it's a convention and not enforced that no classes can implement C, and (2) even though method invocations on instances of _C may be fast, there must be some checks that any given C (passed from clients, at least initially) is actually an _C.

I don't know if you consider it sufficient to know that an implementation could use techniques (like the JVM) whereby a running program is optimized, based on the fact that there is global information available at that point: If the actual program does not include any other subtypes of C than _C and C is abstract then checks on the form x is _C can be eliminated if x is already known to be a C. This does make the work-around quite similar to the expected situation when we have some form of sealedness.

[matanlurey]

@eernstg I hear you on preventing code re-use, and I can be convinced it's not a good idea.

Maybe the minimum amount of (easy, agreeable) work would be to de-sugar something like:

class Point {
  final int x;
  final int y;

  const sealed Point(this.x, this.y);
}

into

class Point {
  final int x;
  final int y;

  factory const Point(int x, int y) = Point._sealed;

  const Point._sealed(this.x, this.y);
}

I've opened another issue, #27389, to track a proposal for @doNotMock.

[bwilkerson]

This has come up again in the form of #30871.

[matanlurey]

Related issue from our friends on Flutter, @yjbanov found that making their heavily used ThemeData class polymorphic regressed AOT benchmarks as much as 6%. They are refactoring it back to monomorphic, but they lack the ability to make sure customers don't subclass it themselves.

[srawlins]

I'd like to implement this. @matanlurey and @yjbanov, can you answer Brian's two questions above? That is:

1. Would you like one annotation, or several more granular? (My preference would be one; if we're vetting this as an official language feature, I don't think it makes sense to do multiple.)
2. Would classes within the annotated class's library be able to implement/extend/mixin? What about classes within the annotated class's package?

[matanlurey]

I think @leafpetersen is supportive here, as well. Would like to this his take.

@srawlins:

1. Would you like one annotation, or several more granular? (My preference would be one; if we're vetting this as an official language feature, I don't think it makes sense to do multiple.)

Let's be careful about calling this an official language feature. It isn't. I want to make sure that this is somewhere between an experimental language feature and an analyzer lint. If and when backends start using @sealed for optimizations or correctness is likely when we've crossed that line.

That being said, I just want 1 annotation:

@sealed class ThemeData {}

2. Would classes within the annotated class's library be able to implement/extend/mixin?

No. If we want to track this as a potential language feature, it's not possible (or at least desirable) to add a bunch of loopholes into how it doesn't actually work. Imagine that you could sub-class String in the test/ folder.

You could always get around this yourself:

// Go ahead and implement/extend/mix-in this in this library.
class _SecretTypeThatIsInheritable {

}

@sealed class PublicTypeThatIsSealed implements _SecretTypeThatIsInheritable {}

What about classes within the annotated class's package?

We have no concept of the word "package" in the language. I hope that changes, but until then it isn't helpful (and if you look at other languages with sealed - C#, Java, etc - none of them have opt-outs).

[leafpetersen]

@lrhn and I have been talking a bit about trying to get this in as a language feature for a medium term release, and prototyping with annotations would be a great way to get started. @bwilkerson and I chatted a bit about this last time I was in Portland. @srawlins maybe you could start pulling together some ideas and requirements for discussion? I think we may have some brainstorming docs from previous language team discussions to pull from as well. @munificent did you have a proposal for this once upon a time?

[srawlins]

Let's be careful about calling this an official language feature. It isn't.

Lol ok I'll be careful. I'll just word it as this annotation vetting the concept, for a possible official language feature.

[munificent]

@munificent did you have a proposal for this once upon a time?

Not a full proposal around semantics, but there was some discussion between Erik and I for syntax. I'll send it your way.

[mraleph]

If this is going to be cased on annotations please consider building it on top of @pragma so that VM could recognize it without much additional work.

[matanlurey]

@mraleph:

so that VM could recognize it without much additional work.

I'm guessing we are a ways away before asking the runtimes to understand the annotation (I think this is intended to be an analysis-only lint/warning until we decide to move forward/get feedback/etc).

[mraleph]

@matanlurey I will rephrase: if we want to also at some point do a quick experiment with what VM could potentially gain from such an annotation then it would be good to have it in a form that VM already understands to a certain degree.

[srawlins]

This has been released in Dart SDK release 2.1.1; the CHANGELOG notes the two new codes: SUBTYPE_OF_SEALED_CLASS and MIXIN_ON_SEALED_CLASS.