Explain how to write mocks for classes using the new class modifiers

[jacob314]

What information is missing?

Documentation on how to mock classes that use the final class modifier.

Some people on my team were recently confused by whether to use the new final class modifier or the legacy @sealed annotation because they didn't know if there was a way to make final work with mocks that need to be in a separate library. Turns out there is a way but it is buried in a comment thread on the language repo.

How would you like us to fix this problem?

I'd like you to recommend the following somewhat esoteric code as the best practice to mock a final class.

// --- Library 'foo.dart'.
final class Foo { ... }

@visibleForTesting
base mixin MockFooMixin implements Foo {} // Could also be in 'src/foo.dart'.

// --- Library 'mock_foo.dart'.

import 'package:mockito/mockito.dart';
import 'package:something/foo.dart';

base class MockFoo extends Mock with MockFooMixin {}

Source: https://github.com/dart-lang/language/issues/3106#issuecomment-1593145359

Tracking issue to remove @sealed which is the alternate legacy way to create a final-ish class that can be mocked. https://github.com/dart-lang/sdk/issues/53310

[jacob314]

Fyi @kenzieschmoll who ran into a final class she wanted to mock. Fyi @eernstg who suggested this pattern for mocking final classes Fyi @jakemac53 who has also thought about mocking final classes.

[parlough]

\cc @natebosch As well.

@MaryaBelanger Would you like to explore/tackle this? I'm not sure where best to put it yet as I feel it doesn't belong in the class modifier documentation outside of a link/reference, but I'll give it some more thought. It'd also be nice to see what everyone tagged thinks first as well.

[jakemac53]

There are a few different workarounds you can use (haven't seen this one!), but all of them ultimately end up violating the exact principle you set out to achieve by marking the class final in the first place. Because of that it is my opinion that @sealed is actually the better approach, because it gives essentially the same level of safety without introducing an extra type into the original library (which will pollute auto complete etc).

[jakemac53]

Just to clarify what I mean, generally the goal (and specifically the goal in this case IIRC) is to have the flexibilty to safely add members to a final class without worrying about breaking anybody. This is simply an impossible goal to achieve if you also want to support mocks/fakes of your class (modulo using noSuchMethod to some extent).

With @sealed users will have to explicitly silence the diagnostic in order to violate this principle, which is actually IMO a stronger signal than having a special mixin that people can use without any diagnostics at all. You can more easily claim "you are doing something we said not to do, sorry".

[jakemac53]

It is also worth noting that this comment // Could also be in 'src/foo.dart'. is not correct - the mixin definition would have to exist in the same library as the final class it is implementing.

[MaryaBelanger]

Would you like to explore/tackle this? I'm not sure where best to put it yet as I feel it doesn't belong in the class modifier documentation outside of a link/reference, but I'll give it some more thought.

@parlough Yeah the team brought it to my attention yesterday and I agree it feels like it's too niche for the main docs. One thing that crossed my mind was maybe adding it to the mockito docs (that's how Dart does mocks, right?)? Like maybe in the best practices section or its own section on that page...

Or, maybe expanding the reference to more than just those couple tables and adding it there... Not sure yet, I'll be reading the language issue and waiting to hear back from the others tagged.

[MaryaBelanger]

@jacob314 Thanks for opening this. @kenzieschmoll Moving the email convo over here so all my notes are in one place:

