Skip to content

Standalone proposal for "closed" and "base" type modifiers. #2592

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
wants to merge 6 commits into from

Conversation

munificent
Copy link
Member

This is a subset of the larger "type modifiers" proposal just covering the "closed" and "base" modifiers. I carved it out from type modifiers because the latter also proposes disallowing using classes as mixins, which is a much bigger change. It's separate from sealed types because "closed" and "base" aren't necessary for exhaustiveness.

@munificent
Copy link
Member Author

In here, I took a very simple approach to trying to address #2451: I basically say that base is poisonous and affects all downstream classes, even ones in the same library. Seems pretty simple, but I'm not sure at all if that's the best approach. I just wanted to get something down so we can iterate on it.


* Implement a type marked `base` outside of the library where it is defined.

Any type that extends or mixes in a type marked `base` is implicitly treated as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is base applied transitively, but closed not? That feels weird to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably fill in a little more motivation and context in this doc. :)

It's worth thinking about these capabilities in terms of the invariants they define and the tools they offer to class maintainers. In other words, why a user might want to remove a capability from a type and what positive value they provides in return.

Base

By preventing a class C from being implemented, you get the invariant that if you have an instance of C, you know you have an object that concretely inherits from C. That means it is safe to cast it to private interfaces that C might implement, call private methods on C, access private fields, etc. You know that any validation or initializing in the constructor of C has been executed on this instance. It's a real C and (except for the things that can be broken by overriding), you know it has all of the state, validation, and behavior that C itself defines.

That invariant is only maintained if base is transitive. If a subclass D of C can in turn expose its own interface and that interface has C as a superinterface, then you could have an unrelated class that implements D but does not extend C. Now you have an object that is an instance of C but doesn't inherit from it.

Another possible alternative is to say that base isn't transitive. D can still extend C, and it can an interface, but that interface does not have C as a superinterface. So any type that implements D is not a subtype of C, but a type that extends D is. That feels pretty shaky to me, which is why I proposed instead we make base transitive.

Another option is to not make base transitive but make it required in subclasses. So you don't get it for free, but it's a compile error if the subclasses don't also write it.

Closed

I think the main useful invariant that disabling subclassing gives you is freedom from overriding. If you have a class C and some of its methods call other methods on this, then you know it will invoke your own methods right on C. You don't have to worry about "does this class get into a weird broken state if this method is overridden?" (This is something Dart class designers should worry about but people mostly just don't and rely on a friendly agreement not to override methods that aren't explicitly allowed to be.)

That invariant doesn't require closed to be transitive. If I have a closed class C and some class D that implements C, I don't care if people extend D or not. Either way, they aren't using any of C's implementation, so the invariant I care about is preserved. D has already severed itself from C's implementation, so whether or not D is extended is D's problem and D's problem alone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That invariant doesn't require closed to be transitive. If I have a closed class C and some class D that implements C, I don't care if people extend D or not.

I don't understand your example - you seem to be talking about implements. The example I have in mind is that C is closed, D extends C (it's my library, I know what I'm doing) but I don't want things out side of my library extending D. Sure, I can mark all of the subclasses of C as closed as well, but it feels to me that that's probably not the right default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maintain a 'private correctness' property for a base class? If we can say that it is safe for a leaking base class to have private properties (but a leaking non-base class with private members may cause noSuchMethod failures that subtypes can't avoid, so we can lint against leaking non-base classes). Surely that's only possible if base is enforced for all subtypes outside the declaring library.

Another property that a transitive base class A enforces is that the subtype hierarchy is a tree (unless this property is violated in the library of A). When the subtype hierarchy is a tree, reasoning about types may be stronger. (E.g., it is known that o is B and o is C cannot both be true if B and C are unrelated subtypes of A.) Similarly, when the subtype hierarchy is a tree and every subtype relation is the subclass relation, it may be possible to use faster implementations of some operations. (I believe some very performant approaches to subtype tests and object layout are only applicable in this situation.)

I'd prefer to require base explicitly on every subtype of a base class, because it is useful for a reader of the code of a class B extends A to know that it's a base class, even in the case where that's the only available choice because the superclass A is a base class.

