[Class Modifiers] Should base be removable within the same library?

[leafpetersen]

We chose in the design of class modifiers to enforce that base transitivity applies even within the same library. This isn't necessary - we could allow you to re-open classes to implementation within the same library, while still allowing enforcement of the same invariants. The argument against that was that it might be too easy to accidentally expose an interface that you didn't intend to.

Subsequently, we've had several use cases show up where there is some desire to not have base transitivity within the same library.

This issue describes one possible use case for a non-transitive base within the same library.

@jakemac53 ran into this again today when trying to design an approach to writing a final class while providing a mockable interface for testing. He found one clever but convoluted pattern using a sealed class with a factory constructor, implemented by a private implementation class and visible for testing public interface.

A simpler and more intuitive pattern would be something like the following:

base class FooService {}

@visibleForTesting
abstract interface class FooServiceForTesting implements FooService {}

This is forbidden by the current transitivity rules.

While this one use case is not definitive, it is interesting that we seem to be encountering uses for this. We could choose to change this - it would be non-breaking to do so. Should we consider dropping the requirement to make base transitive within the same library?

cc @dart-lang/language-team

[leafpetersen]

See also #3106 .

[lrhn]

If we remove the requirement inside the same library, then we need to answer what it means when you implement the non-base class, which has a base or final superclass, outside of their library.

Must the subclass be base? Probably not, since it already hasn't inherited any implementation that must be propagated to subclasses.

library lib1;
final class Foo { something(){} }
abstract interface class FooInterface implements Foo {}

and

import "lib1.dart";

class Fooly implements FooInterface {  // Must be `base`? Probably not.
 something() {}
}
class Fooly2 implements Fooly { // Allowed at all? Probably
 something() {}
}

I think it can be done (I might already have specified something like it before we made base viral, or at least made some sketches), but it will mean that you have to check all the superinterface/superclass paths to classes in other libraries, it's not enough to see that there is a final transitive superclass somewhere, you have to see if the path to it goes through a more open declaration, to known whether you can implement, or drop base.
(Most likely we just need to summarize at the dependency edges between libraries. If it has reopened a superclass, we no longer need to satisfy that superclass' constraints.)

[eernstg]

tl;dr   Special-case mocks?   rd;lt

I would be worried about allowing base to be reopened in general. If someone wants to do that you might just say "don't put base on that class!", because it basically eliminates the meaning of base. You look at the code, see base, and nothing can be concluded (unless you inspect the entire subtype hierarchy of that class in that library).

Evolvability

I think one important perspective on these questions is evolvability. Let's unfold that a bit.

One major reason for limiting subtypes of a class C is that the maintainers of C wish to preserve the ability to evolve the interface of C, that is, the set of member signatures available to clients of C.

Adding a new member is an obvious example. Most other changes are breaking no matter what: Remove a member (it might be called in client code), add an extra parameter, optional or not (the method might be overridden), change the bound of a type parameter (a superinterface in a client subtype may now be malbounded), etc.

Hence, in these discussions, "evolve the interface" probably means "add new members". Let's focus on that case.

The relevant subtypes are the ones that are declared outside the library that declares C, let's call them client subtypes. Also, bottom types are not client subtypes (so we don't have to say "has no non-bottom subtypes" all the time).

We have two obviously relevant ways to limit the subtypes of a given class C:

1. C has no client subtypes.
2. Every client subtype of C is a subclass.

Option 1 is stronger, but they are similar: If C has no client subtypes then the addition of a new member is never breaking. If every client subtype of C is a subclass of C or a class declared in the same library as C then the addition of a new member to C (with an implementation in every subtype of C in the same library that can have a client subtype) is only breaking if there is a name clash (somebody out there already declared a subclass member with that name).

Option 1 can be achieved by using final on C. Option 2 maps very directly to using base, as it is currently defined.

Conflicting forces

final on a class conflicts with mocking, as discussed in #3106. base does not prevent mocking in general, but Mockito relies on generating code that uses implements.

(I don't know how hard it would be to introduce support for mocks using MockFoo extends Foo with SomeMockitoThing ..., but when it uses code generation it would be possible to override every inherited implementation and thus faithfully reproduce the current behavior of a mock. The missing part is that the base mock can't avoid running some generative constructor of the mocked class, which may or may not interfere with the desired mocking behavior.)

Apparently, the readily available approaches to preserve the evolvability of a class conflict with mocking.