(You mentioned checking the spec originally to help answer this question) I'm guessing the spec also didn't answer the mocking question directly (I just ctrl+f'd "mock" and nothing came up), but were there any details in there that helped at all or provided any context / pointed you in the right direction? Especially details that are not available in the main website guide like you mentioned.

[lrhn]

The "could also be in src/foo.dart" can be applied to the entire declaration. If you declare both Foo and MockFooMixin in src/foo.dart, you can choose to only export Foo in the package's public libraries, and require the test files to import src/foo.dart directly to get to MockFooMixin.

That should be a second, serious, discouragement against anyone in another package depending on MockFooMixin, since importing files in src/ of another package is strongly frowned upon.

[jakemac53]

If you want it to only be mockable for yourself then I agree putting it in src/foo.dart, only exporting Foo publicly, and using the approach above is very reasonable. Anybody importing from src you do not need to worry about breaking, generally.

[kenzieschmoll]

The proposed workaround may not be feasible after all. This is fine for creating fakes, but not mocks.

final class IsolateState {}

@visibleForTesting
base mixin TestIsolateState implements IsolateState {}

Then in our mock generation:

@GenerateNiceMocks([
  MockSpec<TestIsolateState>(),
])

// which generates this:
class MockTestIsolateState extends _i1.Mock implements _i31.TestIsolateState {...}

Manually editing the generated mock to be this

base class MockTestIsolateState extends _i1.Mock with _i31.TestIsolateState

removes the analysis errors but no longer makes this class a mock, since it will use the underlying methods from IsolateState for any method that is not overridden.

CC @srawlins

[srawlins]

CC @yanok

[natebosch]

I'd like you to recommend the following somewhat esoteric code as the best practice to mock a final class.

The best practice is to not use mocks when the library relies on final. We can add docs for some hack for the cases where rewriting tests without mocks is too costly. The ecosystem has historically overused mocks, so I'd imagine there is a wide audience for the content. I want to be careful not to advertise @visibleForTesting type solutions as a silver bullet - there would be cleaner ways to write the test, or to write the library with testing in mind, but we can't document those as a general case.

The extends Mock with TestMixinHack pattern won't widen arguments to allow nullable types, so some mockito features won't work without extra manual overrides. We could maybe choose a pattern that a library could follow, where mockito codegen could widen the arguments using the specially designed base mixin. I prefer not to encode workarounds like this into the API, but if it's high enough demand then we might not have much choice.

[natebosch]

have the flexibilty to safely add members to a final class without worrying about breaking anybody. This is simply an impossible goal to achieve if you also want to support mocks/fakes of your class (modulo using noSuchMethod to some extent).

Yes - I believe this is impossible at the language level. If you have the base mixin defined then it is breaking to add new members (edit: that aren't implemented in the mixin) despite the final. Codegen can mask the breakage, but I don't see any way that there isn't a user side code change.

[yanok]

May I suggest another (a bit more verbose, but I think less esoteric) pattern to achieve having effectively final but mockable class?

sealed class Foo {
  // Foo's interface
  // ...
  // Forward constructors to _RealFoo ones
  factory Foo() => _RealFoo();
}

// We can even drop `final`, since it's private
final class _RealFoo implements Foo {
  // Foo's implementation
  // ...
}

// This one could be mocked and passed wherever Foo is needed
@visibleForTesting
abstract class MockableFoo implements Foo {}

I think this achieves the goals:

• Foo is sealed, so can't be extended or implemented outside of its library.
• _RealFoo is private, so it's not visible outside of its library.
• MockableFoo can be mocked and the resulting mock is a valid Foo instance.
• @visibleForTesting ensures there will be a warning if someone tries to implement/extend MockableFoo outside of tests.

The cost is having to type Foo's interface twice: once in sealed class definition and another time adding overrides in _RealFoo. This could be avoided by moving all implementation into the sealed class and making _RealFoo extend it. But the price will be not being able to use the default Foo constructor, so users of Foo would have to always create it with a named constructor. Like

sealed class Foo {
  // Foo's interface with implementation
  // ...
  factory Foo.create() => _RealFoo();
}
final class _RealFoo extends Foo {
  // you may need to add constructors here, but that's all
}
@visibleForTesting
abstract class MockableFoo implements Foo {}

What do people think?

[yanok]

I think we could even turn this into a macro, once macros are ready.

[jakemac53]

The sealed class version does get quite verbose and all you really gain is the ability to work with mockito. With macros I could see us suggesting a solution like that though, but not until that time.

It also still fundamentally has the same issues - you lose a large portion of the value of having a final/sealed class. You cannot safely add members, since there is a version of the class which can be implemented by users. Breaking tests is still breaking.

[jacob314]

I agree that the sealed version is quite verbose. I'd rather add a true reopen keyword than accept having to repeat the interface twice just to use a mock. Short term, I think the most practical solution is to modify the code generator for Mockito to handle base classes. @srawlins has some ideas for how that could be done fairly easily which sound promising.

[eernstg]

Here is an updated version of the example, assuming that we'd want to import the proper class (Foo) from one library and the test interface (FooTestInterface) from a different library:

// --- Library 'src/foo.dart'.

final class Foo { ... }

@reopen
@visibleForTesting
base mixin FooTestInterface implements Foo {}

// --- Library 'foo.dart'.
export 'src/foo.dart' show Foo;

// --- Library 'foo_test_interface.dart'.
export 'src/foo.dart' show FooTestInterface;

// --- Library 'mock_foo.dart'.
import 'package:mockito/mockito.dart';
import 'package:something/foo_test_interface.dart';

base class MockFoo extends Mock with FooTestInterface {}

A few words about the underlying idea: I'm assuming that we want to have the modifier final on the class Foo. One typical reason for this could be that we wish to avoid breakage in Dart code "out there" where someone has created a subtype of Foo (extends or implements). So we just prohibit those direct subtypes by making the class final.

However, we still want to drill a hole in the protection such that it is possible to create subtypes of Foo using FooTestInterface which is @visibleForTesting.

Obviously, there's no firm protection now, because everyone can create a subtype of Foo by creating a subtype of FooTestInterface. But whenever this is done by a class which is not in 'src/foo.dart' and not in the 'test' directory of the package that contains Foo, they can get a diagnostic message from the analyzer. Presumably, this means that Foo can be tested appropriately, and nothing in this setup causes any of those diagnostic messages to be emitted.

Given that Foo is final, every subtype (in the same or in a different library, that doesn't matter) must be at least base. So that's the reason why FooTestInterface is a base mixin. It could also be an abstract base mixin class, if it is considered useful to be able to use extends FooTestInterface as well as with FooTestInterface.

The reason why FooTestInterface must be able to use with is that it would otherwise occupy the extends slot of every subtype we'd create, and that is a serious problem because we want to use that slot for other purposes (such as extends Mock).

Note that FooTestInterface does not provide any instance member implementations, it only introduces all the member signatures of Foo into the interface of the class that uses with FooTestInterface. In that sense it works exactly like implements FooTestInterface. However, we can't do that, because Foo is final, which implies that every subtype (direct or indirect) is at least base, which again implies that it is a compile-time error to have implements FooTestInterface.

Now some comments on other comments:

@jakemac53 wrote:

@sealed is actually the better approach, because it gives essentially the same level of safety without introducing an extra type into the original library (which will pollute auto complete etc).

It's definitely worth keeping in mind that FooTestInterface can pollute the name space. However, it is not difficult to make Foo and FooTestInterface available by means of different imports, as I showed in the example above. It is a bit less convenient for the author of the package that contains Foo, but it is not impossible.

However, I do think @sealed is less specific: It gives rise to diagnostic messages about every subtype of Foo which isn't in the same package (and I don't know if the 'test' directory counts as "the same package"). On the other hand, @visibleForTesting is specifically aimed at enabling tests to be written using entities that are not supposed to be used in non-test code. Also, it's rather easy to learn that mixins like FooTestInterface are intended to be used for testing, only, especially if there's a convention about the name (for example NameofclassTestInterface).

have the flexibilty to safely add members to a final class without worrying about breaking anybody.

As long as we're using mock objects whose class is getting a full set of members in the interface by having the compiler add everything that's missing, the compiler would just add more members if something new has been added to Foo.

So that shouldn't break unless those new members are being called during the test execution—but in that case it's obviously necessary to revise the test, because there's no way the compiler can know what the new members should do. So in that situation the mock would probably be refined by doing additional when ... then invocations during the setup, or whatever is the approach taken when the mock is prepared for the test.

@kenzieschmoll wrote:

The proposed workaround may not be feasible after all. This is fine for creating fakes, but not mocks.

I think this problem does not exist. Here is the code:

final class IsolateState {}

@visibleForTesting
base mixin TestIsolateState implements IsolateState {}

Then in our mock generation:

@GenerateNiceMocks([
  MockSpec<TestIsolateState>(),
])

// which generates this:
class MockTestIsolateState extends _i1.Mock implements _i31.TestIsolateState {...}

I would expect that Mockito gets adjusted such that it will generate working code for this type of setup, but right now we can edit it manually:

Manually editing the generated mock to be this

base class MockTestIsolateState extends _i1.Mock with _i31.TestIsolateState

removes the analysis errors but no longer makes this class a mock, since it will use the underlying methods from IsolateState for any method that is not overridden.

That's not true, _i31.TestIsolateState does not contribute any member implementations at all. So if Mockito will generate this kind of declaration, and it will generate a version of various methods where non-nullable parameters are widened to nullable ones, then mocking should continue to work.

@natebosch wrote:

The best practice is to not use mocks when the library relies on final.

Sure, if that's a common guideline then by all means use fewer mocks. However, the same kind of consideration would arise for fakes as well, and the FooTestInterface should work for that case as well.

If you have the base mixin defined then it is breaking to add new members (edit: that aren't implemented in the mixin) despite the final. Codegen can mask the breakage, but I don't see any way that there isn't a user side code change.

I do think that testing classes (mock/fake/whatever) whose members are generated by a special purpose code generator, or by the compiler, must be considered to be different from a normal subtype.

Maybe we're just masking the breakage, but if that means that we don't have to do anything manually in order to handle the addition of a new member (OK, if it's a special purpose code generator then we probably need to run it) then it should arguably be considered to be "significantly less serious breakage" (or possibly no observable breakage).

So how much work should we expect to have to do in response to the addition of a new member to Foo? Nothing? Run a code generator? Manual source code modifications? Of course, we do have to edit some code in the test if it needs to call that new member, but that's hardly something we can avoid.

@yanok wrote:

May I suggest another ... pattern

Very nice! We have considered patterns using sealed for this purpose in some earlier discussions in this topic area. One concern which was raised is that it may create a conflict with the other possible subtypes of Foo:

final class Foo {}
final class FooA implements Foo {}
final class FooB implements Foo {}

@reopen
@visibleForTesting
base mixin FooTestInterface implements Foo {}

@reopen
@visibleForTesting
base mixin FooATestInterface implements FooA {}

@reopen
@visibleForTesting
base mixin FooBTestInterface implements FooB {}

If we wish to use sealed to get a similar effect, we would inevitably include MockableFoo in the set of types that are needed in order to exhaust Foo, and that's probably somewhat confusing and not very useful. (I'm assuming that Foo wouldn't be sealed otherwise, we're just sealing it because we want to use this pattern for testability).

[Edit: added missing @reopen in one example.]

[jakemac53]

However, I do think @sealed is less specific: It gives rise to diagnostic messages about every subtype of Foo which isn't in the same package (and I don't know if the 'test' directory counts as "the same package").

IMO this is an advantage of @sealed :). By many breaking change definitions (including flutters), breaking tests is also considered a breaking change. So an approach based on @visibleForTesting actually doesn't buy you anything when it comes to modifying the class. With @sealed people can still violate it, but at least they have to explicitly ignore a diagnostic so they are aware they are in unsupported territory.

As long as we're using mock objects whose class is getting a full set of members in the interface by having the compiler add everything that's missing, the compiler would just add more members if something new has been added to Foo.

There is no compiler mechanism that does this though right? Until/if that actually exists, I don't think it is worth considering when designing docs written for the current language. Just using noSuchMethod is not sufficient for existing mocks (they need to widen parameter types to accept nulls), hence why they are codegen based. It is relatively trivial to regenerate your mocks, but that is a manual process still and not a part of the compiler.

[natebosch]

Just using noSuchMethod is not sufficient for existing mocks (they need to widen parameter types to accept nulls)

I think I was wrong about how dependent we'd be on codegen to mask the breakage - this is only the case when the widened parameters need to be used - which implies an edit in the test. I think codegen is a reasonable hurdle for adding new uses of the method in your code/test.

[jakemac53]

I think I was wrong about how dependent we'd be on codegen to mask the breakage

Do generated mocks implement both noSuchMethod and also override all members that were known at compile time?

Either way, mockito is not the only thing that this is opened up to. People could have hand written mocks/fakes/stubs and we can't enforce that they override noSuchMethod.

[eernstg]

@jakemac53 wrote:

There is no compiler mechanism that does this though right?

Well, any class that has a noSuchMethod implementation which is not inherited from Object will have members generated automatically by the compiler for the entire interface (except members that are declared manually). Those members will invoke noSuchMethod, passing their own name as a Symbol. Perhaps Mockito uses generated methods entirely today, but otherwise I'd expect the compiler-generated stubs to be the main reason why Mockito works.

(OK, if the noSuchMethod stub is generated because the missing member has a private name from a different library then it won't invoke noSuchMethod, it will throw.)

[jakemac53]

Even if I thought the semantics of these workarounds were good, I still think that pushing a best practice that results in testing specific types being defined in non-testing libraries is just a very poor design choice, and worse than not having final/sealed at all. It is a huge code smell.

My suggestion for documentation for mocking final classes would be to just remove final :).

[natebosch]

If we are relying on the analyzer specific @visibleForTesting to keep code on the rails anyway, maybe we should add an annotation to express this more directly?

@implementableForTesting
interface class WantsToBeFinalButMockable {

Analyzer can give extra diagnostics for implements outside of tests, same as it would for usage of @visibleForTesting.

Edit: We could probably also express the noSuchMethod requirement.

@implementableForMocking
interface class WantsToBeFinalButMockable {

Analyzer would only allow implements for a file under test/ and with a non-Object noSuchMethod implementation.

[yanok]

@eernstg but in your example you still need to match on Foo to make the match exhaustive. Nothing changes with sealed. You still have to match on the base type and no, in this case matching on MockableFoo is not needed, since it will be covered by Foo.

[eernstg]

@yanok wrote:

still need to match on Foo to make the match exhaustive

Exhaustiveness is not computed based on final classes, it is specifically the job of the modifier sealed. For example:

abstract final class A {}
final class B1 extends A {}
final class B2 extends A {}

void main() {
  A a = B1() as dynamic; // Obviously no promotion, so `a` has type `A`.

  var x = switch (a) { // Error: not exhaustive, does not cover `A`.
    B1() => 1,
    B2() => 2,
  };
}

Exhaustiveness analysis refuses to consider B1 and B2 to be an exhaustive set of types for an expression of type A: Even though we can't actually have anything else than a B1 or a B2 at run time (because A is abstract, and no other subtypes are declared in this library, and A is final so they can't come from other libraries), the exhaustiveness analysis doesn't take that step, it just requires all of A to be covered, period.

This means that a final hierarchy (indeed any hierarchy that doesn't have any sealed declarations) doesn't create any expectations about specific sets of types being exhaustive for a shared supertype, so we won't have types like FooTestInterface (formerly known as MockFooMixin) popping up in error messages like "this switch is not exhaustive, you need to include FooTestInterface". I don't see how this can be avoided if the design pattern uses sealed.

[yanok]

Let's just get back to the beginning of the discussion.

Documentation on how to mock classes that use the final class modifier.

The proper documentation is "YOU DON'T". And then we could probably add a footnote in small script about how this could be broken.

Motivation: from the language PoV there is exactly one way to mock a final class -- to put the mock into the same library. But this is of course a very bad option and we should advise strongly against using it, since putting mocks with their nSM magic in the non-test code is a bad idea. This is also technically impossible now, since Mockito codegen can't generate parts, but the technical problem is solvable while the problem with mixing test and non-test code stays.

Now, if we rule out "class and its mock in one library" option, it's safe to say that strictly speaking it's not possible to mock a final class. We have to give up on something: either we can't mock it, or it is no longer final. The source code might still say it's final, but once we've added something like MockFooMixin Foo is effectively not final anymore, the guarantees final gave us are gone.

So, the only thing we could talk about here is trading a bit of finality for the possibility to create mocks. This doesn't sound very formal, since it doesn't make sense to be 99% final. I guess formally we want it to provide the same guarantees that final does, given some assumptions hold.

And all three approaches (base mixin, sealed and currrently non-existing reopen) are actually doing exactly this: in any case Foo is not final anymore but it's almost final, provided users promise to only implement it with nSM (but even that could break if new private members were added to Foo).

What I like about sealed solution is it doesn't lie to you: Foo is no longer final and the source code is very explicit about it. Potential reopen solution is a bit worse here: Foo is still final in the source code, even though it is not actually final. But at least there is an explicit reopen and IDEs could for example highlight final modifier on Foo, saying it was reopened. base mixin solution is the worst here: Foo is listed as final and there is no clear indication that there is a backdoor to implement it externally. In fact it is quite the opposite: one thing is final and another one is base, who would have guessed that the two combined give us a possibility to effectively implement Foo? My main complain about the mixin solution is it's very obscure. Curious why this possibility was even added?

But again, any option is just making a hole in finality, so for Foo to be reasonably final we have to make sure MockFooMixin/MockableFoo/reopened Foo doesn't leak too much. I agree @visibleForTesting is not providing enough protection. (BTW, despite the documentation for it saying that it only won't complain from test/ of the package where Foo is defined, it's actually quite happy with test/ of any package). We could have another annotation that would check that implementors have nSM overridden, that would improve precision a lot, but adding private members to Foo could still break users.

I'd propose to consider something more radical. We already have a "don't mock classes you don't own" rule in the general mocking guide at Google, and it's even more true for mocking final classes you don't own. So let's just recommend not to export MockFooMixin/MockableFoo/reopened Foo as part of your public API. Package own tests could still use it, and could be broken by changes made to Foo. But then it's a package owner's job to fix them.

[yanok]

@eernstg Hm... but then your point actually goes in the other direction: exhaustiveness analysis for sealed classes can see that some options are not needed, while cannot see the same for final classes. But this means we definitely won't have to add more clauses with sealed, than we already do with final, right?

In your example:

final class Foo {}
final class FooA implements Foo {}
final class FooB implements Foo {}

@visibleForTesting
base mixin FooTestInterface implements Foo {}

@visibleForTesting
base mixin FooATestInterface implements FooA {}

@visibleForTesting
base mixin FooBTestInterface implements FooB {}

we would have to match all three FooA, FooB and Foo to take them apart. Nothing changes with sealed: https://dartpad.dev/5a4707d12115b85c216eb78c25a98afe?

still have to match FooA, FooB and Foo. And that is exhaustive, so no complaints about FooTestInferface. Am I missing something?

[eernstg]

@yanok wrote, in response to @jacob314:

Documentation on how to mock classes that use the final class modifier.

The proper documentation is "YOU DON'T". And then we could probably add a footnote in small script about how this could be broken.

Sounds good! Many final classes would presumably be so small and simple that they can just be used as-is in a test. This is a good starting point, because it avoids discrepancies between mock behavior and actual behavior.

However, when that doesn't suffice, the developer would read the small script, and then we're back where we came from.

the only thing we could talk about here is trading a bit of finality for the possibility to create mocks

Right, that's also how I see it. (It's about mocks and other specialized subtypes which aren't subject to breakage to the same extent as other classes, because they have a non-trivial noSuchMethod or they are obtained by code generation).

In fact, 99% final might be perfectly fine if the exceptions are all auto-updated if a new member is added, or the signature of an existing one is changed in a breaking manner.

Even the addition of a private member might be OK: If that private member is only called from other members of the same receiver then they won't be called at all by a mock, so it doesn't hurt that they would throw if they were called.

What I like about sealed solution is it doesn't lie to you

Sort of. The modifier sealed does tend to imply "this class is intended to have a set of direct subtypes which is known locally", which is basically the same thing as saying "this class was made for being the scrutinee of switch expressions". That might not be accurate.

Also, sealed restricts the direct subtypes, but it doesn't put any restrictions at all on indirect subtypes. So if you want to avoid having subtypes in other libraries then you'd want to get a notification if you ever add a direct subtype to the sealed class, because that's a completely unrestricted opening of your sealed class. If the class is not sealed but final then you'll either get an error (e.g., class SubFoo implements/extends Foo {}) or a lint, assuming that implicit_reopen is enabled (e.g., base class SubFoo implements/extends Foo {}), unless and until that local subtype is marked by @reopen.

In particular, I think it's going to be a source of confusion if there is a sealed class Foo, and there is no reasonable way to use it in a switch expression, because it is impossible to come up with a set of direct subtypes of Foo that will exhaust Foo: _RealFoo cannot be denoted, and we wouldn't actually want to switch on _RealFoo and MockableFoo anyway.

base mixin solution is the worst here: Foo is listed as final and there is no clear indication that there is a backdoor

That is a general property of class modifiers. I was pushing for a more strict ruleset, but at least we got the implicit_reopen lint and the associated @reopen metadata which ensures that you can rather easily check every class modifier you encounter: Always enable implicit_reopen, and always search the current library for @reopen. You can only trust the interface part of any class modifier when no subtype of the one that has the class modifier has been reopened (so any final could have been weakened to base by a subtype in the same library, and any interface could have been dropped entirely by a subtype in the same library).

However, with sealed there are no restrictions on indirect subtypes at all, and implicit_reopen/@reopen won't help you. In short, it's not the purpose of sealed to restrict indirect subtypes, if you want them restricted in some way you just have to remember to not create anything that violates those restrictions.

So here is an example using a base mixin:

final class Foo {
  Object m(int i) => 'Foo: $i';
}

final class FooA extends Foo {
  String m(num n) => 'FooA: ${super.m(n.toInt())}';
}

final class FooB extends Foo {
  Comparable<String> m(num n) => 'FooB: ${super.m(-n.toInt())}';
}

@reopen
@visibleForTesting
base mixin FooTestInterface implements Foo {}

@reopen
@visibleForTesting
base mixin FooATestInterface implements FooA {}

@reopen
@visibleForTesting
base mixin FooBTestInterface implements FooB {}

A corresponding example using sealed classes:

sealed class Foo {
  factory Foo() = _RealFoo;
  Object m(int i);
}

sealed class FooA extends Foo {
  factory FooA() = _RealFooA;
  String m(num n);
}

sealed class FooB extends Foo {
  factory FooB() = _RealFooB;
  Comparable<String> m(num n);
}

class _RealFoo implements Foo {
  Object m(int i) => 'Foo: $i';
}

class _RealFooA extends _RealFoo implements FooA {
  String m(num n) => 'FooA: ${super.m(n.toInt())}';
}

class _RealFooB extends _RealFoo implements FooB {
  Comparable<String> m(num n) => 'FooB: ${super.m(-n.toInt())}';
}

@visibleForTesting
abstract class FooTestInterface implements Foo {}

@visibleForTesting
abstract class FooATestInterface implements FooA {}

@visibleForTesting
abstract class FooBTestInterface implements FooB {}

With the base mixin, we'll get a lint on each ...TestInterface if we forget @reopen (assuming that we've enabled that lint, as usual), but with the sealed class we don't get any notification at all if we forget to make any subtype of Foo or FooA or FooB sealed (or private, or final).

Of course, the fact that we need to duplicate the member signatures (in Foo and in _RealFoo, etc.) and set up constructor redirections was mentioned already, but I think it's worth noting that this added complexity will also grow when the type hierarchies grow in complexity (for instance, _RealFooA needs to implement FooA, but it also needs to extend _RealFoo).

So I'm still not convinced that the base mixin is the worst approach. ;-) Mockito will need to be generalized to handle that case as well, but that shouldn't be impossible.

[eernstg]

@yanok wrote:

exhaustiveness analysis for sealed classes can see that some options are not needed, while cannot see the same for final classes. But this means we definitely won't have to add more clauses with sealed, than we already do with final, right?

The point I'm making is that we don't want exhaustiveness for these classes in the first place, and that makes the keyword sealed on Foo a source of confusion.

If we do want exhaustiveness for Foo then by all means it should be sealed. But in that case it's not likely to be convenient that it has a subtype like MockableFoo (corresponding to FooTestInterface in my example) and even less convenient to have _RealFoo that we can't denote, because we'd want to switch on the "real" subtypes, but we're forced to switch on Foo() as well (there's no way we can write a switch which is exhaustive without some kind of a default case, and Foo() will do when the scrutinee is statically a Foo()).

void m(Foo foo) {
  var x = switch (foo) {
    FooA() => 'A stuff',
    FooB() => 'B stuff',
    Foo() => 'Mockable stuff or _Real... stuff',
  };
}

So I'm not saying that final (and base mixin) will give you better switches, I'm saying that it's not about switches, and sealed is just not a good match for this kind of usage.

[yanok]

@eernstg :

Also, sealed restricts the direct subtypes, but it doesn't put any restrictions at all on indirect subtypes.

Yes! but that's exactly my point. The moment I see sealed, I know I must look at direct subtypes. And once I do so, I find _RealFoo, which is final and private, so I know I don't have to worry about it, and abstract interface class FooTestingInterface, from which I can immediately conclude that this is a backdoor I need to worry about. Or, maybe, it has an @implementableForMock annotation, which I trust, so I conclude it's all good. That's how I want it to be.

With the base mixin solution, I see final Foo, but I have to know that it doesn't actually mean final. So I have to check if implicit_reopen lint is enabled for this code and hopefully it is. Then I need to check all subtypes of Foo, not just direct ones, if they have a @reopen annotation. But that's not all! I need to also understand if it's the kind of reopening I worry about. Since if I care about being able to add new members to my class safely, maybe lowering the protection to base is fine, but abstract base that only implements Foo and doesn't provide actual implementations is not ok, since it allows effectively implementing Foo via extends, so will suffer from the same issues. And if the lint is not enabled (and I can't enable it myself, since I'm just looking at the code in CS for example), this only gets worse.

it is impossible to come up with a set of direct subtypes of Foo that will exhaust Foo: _RealFoo cannot be denoted, and we wouldn't actually want to switch on _RealFoo and MockableFoo anyway.

That's intentional, Foo is not meant to be matched. Yes, it's not a usual case of sealed class, but I'd not even say it's an abuse: we literally just want it to be sealed, it doesn't necessarily imply we want users to match on it. At the same time, using mixin to just smuggle an interface is clearly an abuse ;)

That is a general property of class modifiers. I was pushing for a more strict ruleset, but at least we got the implicit_reopen lint and the associated @reopen metadata which ensures that you can rather easily check every class modifier you encounter: Always enable implicit_reopen, and always search the current library for @reopen. You can only trust the interface part of any class modifier when no subtype of the one that has the class modifier has been reopened (so any final could have been weakened to base by a subtype in the same library, and any interface could have been dropped entirely by a subtype in the same library).

But why? I can understand class modifiers being completely informational, such that one gets a warning for reopening, but can always just silence it. Or having them completely enforced, such that reopening is always an error. And Dart somehow ended up implementing something in the middle. Why can't I just reopen final Foo to be abstract interface class FooTestingInterface implements Foo if I can achieve effectively the same via base mixin trick? Seems like "security (or rather safety in this case) by obscurity" to me.

So I'm still not convinced that the base mixin is the worst approach. ;-) Mockito will need to be generalized to handle that case as well, but that shouldn't be impossible.

I will make my claim more precise: the base mixin is the most obscure approach. One can actually argue that it makes it better than others ;) Making Mockito change is not hard, I probably already spent more time writing comments here than I would spend implementing it in Mockito :) But I really think there are bigger issues with this approach than having to special case this in Mockito.

[lrhn]

But why? I can understand class modifiers being completely informational, such that one gets a warning for reopening, but can always just silence it. Or having them completely enforced, such that reopening is always an error. And Dart somehow ended up implementing something in the middle.

The Dart class modifiers are restrictive. They are just not, or wasn't originally, transitive.

You can declare a final class, and nobody outside of your library can extend or implement it. That protection is absolute. (Which is why we have this issue.)

However, you, inside your own library, can make subclasses without any restrictions. Those do not have to be final. They originally didn't have to be base, and there is no technical reason to require them to be base today. It was just considered too much of a foot-gun to declare a final class Foo and a /*completely open*/ class Bar extends Foo by accident, because you forgot to write a modifier, and not recognize that you have broken open your own abstraction. So now, you have to at least make the subclass be base, to ensure that nobody can implement the interface, anywhere (except you, in the same library, which also makes it actually useful for optimizing compilers).

So, the modifiers are tools that you can use to build an impenetrable barrier of unimplementable classes, a strong abstration, but you can also use them to build something less strong if you want to. You can give subclasses fewer constraints than the superclass (a subclass can always choose to allow itself to be extended, so final to base, or interface to open or base, but it cannot allow itself to be implemented if the superclass cannot).

We could remove all the bindings, and allow a library to remove any constraint in a subclass (as long as the constraint isn't inherited from another library). There is nothing inherently wrong with that, it might just be a little too easy to use it wrong. That's where the lint comes in. From the language's perspective, there is nothing special about reopening, going from final to base in a subclass. There are rules about what you can and cannot do, and the program satisfies those, so it's valid. The language doesn't care whether you're doing it by accident or not, either it assigns a semantics to the program, or it's an invalid program. There are no degrees of "valid".

The lint, which the language semantics knows nothing about, gives you a warning if you reopen a supertype, unless you mark the class with @reopen. That's technically redundant, which is why it can be used as validation. If you say the same thing in two different ways, it's probably deliberate. That will help you to not accidentally reopen, but still allow you to do so, if you want to.

[eernstg]

@yanok wrote:

I need to check all subtypes of Foo

First note that this is basically only relevant to the maintainers of the library that declares Foo. They are the only ones who'd want to establish and maintain constraints on subtypes (using class modifiers), and they are the only ones who'd want to detect if and how anything is being reopened.

As a developer who is just using Foo (working on client code in some other library), the given constraints will pop up as soon as we violate them, and we don't really need to care about constraints that we make no attempts to violate.

If we encounter a constraint violation (say, because class MyFooMock implements Foo ... is an error), it is helpful to have some conventions: It could be that (1) there is no reopening, and we just can't mock Foo at all, or that (2) Foo has been reopened in a specific way that we recognize as a test support idiom, and then we proceed to use MyFooMock extends Mock with FooTestInterface or whatever it takes. Again, we just search for @reopen in the library that declares Foo in order to detect reopening declarations.

With sealed class Foo, no similar search would work: No constraints are placed on indirect subtypes. For example, there is no constraint on being a subtype of Foo if we have the following:

sealed class Foo {}
class FooA implements Foo {} // Oops!

The point is that any library can declare a class that implements FooA, thus making the addition of new members to Foo a breaking change. In this case you really need to search all subtypes of Foo and check that every one of them maintains the relevant constraints.

I don't understand why that could be considered to be a stronger protection than an approach where reopenings are flagged with a @reopen annotation.

using mixin to just smuggle an interface is clearly an abuse ;)

Fair point! :grin:

However, it works, and it is kind of nice that the passageway is narrow. As you say:

the base mixin is the most obscure approach. One can actually argue that it makes it better than others ;)