I agree with @munificent that closed does not need to be transitive: When we enforce that subtypes can't inherit anything, it's trivially maintained for further subtypes that they also can't inherit anything.

The obvious metaphor could be the following: To break a chain, it's enough to break one link. To have a working chain, we have to transitively ensure that none of the links are broken. (So with closed we're breaking the chain at the first link; with base we're transitively ensuring that every subtype link is a subclass link, so we know that all of them will inherit these private members).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @munificent that closed does not need to be transitive: When we enforce that subtypes can't inherit anything, it's trivially maintained for further subtypes that they also can't inherit anything.

This is not true, and I give an example of why not in the comment immediately above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe closed should be "transitive in the current library only"?

It doesn't matter outside of the current library. Outside of the current library, you can't extend it, so you can't "inherit it". It's only inside the current library that transitivity or non-transitivity is observable. And it seems to me to be surprising to users if base is transitive (inside the currently library) but closed is not. Users have to remember that putting base on a subclass inside the library is redundant, but putting closed on a subclass inside of a library is not redundant. That seems like a foot gun to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter outside of the current library.

I would certainly expect global transitivity vs. transitivity in the current library (let's call it 'library transitivity') to make a difference:

// Library L1.
closed class C {}
class D extends C {} // Means `closed class D extends C {}`.

// Library L2.
import L1;
class E1 extends D {} // Error because `D` is closed.
class E2 implements D {} // OK.

// Library L3.
import L2;
class F2 extends E2 {} // Error?

With global transitivity, E2 is implicitly closed because it has C in its superinterface graph and C is closed (libraries don't matter here). With library transitivity, E2 is not closed. So F2 is an error in the former case and OK in the latter case.

The fact that C is a closed class provides some guarantees (I haven't quite found any essential ones, yet, but those guarantees must be the motivation for having the closed modifier in the first place). I can't see how any reasonable guarantees would be maintained or violated by restricting whether or not F2 is allowed to inherit member implementations from E2. Basically, I'm thinking that it's none of C's business whether F2 inherits from E2 or not.

That's the reason why I do not think it's obviously desirable to make closed globally transitive. But it may very well be a good idea to make it library transitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With global transitivity, E2 is implicitly closed because it has C in its superinterface graph and C is closed (libraries don't matter here).

Correct me if I'm wrong @munificent but I don't believe that there is any proposal to make access capabilities be carried via implements (super-interface) relations, only extends/with (superclass ) relations. So both E2 and F2 are fine, in any of these proposals, as best I know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Library L1.
some modifiers class C {}
class D implements C {}

So you're saying that class D could be used to allow every possible capability for out-of-library subtypes of A? Can we then have useful guarantees at all?

Copy link
Member Author

@munificent munificent Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maintain a 'private correctness' property for a base class?

Yes. The motivation section I added later talks about this. It's one of the motivations for requiring that if a supertype can't be implemented, then all subtypes can't be implemented either. See #2451.

The other correctness property you get by preventing implements is that it means every instance of the class must have invoked one of the constructors defined on that class.

I'd prefer to require base explicitly on every subtype of a base class, because it is useful for a reader of the code of a class B extends A to know that it's a base class, even in the case where that's the only available choice because the superclass A is a base class.

Good call. Done.

Correct me if I'm wrong @munificent but I don't believe that there is any proposal to make access capabilities be carried via implements (super-interface) relations, only extends/with (superclass ) relations.

The only thing I proposed being propagated in any way or form was base. And I'm changing that to say that it's not carried either, and instead the subtypes (aka the subclasses) are required to explicitly state it.

You said:

// Library L1.
closed class C {}
class D extends C {} // Means `closed class D extends C {}`.

Which is incorrect, according to this proposal. It should be:

// Library L1.
closed class C {}
class D extends C {} // Means `class D extends C {}`, can extend or implement D.

It inherits no capability restrictions from C. Outside of L1, a user can't extend C but they could extend D. If the author of L1 doesn't want that, they can write:

// Library L1.
closed class C {}
closed class D extends C {} // Means `closed class D extends C {}`.

I am definitely worried about the footgun that users might assume the restriction is propagated down and will be surprised that it's not. But aside from that, I like the semantics of not propagating down any capabilities or restrictions. It means:

  1. You can see the capabilities of a class by looking only at its own class declaration.
  2. We only need a set of modifiers to toggle the defaults in one direction. We don't also need unclosed and unbased for when a subclass inherits a restriction but the library author wants to remove the restriction in a subclass.
  3. It's simple. The specification in this document is, like, three sentences. (And one of them is to close the implements loophole in base class modifier loophole #2451.)

@leafpetersen
Copy link
Member

I'm fine with landing this as a proposal, but I'm not really sold on it yet. :)

@leafpetersen
Copy link
Member

Added a concrete counter-proposal here

@lrhn
Copy link
Member

lrhn commented Oct 31, 2022

My concrete problem with the names is that base and interface are positive names, used as a restriction.

The default is that you can extend and implement.
That's the same as writing base interface (you can do both).
Writing only one of those is effectively the same as not allowing the other one, base class Foo means not implementable, interface class Bar means not extensible, but base interface class Foo allows both, and we need closed to prevent both.
It's just feels backwards to me.

It is sort-of consistent in the sense that if there is any modifier, then we remove all the capabilities and add back the ones we wrote, with closed meaning adding none (which is different from writing nothing, because that just inherits capabilities instead).

capability modifiers
-extends -implements closed
-extends +implements interface
+extends -implements base
+extends +implements base interface
- inherit -

It's inconsistent in that we say that the default is permissive, but then I'd expect that you should be removing capabilities, not adding them back.

@munificent
Copy link
Member Author

Heads up that I added a motivation section just to try to make sure we're on the same page about why we might want to do this and what kinds of behavior that might suggest for the feature.

@munificent
Copy link
Member Author

My concrete problem with the names is that base and interface are positive names, used as a restriction.

Are you responding to Leaf's proposal or this one? If the former, it might be good to move this comment over to that issue.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite good! I suggested a couple of changes, in particular that the ability to derive a mixin should be omitted from a closed class, and perhaps even from a base class.

**Breaking change:** Treating `closed` and `base` as built-in identifiers means
that existing code that uses those the names of type will no longer compile.
Since almost all types have capitalized names in Dart, this is unlikely to be
break much code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not expect any parsing difficulties even in the case where closed and base remain as plain identifiers, because class is a reserved word.

Of course, we really don't expect any type to have the name closed or base anyway, but if we want to avoid the potential breakage entirely then there is probably no need to make those two words built-in identifiers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done.


* Implement a type marked `base` outside of the library where it is defined.

Any type that extends or mixes in a type marked `base` is implicitly treated as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maintain a 'private correctness' property for a base class? If we can say that it is safe for a leaking base class to have private properties (but a leaking non-base class with private members may cause noSuchMethod failures that subtypes can't avoid, so we can lint against leaking non-base classes). Surely that's only possible if base is enforced for all subtypes outside the declaring library.

Another property that a transitive base class A enforces is that the subtype hierarchy is a tree (unless this property is violated in the library of A). When the subtype hierarchy is a tree, reasoning about types may be stronger. (E.g., it is known that o is B and o is C cannot both be true if B and C are unrelated subtypes of A.) Similarly, when the subtype hierarchy is a tree and every subtype relation is the subclass relation, it may be possible to use faster implementations of some operations. (I believe some very performant approaches to subtype tests and object layout are only applicable in this situation.)

I'd prefer to require base explicitly on every subtype of a base class, because it is useful for a reader of the code of a class B extends A to know that it's a base class, even in the case where that's the only available choice because the superclass A is a base class.

I agree with @munificent that closed does not need to be transitive: When we enforce that subtypes can't inherit anything, it's trivially maintained for further subtypes that they also can't inherit anything.

The obvious metaphor could be the following: To break a chain, it's enough to break one link. To have a working chain, we have to transitively ensure that none of the links are broken. (So with closed we're breaking the chain at the first link; with base we're transitively ensuring that every subtype link is a subclass link, so we know that all of them will inherit these private members).

@munificent
Copy link
Member Author

Closing this in favor of #2646.

@munificent munificent closed this Nov 22, 2022
@leafpetersen leafpetersen added the class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins. label Dec 14, 2022
@munificent munificent deleted the closed-and-base branch April 21, 2023 18:55
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

Successfully merging this pull request may close these issues.

5 participants