We can use sealed (cf. this comment, or @jakemac53's approach which might be similar), but this does not match all properties of the modifier: sealed implies abstract, which may be inconvenient in this case; sealed is associated with exhaustiveness checks, but that's irrelevant in this case. In other words, this usage isn't a perfect match for sealed, it is just something that adds up to a usable approach with some boilerplate.

Should we special-case mocking?

A concrete class that has an implementation of noSuchMethod which is different from Object.noSuchMethod is yet another safe client subtype, in the sense that it will have an implementation of every member of its interface, also in the case where the superinterfaces have been updated to have additional members. We could then consider a third invariant around a class C that we want to evolve:

3. Every client subtype of C is a subclass or a mock.

We could have some additional requirements for a class to be recognized as a mock (e.g., it might need to have an annotation or be declared in a library in a test or testing directory in its path, etc), but let's just keep that in mind and rely on noSuchMethod here.

We could then maintain this invariant by relaxing base such that implements BaseClass is allowed, in any library, for a class that has a non-trivial noSuchMethod.

This basically means that mock classes can violate any and all guarantees that base would otherwise provide. For instance, it would imply that invocation of members private to the library of C could throw, even though base can otherwise be used to ensure that private members do have an implementation.

The conceptual point is that a mock class is inherently unsafe. For example, it is only intended to work in some very specific scenarios during testing, and any use of noSuchMethod seems to imply that the class isn't expected to be a complete and faithful implementation of its superinterfaces. The complementary conceptual point is that we do not expect noSuchMethod to play a significant role in production code.

We could even allow Mock implements FinalClass where FinalClass is final and declared in a different library, as long as Mock is a mock class.

Next question: What do we destroy if we allow mock classes to violate the rules?

• Performance improvements? Any guarantee which may be violated is not a guarantee, but whole world analysis can detect that the rules haven't been violated in this particular program, in which case it can enable the corresponding optimizations. Is this good enough?
• Typing improvements? Presumably, we would not be able to promote final instance variables of final classes if the run-time type of the receiver could be a mock. It would be rather confusing if promotion is turned off in library a.dart just because we have a mock in my_test.dart.
• Other things?

If we don't actually have to pay for this extra permissiveness towards mocks (especially in a program that does not contain any mocks) then it could be a useful approach to relax base (and perhaps even final) in this way.

[jakemac53]

• It would be rather confusing if promotion is turned off in library a.dart just because we have a mock in my_test.dart.

Not just confusing, it's not possible at all to work that way in a modular compile. You can't necessarily see my_test.dart at all when compiling a.dart.

[leafpetersen]

tl;dr   Special-case mocks?   rd;lt

I'd suggest moving this to a different issue. I'm not super keen on it (I don't see how it stops people who want to ignore final from saying "it's ok, I'm a mock!" and then going to town), but I do see some paths forward there. But again, different issue.

I would be worried about allowing base to be reopened in general. If someone wants to do that you might just say "don't put base on that class!", because it basically eliminates the meaning of base. You look at the code, see base, and nothing can be concluded (unless you inspect the entire subtype hierarchy of that class in that library).

It most definitely doesn't eliminate the meaning of base. It does mean that you, as a library author (or editor), need to understand the portion of the subtype graph in one single library (the one that the class is defined in), in order to understand whether or not a given base class exposes an interface implicitly. This really does not seem like a big imposition to me.

I'm slightly more sympathetic to the "I don't want someone to accidentally expose an interface from a base class in my library by adding another non-base class", but for that we already have a lint.

[lrhn]

Re. Special-case mocks, I'd be happier to introduce friend libraries, so test files can pretend to be in the same library as the thing they test, than to special case some kind of class. (Not sure friend libraries would work, but happier to look in that direction.)

The questions here are:

• Can we remove the transitive requirements of base/final in the same library? (Fairly sure that's "yes", we can specify that in a consistent way.)
• Does it solve the problem presented here? (... maybe! It will allow the library to expose a subclass which can be implemented for mocking the superclass.)
• Does it solve the problem well? (More questionable. You'll have to define a second subclass for every class you want to mock. You have to control access to only the real classes using export...show declarations. More declarations, more chances to make mistakes, and for code that is only used in testing.)
• Does it introduce other problems? (Probably. It makes it much easier to accidentally reopen a base class and lose the effect that base was intended to provide - that all subclasses inherit implementation. A lint + @reopen probably makes that a harder mistake to make. But it means that you cannot know, from looking at a base declaration, whether it actually guarantees something or not.)

[eernstg]

@leafpetersen wrote:

I'd suggest moving this to a different issue.

Cf. #3111.

[munificent]

If I recall, the main reason we decided to specify the restriction this way is because it avoids confusion when there are multiple supertype paths to a base/final type. Consider:

// lib.dart

base class B {}

class C implements B {}

// main.dart
import 'lib.dart';

class S implements B {} // Error.
class T implements C {} // OK.
class U implements B, C {} // Error?

base class V extends B implements C {} // OK.
class W implements V {} // Error?

I can see an argument that U should be allowed. It breaks no invariant of lib that lib hasn't already allowed to be broken. Adding B to the implements clause doesn't introduce any new superinterface because C already has B as a superinterface. But if we allow this, it means that removing C from U's implements clause would cause implements B to become an error, which seems totally confusing.

I can see an argument that U should be an error since it uses a base class from another library directly in an implements clause.

But then I don't know how to reason about indirect uses of base classes. Is W an error because it indirectly implements a class marked base from another library? Or is it allowed because it there is a path through C to B?

I'm sure we can come up with an algorithm that answers all of these questions and preserves the invariants we care about (and I think Lasse did). I'm just not sure if users will ever understand it. And, if they don't, I worry a lot that they will make changes to package APIs that are breaking to their users without realizing it.

[jakemac53]

I can see an argument that U should be an error since it uses a base class from another library directly in an implements clause.

Yes, this is the right answer imo

But then I don't know how to reason about indirect uses of base classes. Is W an error because it indirectly implements a class marked base from another library? Or is it allowed because it there is a path through C to B?

Well shit 🤣 . This should also be an error, otherwise you are opening up all base classes through a simple layer of indirection.

[lrhn]

I'm sure we can define this in a way that is consistent and not arbitrary.

I'd be fine with saying that it's an error to have a base, final or sealed declaration from another library as declared superinterface, and to have an interface, final or sealed declaration from another library as declared superclass or mixin. You can just not do that.
If you also have an open or compatible subclass from the same library in the implements list, that's enough.

Don't ever directly do something to a class from another library that it doesn't allow. That's fair.

We still need the transitive restrictions too, to prevent a class from implementing another class in the same library, if it extends an other-library base class. I'd track that as a propagated flags on the declarations inside a library.

If any class has a base declaration from another library as declared super-class or mixin (which is allowed), then it's not locally implementable. Which means that it must itself be base-or-more and cannot be implemented even inside the same library, because the implementation-restriction comes from elsewhere.
If a class S is not locally implementable, then a subclass of S inside the same library is also not locally implementable.

Effectively we check restrictions, and register transitive restrictions, on library boundaries, where a declaration in one library subtypes a declaration in another library. Then we propagate the transitive restrictions inside the same library, to ensure nobody uses the "local library exception" to skirt the other-library restriction.

When we need to say why something must be so, we point to the class boundary relation, "because the superclass "Bar"-in-this-library extends class "Foo"-in-other-library which is marked base".
There may be more reasons, but remembering one is enough.

// lib.dart

base class B {}

class C implements B {}

// main.dart
import 'lib.dart';

class S implements B {} // Error. Agree
class T implements C {} // OK. Agree
class U implements B, C {} // Error. Remove `B` to remove the error.

base class V extends B implements C {} // OK. Agree
class W implements V {} // Error. `V` is not locally implementable because it extends `B`.

[jakemac53]

Fwiw more generally, I do actually think that by default base should be transitive within the same library, because I might be adding it for my own sake. See #3121 as a use case I just hit for that (granted its not actually valid today).

I think if we did allow re-opening it within the same library, it should require something explicit to do so.

[lrhn]

The current (and original) idea about "reopening" is that it doesn't require anything at the language level, you just write what you want for each class. The language doesn't have any concept of "reopening", it just does what each class says. "Allowing reopening" inside the same library just means not requiring you to add base-or-more to subclasses of base classes. They've always been able to ignore the base restriction, but with this change, they're also no longer required to propagate the restriction.

Then you can enable a lint which warns you if you reopen (for some definition of what that means), and you can use a @reopen annotation to get rid of that warning again.

So, it doesn't require anything explicit to reopen in the language itself, but if you include the lint, then it does.
I hope that will be enough.

(We still have transitive restrictions that apply to other libraries. If a class extends a base class from a different library, it will propagate the base restriction to itself and subclasses. You can't see that in the modifiers, because they won't tell you that you can't implement the class inside its own library, but we force you to add a base-or-more modifier so that it's at least visible to other libraries that they cannot implement.)

[jakemac53]

So, it doesn't require anything explicit to reopen in the language itself, but if you include the lint, then it does.
I hope that will be enough.

I would be satisfied with a lint, yes. I probably would want it to apply to all the things - and require an annotation to silence it. But I think that is a reasonable solution.