[yanok]

You can declare a final class, and nobody outside of your library can extend or implement it. That protection is absolute. (Which is why we have this issue.)

That is true, but usually people care about semantic properties and not syntactic ones (well, at least I hope so). So, even though nobody outside of my library can extend or implement the class, that doesn't mean there are no subtypes of the class, implemented outside of the library. But that's what people really care about. Note the comment above: people want to use final to make sure there are no external implementations, so they could add new members without braking anybody. But that's just not true if we are not enforcing modifiers transitively.

It was just considered too much of a foot-gun to declare a final class Foo and a /*completely open*/ class Bar extends Foo by accident, because you forgot to write a modifier, and not recognize that you have broken open your own abstraction.

Yes, that's exactly what I'm complaining about. Why complete removal is a compiler error and lowering to base is just a lint? Seems quite arbitrary. If we just want to prevent accidental removal, that looks exactly like the purpose of lints.

There are rules about what you can and cannot do, and the program satisfies those, so it's valid.

This is true. But usually we hope that the rules imply some nice properties, like memory safety, etc. And if the rules doesn't imply the properties that users expect, that results in lots of disappointment.

[yanok]

@eernstg

First note that this is basically only relevant to the maintainers of the library that declares Foo.

Sure. This is all about maintainers. The bug started with an ask about how to replace @sealed that was a help for maintainers. final is also there to help maintainers, otherwise we would just recommend to remove final and that's it -- problem solved. So we do care about maintainers experience. And I do think the base mixin trick makes maintainer experience worse. Please note that maintainers are not always the same people who wrote the code.

Users experience is much less affected by the choice of approach here:

1. First of all, don't mock classes you don't own, package users shouldn't be mocking these classes at all.
2. Even if they want to, currently Mockito will suggest to try mocking one of the variants if asked to mock a sealed class, so users could just look at subclasses and pick one that's not final/base/sealed. Usually it will also have a suggestive FooTestingInterface name and @visibleForTesting annotation (or even something more suggestive, like @visibleForMocks). I don't see how that is more hard than looking for @reopen.
3. If we agree on the specific idiom, we can even encode it into Mockito, so users will think they are mocking final/sealed classes, while Mockito will look for *TestingInterface with a specific annotation instead and use that. So, UX would be the same in the end.

Regarding your class FooA implements Foo {} example: from the users perspective that's actually an easy case, they could just mock FooA and that would be enough for them. From the maintainer perspective that's absolutely a mistake, but I expect people to be able to look at direct children of a sealed class and say if they are reasonable. We could do a @almostFinal annotation and a lint to check this, but I don't really think it's necessary.

I don't understand why that could be considered to be a stronger protection than an approach where reopenings are flagged with a @reopen annotation.

I wouldn't say it's a stronger protection. It's more about being explicit about the intent. sealed calls for attention to its direct children. The base mixin version without the lint, that we discussed before, just silently sneaks the backdoor, that's why I wrote it's the worst in this respect. With having to write explicit @reopen it's much better, but still worse than sealed IMO, because one has to remember to look for reopens (IDEs might provide some help though).

[eernstg]

Let's just keep using Foo as the name of the constrained (sealed/final) class, with subtypes FooA and FooB. The assumption is that we wish to ensure that out-of-library subtypes of these classes are subject to some kind of vetting based on @visibleForTesting, @visibleForMocking, or some other device. The outcome would be that the Foo... classes don't have any other out-of-library subtypes than the vetted ones (if you're cheating and creating a "real" subtype and ignoring the lints about @visibleForTesting etc, it's your fault when it breaks).

Presumably, the vetted subtypes will not break (not much, anyway) even if the Foo... classes are updated, because they have automatically generated members. For example, a modified parameter type or an extra member could just give rise to automatic generation of slightly different noSuchMethod forwarders, no manual work. Of course, if you add a new method to Foo and a test needs to call that method on a mock, you'd need to change the test and set up the mock, etc., but lots of changes that will be breaking for other subtypes won't break a mock or a mock-based test that doesn't care about that new method or that updated signature.

Also, please assume that implicit_reopen has been enabled, and @reopen has been added on all declarations where this is needed in order to avoid all lint messages from implicit_reopen.

@yanok wrote:

Users experience is much less affected by the choice of approach here:

1. First of all, don't mock classes you don't own

When I mentioned the constraints that clients could encounter (such as "you can't create a subtype of Foo because it's final"), I was focusing on the actual protection that the maintainers of Foo have against unwanted dependencies. This means that the interesting case is when client developers create a non-mock subtype of Foo because there is no protection; for example, if we use sealed class Foo but forget about sealed on FooA then we can create a subtype of Foo by creating a subtype of FooA, and there isn't going to be the slightest hint that this dependency is unwanted.

So what I'm saying is that, with the approach based on sealed, you must remember to make every public subtype of Foo a sealed or final class, except that you will presumably drill a hole in this protection by having a non-sealed, non-final class like @visibleForMocking abstract class FooMockable implements Foo {} for each class which must be mockable.

However, if you're using the base mixin approach (where Foo is final) you don't have to remember anything: If you don't want to drill a hole in the protection then all your subtypes of Foo will be final, and they will be in the same library. If you do want to drill a hole then you'd have to mark the given class with @reopen. Depending on the purpose of drilling that hole, you could also mark that class with @visibleForMocking, @visibleForTesting, or whatever is needed in order to ensure that client code will get the right (lint) messages about how to use this particular type.

The point is that every one of these "hole types" is marked explicitly with @reopen, which means that it will be a local check to ensure that each of these hole types is also marked with @visibleForMocking, @visibleForTesting, or whatever is appropriate.

In short, one approach requires that you remember to put sealed on a bunch of classes (final will do, too, or they can be private), and on each of the hole types you need to remember to put @visibleForMocking or whatever is appropriate. With the base mixin approach the analyzer will tell you to put @reopen on every declaration which is actually reopening something, and you know that the @reopen-ed declarations are exactly the ones that need to have @visibleForTesting or @visibleForMocking.

(OK, you might want to drill a hole for other reasons, but you would still have @reopen right there as a reminder, and the motivation for reopening could be given in some other way, e.g., in a DartDoc comment.)

2. Even if they want to, currently Mockito will suggest to try mocking one of the variants [of a sealed class] ... Usually it will also have ... @visibleForTesting ... I don't see how that is more hard than looking for @reopen.

True, it isn't difficult for clients to see that things are looking good: This type is a subtype of the one that I need to mock, and it's marked @visibleForTesting.

But I'm focusing on the case where it isn't all good: The maintainers of the Foo... classes have forgotten to add that @visibleForTesting, or they forgot to make that type sealed in the first place, so the client is now happily creating a subtype (not necessarily a mock) of a type that they shouldn't subtype.

The point is not that it's easy or hard for a client to look for @reopen, the point is that the Foo... class maintainers can trust reopening declarations to have the @reopen marker, which means that a very simple textual search will tell them that they are dealing correctly with every reopening declaration, and no declaration is reopening by accident.

Regarding your class FooA implements Foo {} example: from the users perspective that's actually an easy case, they could just mock FooA and that would be enough for them.

Sure, it isn't difficult to walk through a door which is unlocked, but should have been locked.

From the maintainer perspective that's absolutely a mistake, but I expect people to be able to look at direct children of a sealed class and say if they are reasonable.

You'd need to check the entire subtype hierarchy. If you want to constrain the subtypes of Foo then you'd need to see that the constraints are maintained properly by the direct subtypes (such as FooA and FooB), and by their direct subtypes, and so on, because subtyping is transitive.

Also, you'd need to remember to repeat this check every single time there's a change to any one of those subtypes, and when a new one is added or an existing type is turned into a subtype of Foo (direct or indirect).

I thought you said that it isn't appropriate to ask the maintainers of Foo to perform this kind of whole-library analysis in order to decide a question that a compiler is trivially able to decide?

Foo is listed as final and there is no clear indication that there is a backdoor to implement it externally.

So it isn't fair to expect that people will search for @reopen, but it's OK to expect that they search through the entire subtype hierarchy of a sealed class and check that each of them is (1) sealed, (2) final, (3) private, and not leaked via a type alias, or (4) named ...Mockable or ...TestInterface and marked with @visibleForTesting/@visibleForMocking/...?

[yanok]

if we use sealed class Foo but forget about sealed on FooA then we can create a subtype of Foo by creating a subtype of FooA

This is a valid argument. Indeed, if one "forgets" to take care of FooA by making it final or making it sealed and doing something with its children, FooA will remain open. And with final Foo complete reopening by accident is not possible and making it base requires an explicit @reopen. Agreed. But do we really need to optimize for people forgetting things? What I someone "forgets" to make Foo final in the first place?

or they forgot to make that type sealed in the first place, so the client is now happily creating a subtype (not necessarily a mock) of a type that they shouldn't subtype.

That's a weird argument. And what if they forgot to make it final in the first place? The client will also happily create a subtype.

You'd need to check the entire subtype hierarchy.

Right, if you have a hierarchy of final classes that you want to turn into "mockable but almost final", to be completely sure you'd have to check that final subclasses were turned into "sealed + real + mockable" correctly. Agreed. We could have a lint for this though, just like we have a lint for @reopen.

Ok, I think we are going in circles already. I have a feeling that your arguments are geared towards writing the code ("what if they forget to make it ...?"), while my arguments are geared towards reading the code. As a person who reads way more code than writes, I don't like the idea that final doesn't imply a simple property "all subtypes of this class live in this library", as I think that's what people expect it to mean. I like the idea that we even encourage an idiom based on breaking this property even less. It's about being explicit vs implicit. I don't like having to check if there are holes drilled (but I agree that having to put an explicit @reopen simplifies it), I'd like the code to explicitly say "beware, there might a hole". That's one thing. Another thing is being obscure. If where will be a way to just wide open interface class FooTestInterface implements Foo with explicit @reopen, it'd be way better than base mixin approach IMO. You still have to look for @reopen, but at least once you found it, the intent is super clear.

But since apparently I'm the only one concerned with this, it will just stay my special opinion.

[eernstg]

(Sorry about the noise level, and going around in at least half circles, I'll try to respond briefly ;-)

But do we really need to optimize for people forgetting things?

Ideally, one keyword on one class should be enough to get the desired protection for that class. All the derived requirements should be machine-checked.

E.g., if Foo is final then FooA will be final because everything else causes diagnostic messages, or FooA can use @reopen to allow reopening it to base. When we have @reopen on all reopenings then it's a simple and local check that each of them has @visibleForMocking or whatever is appropriate. If it's just wrong (e.g., FooA should not reopen anything) then we'll have a good opportunity to see that it is wrong at the time where the compiler tells us that FooA must have a @reopen (or it must be final).

So we're helping developers to maintain consistency (like: checking/satisfying derived constraints). Arguably, we could say that it's all about allowing people to forget about all the derived checks, because the computer will check it for them. I'd say that this is a very typical and valuable use of a computer.

you'd have to check that final subclasses were turned into "sealed + real + mockable" correctly. Agreed. We could have a lint for this though, just like we have a lint for @reopen.

Indeed, and that would be fine! I was just considering how we could create this "almost final" situation using the features available today.

I don't like the idea that final doesn't imply a simple property "all subtypes of this class live in this library", as I think that's what people expect it to mean.

I was fighting for that approach in the language team for months. I didn't get that, we only got the implicit_reopen lint and the connection to @reopen. However, if we do take that lint very, very seriously then I think it can work quite well.

In the end, of course, we could also have a do_not_reopen lint which would flag every class that reopens anything, thus ensuring that @reopen is never even needed. :grin:

[yanok]

I was fighting for that approach in the language team for months.

It's a pity that you lost this fight :(

[atsansone]

@jacob314 : Is there a final TL;DR here? This discussion is important, but doesn't help docs get to what needs to be updated. Can you or someone else clarify?

[eernstg]

I still think the approach that @jacob314 described in the original post is a viable design pattern for this purpose. There could be others, but there shouldn't be a problem in having a list of viable approaches of length greater than one. ;-)

[natebosch]

I believe that @implementableForMocking is the most direct and readable way to express this. Are there concerns around implementing the diagnostic for that annotation?

https://github.com/dart-lang/site-www/issues/5143#issuecomment-1692417180

[lrhn]

An @implementableForMocking (or slightly more general @implementableForTesting) would effectively be adding a warning if someone implements the interface in a program that isn't a test (has an entry point recognizable as a test, fx. being in test/). It can be ignored, but if someone does that, and they then get broken by it, it's on them.

The class won't be final or base, and won't get any benefits that might be derivable from that. Which probably aren't that much, a whole-program compiler can see if no subclass is retained after tree-shaking, and optimize accordingly. Might affect promotability a little, but so far that's only for private members anyway.

It should work. It fits with @publicForTesting.

[parlough]

\cc @bwilkerson and @pq for insight/opinions on the discussed annotation and diagnostic.

[Levi-Lesches]

So how much work should we expect to have to do in response to the addition of a new member to Foo?

By many breaking change definitions (including flutters), breaking tests is also considered a breaking change.

My two cents on an old thread. I think these two quotes show a slight difference in who is interested in a breaking change. Consider three packages:

• package:a declares a class A
• package:b declares a class B implements A
• package:c wishes to use packages a and b

Now, consider the author of package:a adds a new member to class A. How does this affect packages b and c? It depends:

1. If class B is in package:b's main API, then this will break package:b and package:c
2. If class B is in package:b's tests, then this will only break package:b.

I think that's a very important distinction. When package:b says it depends on a specific version of package:a, that means any other package or application can use both packages so long as they are compatible, without breaking. And when package:a updates its minor version, users don't have to worry about package:b. That's very important to preserve, and I believe that's the main benefit of modifiers like final.

(Side note: base seems to have two uses: "this code depends on some private implementation you must inherit", and "I don't want new members to be a breaking change". base is relevant to this conversation only in the second context, but probably not the first. That's important to https://github.com/flutter/flutter/issues/127396 as well, which suggests to use base purely to prevent new additions being breaking changes, even when implementations really would want to override any inherited behavior that won't work on their platform).

On the other hand, case 2 feels much less important to me. It's obviously still important -- you wouldn't want your application with lots of dependencies to break every time you run dart pub get -- but it doesn't impact the whole ecosystem in the same way. And, as @eernstg said, if the fix is just running one command to regenerate code, I don't see that as being such a big deal. Especially if you have to explicitly ignore a warning to get broken in the first place.

Based on that:

• I like implementableForTesting because it distinguishes between both cases more effectively.
• Would it be reasonable for final to be just a warning in tests? The author is saying they don't want to be held accountable for anyone using their class in an unintended way, but sometimes tests need that kind of invasiveness to override, mock, or stub behavior. If package:b's tests explicitly ignored the warning, then any breakage would be on the author of package:b.
• Macros change the premise of the question entirely. Macro-generated augmentations are not regular code that needs to be checked in and uploaded to pub, but rather the macro application becomes part of the code and even public API. Thus, if mockito were to become a macro, then @mock class B implements A would not need any changes if class A adds a new member. Not only can package:b keep running their tests, but package:c could use B without issues because the macro would be applied within package:c. (please let me know if I'm misunderstanding how macros work here). Maybe then some new consideration should be had for macros that implement a final class, since they may be able to automatically adapt to new members? Maybe macros that generate noSuchMethod should get special attention here.