dart-lang / language

Design of the Dart language
Other
2.65k stars 202 forks source link

Should mocks be exempted from `base` constraints? #3111

Closed eernstg closed 1 year ago

eernstg commented 1 year ago

[NB: This is a copy of an entry in a different thread. It was a bit too off-topic, so here is a separate issue for it.]

This issue raises the question: Should we allow a mock class to have an implements C clause where C is a base class in a different library?

Evolvability

I think one important perspective on class modifiers 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:

  1. 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?

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.

eernstg commented 1 year ago

Comments on this idea in the other thread, with paraphrased topics:

jakemac53 commented 1 year ago

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

Everything :). This is no better than an annotation if we allow people to violate it, and we should have shipped annotations instead if that is what we want to do.

eernstg commented 1 year ago

tl;dr OK, forget final; however, base could work rd;lt

Everything :)

That's possible! And that's also the reason why I was always pushing for consistent and enforced rules around these modifiers.

However, if we can actually maintain some/most of the properties in some sense then it is worth being aware of that possibility.

@leafpetersen argues here that the performance implications in relation to final classes are much more serious than one might think based on "we do whole program compilation anyway, so we'll just discover that a class is strictly final if that is actually true". I buy that. I said similar things previously, and in particular: I think we should preserve the ability to have a good story about separate compilation.

So let's just consider the situation where a mock can implement a base class B. This implies that it won't be forced to run the superclass constructors all the way through the chain of base classes starting from B, and we may not have implementations of certain private methods in the library of B (and other libraries from the superclass chain).

However, these violations of guarantees that we would otherwise have do not seem to disable any optimizations. (Perhaps member invocations can be faster if they can use traditional single inheritance class dispatch rather than interface invocations, but I don't think that's going to make a huge difference. Also, interface invocations will still be needed.) So these guarantees are motivated by correctness: We want to make sure the invocation of private members in the library of B are safe, and also that validation code in constructors in the library of B will be executed.

We do have the option to say that "when you're a mock, it's your fault if you fail because you aren't sufficiently correct!". The point is that mocks are by their very nature incomplete, and hence they are not going to be correct according to the full spectrum of usages of the mocked type anyway.

We do also have the guarantee that adding a member won't break the mock (other than by a name clash, but that's already true for all other subtypes of a base class).

In other words, I'm not convinced that we're destroying everything if we allow mock classes to implement base classes in a different library.

rrousselGit commented 1 year ago

Personally, my stance on this is that a good practice when using class modifiers would be to also provide a mock. I'm not fond of the idea of "tests behave differently".


I'm also against the idea of relying on being inside the test folder (but ideas like relying on noSuchMethod may be fine).

It would've been fine if final/base were annotations instead of language keywords. But as language keywords, I don't like the idea of having them behave differently based on where they are.

To begin with, tests may not be inside an actual test folder.
For example, I have a documentation website. And the code snippets within that website are stored inside .dart files, which are then injected in the website using some form of macros. This enables static analysis of the website to detect compilation errors.
As such, if my documentation speaks about tests, I'll have a test snippet next to the document. But it wouldn't be in a test folder. They aren't tests for the website. They are the source code of the website.

eernstg commented 1 year ago

I'm not fond of the idea of "tests behave differently".

100%!

But I don't think we have that: The tests would have been possible anyway if the mocked class hadn't been base, or if the mock class had been declared in the same library as the mocked class. The code, including the mock code, isn't going to do anything which is different from what it does in code where that base modifier hasn't been added.

So we're really not introducing any new mechanism or behavior that makes the code behave differently. We are just allowing the mock to exist tomorrow, just like it's allowed to exist today (where the mocked class isn't yet base).

jakemac53 commented 1 year ago

Here are some more concrete concerns :)

"we do whole program compilation anyway, so we'll just discover that a class is strictly final if that is actually true".

Only for production code - debug code does not do whole program analysis. But yes for production builds I agree there isn't a performance benefit. I don't think that was ever a motivation anyways though?

The primary thing we concretely lose is any ability to improve the developer experience:

As a package developer, one of the biggest benefits of putting final on a class is that it blocks mocking.

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

This is making an assumption that isn't actually true - code generation does not happen automatically, and mocks of any class can be shipped from any package. Mockito explicitly opts into allowing this by building to source and not to cache. Generated files in this mode are really no different than user written files.

We do have the option to say that "when you're a mock, it's your fault if you fail because you aren't sufficiently correct!". The point is that mocks are by their very nature incomplete, and hence they are not going to be correct according to the full spectrum of usages of the mocked type anyway.

And yet, when I inevitably break some usage of a mock, those developers will file an issue on my repo that I broke them. If this is a flutter customer test it will be considered a breaking change and I will be required to fix the test. Even though the customer is violating my explicit intention.

jakemac53 commented 1 year ago

Another point that I think is super important here, is that it is not as if API developers do not have the ability to open their classes up for implementing/extending/etc. They have all the options possible available to them, and that is why we have so many modifiers, we tried to allow every useful combination of features.

So, if a package author chose to use those features in a way that blocks mocking, why should we allow a user to violate that? What makes tests so special? It is just fundamentally wrong, imo.

lrhn commented 1 year ago

The one thing we could possible do, is to make some things work differently in development and production mode. We have these modes, so if we start making them more explicit and precisely defined, we could allow some restrictions to be ignored in development mode, but not in production mode. That would allow tests, in the same package only, to ignore certain restrictions.

But that leaves a big usability issue when the code that works while you are developing it, stops working when you think you're done. Having too big a distance between development and production just means that you need to run "production tests" as well as "development tests". I'd rather not go that way.

Restrictions need to actually restrict. They only work if they actually work. (And tautologies are tautologically true).

We can help find ways to make mocks that are valid within the language, but breaking the language in order to test it just means you can't trust the test.

jakemac53 commented 1 year ago

The one thing we could possible do, is to make some things work differently in development and production mode.

People ship using mirrors today, by shipping in what we would typically consider "development mode" (the JIT vm). Even if we defined "development mode" as "asserts enabled", they would just ship that way :).

They also ship with DDC because it makes interop easier.

eernstg commented 1 year ago

@jakemac53 wrote:

It eliminates the possibility of field inference (at least outside the library)

Not sure what that is? Inferring the type of an instance variable var x; based on superinterfaces should still work. If it's about promotion of final instance variables then it would already be impossible for a base class, unless that instance variable had a stable getter, or it was private and known to be non-overridden, etc. Nothing lost that I can see.

Breaking change guarantees are not as simple as implied:

I'll buy that one! ;-)

What if a class overrides methods and uses noSuchMethod? Now changes to the overridden methods are breaking.

That is already true for other subtypes of a base class, almost any change to the signature of a potentially overridden method is a potentially breaking change. A mock with an explicit method declaration is just one more class which can be broken in the same way. So you can't (safely) make that change in a base class anyway, nothing lost.

Does this only apply for a class which only defines a noSuchMethod and nothing else?

I don't think it is necessary to put up any further requirements, you can write other members, and they don't create any further breakability relative to changes in a base class.

In short, the mock isn't worse than other subtypes of the base class, so nothing is lost.

What about mixins etc?

A mixin which declares a noSuchMethod is guaranteed to be part of a class that has a non-trivial noSuchMethod, and hence it can be allowed to have implements BaseClass.

So I think it makes sense to say that we can have "mock mixins".

Other mixins should not have that permission, because it isn't guaranteed that the result of a mixin application has a non-trivial noSuchMethod. If it has a non-standard signature (like int noSuchMethod(Invocation)) then we might be able to guarantee that there is a non-trivial noSuchMethod, but I'd prefer to ignore that. So other mixins: Nope!

This is now becoming way too niche imo. Mockito does override methods too because it widens the api signature.

What's the problem? If you change the mocked class then definitely you'd expect to have to run the code generation one more time as well. And it may then widen the api signature again, relative to the updated interface of the mocked class, including newly added members, if any.

What if some non-mocked function from a dependency starts using the mocked class in a new way and you hadn't set up mock expectations to handle the new function calls etc?

How would that be different from the situation today?

According to flutters breaking change policy as an example, that is a breaking change for you (if that test is in customer tests in the flutter repo).

The non-mocked function from a dependency would then still start using the mocked class in a new way, it is still a breaking change, and it would be handled exactly like it is today.

I'm not saying that breakage will magically go away, I'm just saying that there is nothing (that I've noticed) which would make the breakage worse when the subtype is a mock, compared to the situation where it is some other subtype of the base class.

The thing which is actually worse is that the mock is allowed to exist even though it won't run those base class constructors, and it doesn't have those private member implementations. But maybe it isn't so bad if we allow mocks to cheat in these ways because they are already cheating in so many other ways.

I don't know. I'm really just exploring the idea. If we end up dropping the idea completely then we know more about why it is a bad idea. ;-)

jakemac53 commented 1 year ago

Not sure what that is?

I meant type promotion - it was early still 🤣 .

That is already true for other subtypes of a base class, almost any change to the signature of a potentially overridden method is a potentially breaking change. A mock with an explicit method declaration is just one more class which can be broken in the same way. So you can't (safely) make that change in a base class anyway, nothing lost.

Afaik the pain is mostly around final and not base. In theory, mockito could extend a base class instead of implementing it, I think. Although it would be a change for sure, and it wouldn't be able to prevent the constructor body from running, so that might come with certain implications/limitations.

Lots of the other responses I think are around just handling base. I agree that is easier/safer to do, but it seems weird to me to allow opening that but not final, even if it is less dangerous. Most of the use cases today are people asking to open up final classes also.

How would that be different from the situation today?

It is different because today I can block people from mocking, period :).

But maybe it isn't so bad if we allow mocks to cheat in these ways because they are already cheating in so many other ways.

Yes, and I want to be able to not allow them to cheat :). Otherwise, what is the point?

eernstg commented 1 year ago

Yes, and I want to be able to not allow them to cheat :)

😄 I can follow that.

I do think we shouldn't touch final classes (it destroys too many potential benefits to allow mocking on them), so it's only about base classes. I changed the issue title to emphasize that point.

final would then actually prevent cheating. And we'd get to know as a community that a final class simply isn't mockable, period, and hence final wouldn't be used on any class which must be mockable.

It's kind of a delicate balance: Allowing a base class to be implemented in a different library, but only by a mock class, doesn't seem to create any huge problems. On the other hand, if mockito can use class MockC extends C with SomeMockThings ... then the justification for this special permission could be a lot weaker.

ds84182 commented 1 year ago

If you're consuming a library, and you want to mock it, either mock an interface exposed by the library or wrap the library in your own interface and mock that.

Libraries that choose to be mockable should expose implementable classes or suitable mocks. This can be accomplished with friend libraries to avoid namespace pollution from test-only mock and interface classes.

If a library is not mockable and you think it should be, that's an upstream bug. Consider contributing a fix or fork it.

No exceptions to the rules otherwise we're back where we started. Some things aren't meant to be mockable so there shouldn't be a generalized escape hatch/hack.

rrousselGit commented 1 year ago

For reference, I was actually considering marking a class as base because folks repeatedly incorrectly mock the class.

My class has some private members. Folks cannot "implement" the class. They have to extend it.
I was thinking that marking the class as base would tell folks more naturally that they could do class MyMock extends MyBaseClass with Mock instead of class MyMock with Mock implements MyBaseClass. (although folks would have to mark their class as "base" too, which is a bit inconvenient)

So ignoring the modifier would go directly against this.

eernstg commented 1 year ago

@rrousselGit wrote:

I was actually considering marking a class as base because folks repeatedly incorrectly mock the class.

Interesting! This is perfectly in line with arguments given in lots and lots of language team discussions about this feature, along the lines of "base class C is intended to indicate and enforce that this class yields a type that is tied to a certain amount of implementation (for instance, some members are private to the library of C, and we want to make sure we have an implementation of those)".

However, the main reason why I created this issue was that it might be appropriate to send this message and have this enforcement when it comes to "official subtypes" of C, and yet it might still be OK for a mock to break the rules. Instances of the mock class would be used in some very predictable and limited ways, and it might be known that those private members won't be invoked on the mock. (A test can be white-box, and in this case there is nothing wrong in knowing such things.) Another consideration is that a mock won't be broken by adding a new instance member to the superinterface (no more than the subclasses that the base modifier allows, anyway).

Anyway, what I'm hearing on this issue is that there is very little support for allowing mocks to implement a base superinterface. This is nice, too, because it keeps the whole ruleset simpler.

slaci commented 1 year ago

Originally I started this discussion on the flutter discord which resulted in #3106. My main problem was: when a flutter plugin's author added final to every class in the name of dart 3 upgrade, all my test broke down. Well in reality mockito could not generate the mocks anymore. I don't blame him, as currently if you read the documentation (dart class modifiers) as a package developer and you see final, then you know that's what you ever wanted, because it can prevent many sorts of bc breaks. However, the documentation does not mention that this prevents mocking (well there are only pros there currently) which may be obvious for lang developers and some people, but maybe the last thing the rest consider.

One problem is that previously everything was implementable (eg. mockable) so devs could write the @GenerateNiceMocks(MockSpec<PluginClass>()) one-liner and use the generated mock (as I did; bad for me). Now if the plugin starts using final, then it breaks everything and app devs have to create a wrapper class and refactor every usage (code and tests) to use that. This is defended by the fact flutter docs always have been recommended creating a wrapper and now some of us are seeing/experiencing why, the hard way of course. Previously we only had to create wrapper for those plugins which use global functions/things. This topic is explained more elegantly in #3106.

Adding this point:

As a package developer, one of the biggest benefits of putting final on a class is that it blocks mocking.

to the class modifiers docs would be helpful to make this visible for package developers (then they can decide if they care or not). Personally I don't see why disallowing mocking anything makes people so happy, but I see there are many people wants to prevent this for some (for me at least) unknown reasons. And this: "because folks repeatedly incorrectly mock the class.", I'm really curious how to incorrectly mock a class 🙃

rrousselGit commented 1 year ago

And this: "because folks repeatedly incorrectly mock the class.", I'm really curious how to incorrectly mock a class 🙃

Users generally mock things that shouldn't be mocked too.

A typical example would be a custom ChangeNotifier:

class Example with ChangeNotifier {
  void doSomething() {}
}

Users may want to mock Example.doSomething and write:

class ExampleMock with Mock implements Example {}

The problem is that this mocks ChangeNotfiier.addListener too. So while they did mock doSomething, their ChangeNotifier is pretty much unusable.

Instead folks should likely write:

class ExampleMock with Mock, ChangeNotifier implements Example {}

Where this overrides doSomethingbut not addListener

eernstg commented 1 year ago

@slaci wrote:

when a flutter plugin's author added final to every class in the name of dart 3 upgrade, all my test broke down

This is very interesting! I'd say that "obviously adding final to an existing class is a breaking change". But the "obviously" part may not be true... :smile:

slaci commented 1 year ago

@eernstg Yes, this "dart 3 upgrade" was signaled by semver also as breaking (bumped major), so really no blame on him. On my related mockito issue another guy reported that other plugin author added finals to a package classes too (I haven't checked if he bumped major too or anything, was some openai package iirc). There are mock related examples in the docs, but it seems like they are easy to overlook.

Yes adding final to an existing class is always breaking (not just mocks), which may be obvious. But how hard it breaks mocks may not be very obvious (I wouldn't have thougth about it either). And I think the issues are mainly about (at least #3106 surely) this. It was easy to create mocks before for any class, investigate if its possible to make it easy again with class modifiers. I'm sure this is why you are investigating the opening of the base classes, because we already have solutions which are more correct, but make mocking more tedious (and of course there are some which are less correct and the sealed black magic). All proposals which are trying to make it easy or easier (like this issue) are still open, so I just wanted to add my realworld experience.

@rrousselGit This is off-topic for this issue, so I just reply briefly, but please don't continue here. For me mocking means the following: I generate a mock with mockito and define the needed functionality by when(pluginObj.method()).then..() calls. So for me if I mock a changeNotifier, then thats the correct behavior if that doesn't subsribe to anywhere and doesn't do anything by itself. Trying to test non-testable code will produce these half-baked things, but I would be surprised if it would be solved by preventing mockability.

Wdestroier commented 1 year ago

Could it be possible to write the business logic and the tests in the same library? It's more practical to write the test code and update the test code when it's in the same file below the actual code. Then Mockito can generate sealed class BaseClassMock implements BaseClass {} in the same library.

Example of code and tests in the same file: https://github.com/cloudwego/pilota/blob/main/pilota/src/thrift/compact.rs#L1504

lrhn commented 1 year ago

Could it be possible to write the business logic and the tests in the same library?

That would most likely make package:test a required dependency for using your package in production. You wouldn't want that.

eernstg commented 1 year ago

@slaci wrote:

Yes, this "dart 3 upgrade" was signaled by semver also as breaking (bumped major), so really no blame on him.

I don't think it's a given that a package upgrade is breaking, just because it was performed in order to migrate the implementation of the package from language 2.19 to 3.0. For example, the package could include changes such that every class which could previously be used as a mixin are now changed to be mixin class declarations. Also, no existing class is forced to have any class modifiers. This is probably already enough to ensure that the package upgrade doesn't have any changes that are breaking because of the language upgrade. So if there are no other breaking changes then the package can get away with bumping a minor version, and it wouldn't violate semver as used by Dart.

Wdestroier commented 1 year ago

That would most likely make package:test a required dependency for using your package in production. You wouldn't want that.

I'm assuming the compiler can find a way to not bundle the dev_dependencies and drop the unused UserServiceTests classes. I agree people wouldn't be happy if everything was bundled in the final app.

jakemac53 commented 1 year ago

I'm assuming the compiler can find a way to not bundle the dev_dependencies and drop the unused UserServiceTests classes. I agree people wouldn't be happy if everything was bundled in the final app.

Even if tree-shaking was 100% perfect for this case (in reality it won't be), it would significantly increase compile times which would regress the development cycle, including hot reload.

There is typically far more test code than app code, you don't want all that code in your app even if it gets ultimately tree shaken out.

lrhn commented 1 year ago

Maybe the compiler can be smart. Maybe it cannot. It definitely means that you need package:test to compile the production code, because it will be a dependency of the library in lib/... that is being compiled. Along with every (plentiful) dependency of package:test. That increases the risks of version conflicts when you don't have to just match the actual dependencies of a package, but also all its dev-dependencies. And it risks someone using a package which is really just a dev-dependency in code that is retained in production, by mistake. Since there is no separation, there is no way to know if it's deliberate or not. (And @PublicForTesting will stop being useful, but probably also stop being needed.)

And that's if tree-shaking actually works perfectly, which I can guarantee it won't.

rrousselGit commented 1 year ago

Personally, the main appeal I see for the idea of disabling certain class keywords for mocks is the idea that there are multiple competing mock packages.

The current workaround to class modifier restrictions is to have the package exposing a class Foo to also expose a FooMock. But that implies the package is aware of which mocking library the users of the said package are using.

There are probably some alternate patterns to work around this, like exposing a "view" or a "fake" which users then use to build their mocks. But it's complexifying things quite a bit.


I also wonder if we can truly depend on noSuchMethod to determine if a class is mock.

On one side, folks may use noSuchMethod in their apps instead of tests, in which case it seems illegitimate if the class keywords are ignored.
On the flip side, I've had plans to make a mock package for a while. And I could see myself making a mock package that doesn't rely on noSuchMethod. After-all, we have code-generation, so we don't really need noSuchMethod to override all public APIs.

jakemac53 commented 1 year ago

And I could see myself making a mock package that doesn't rely on noSuchMethod. After-all, we have code-generation, so we don't really need noSuchMethod to override all public APIs.

Fwiw mockito already had to switch to a codegen approach for null safety, because it has to widen some types to be nullable. Afaik though it no longer relies on noSuchMethod either.

jakemac53 commented 1 year ago

I also wonder if we can truly depend on noSuchMethod to determine if a class is mock.

Fwiw I think the idea was more that any class defining noSuchMethod can more safely implement a base class, it at least can't really be statically broken by new members being added.

eernstg commented 1 year ago

I also wonder if we can truly depend on noSuchMethod to determine if a class is mock.

... it at least can't really be statically broken by new members being added.

Exactly, that was my main motivation for relying on noSuchMethod.

The other basic idea was that the blame could be shifted: Normally, we want to ensure that client classes won't break unexpectedly, and base or interface can be used to allow client subtypes to exist, and still reserve the right to make certain changes (in the sense that they won't be breaking).

However, mock classes might get special privileges in return for being blamed if they do break. In particular, if a mock class is allowed to have implements BaseClass where BaseClass is declared in a different library L then the mock instances would have throwing implementations of all members private to L, but it is my problem as a mock class maintainer to use the mock instances in such a controlled manner that no such thing happens. Given that mock classes would in general be highly incomplete approximations of the real thing, this must be the conditions that mocks are working under, anyway.

In any case, I still take the response to this issue to mean "keep it simple, we don't want to give any exemptions".

lrhn commented 1 year ago

It's always dangerous to infer intent from code patterns.

Saying that a class with non-trivial noSuchMethod has special rules may just make people give all their classes noSuchMethod(i) => super.noSuchMethod(i); declarations.

So yes, I don't want to give any exceptions. I'd rather create new patterns suited to the current language, than break restrictions based on how old code patterns looked.

It has a learning curve, a migration cost, etc., but it gives a consistent language at the end.

You can extend a base class, that's enough to mock it using code generation and overrides. So do that, if you really want to mock.

You cannot extend a final class. If you want to, you must provide a non-final subclass. (Which can be an empty abstract base mixin class FooForMock implements Foo {} that is not exported in the public API, and which can be used precisely like an interface would, just using with instead of implements, and carrying a transitive base requirement).

You can do that for the base class too, to have an empty class for mocking, if you don't want to have to generate every member.

And it will turn off any benefits you'd get from the compiler for declaring the classes base or final, which is the cost of having abstract extensible subclasss with no member declarations. Can't have it both ways.

slaci commented 1 year ago

@lrhn Declaring a class as interface class may seem weird, but in dart it would be the simplest alternative to what you are describing (if I understand correctly). It prevents extending the class itself, but it makes possible to implement (eg. mock) it. This was the solution for the package where I reported this final mocking issue first and we called it good enought.

This is not equivalent to final, because custom (userland) implementations may break when you change method signatures (as the interface is open), but seems to be equivalent to splitting your class into a base + final or abstract interface + final or introducing some mixins. It brings very little if anything compared to final though (no bc break prevention, no potentional perf optimization), but as we see currently you have to open the interface somehow for the mocking use-case (or the sealed hack 😱) and that does not need to be overcomplicated.

stuartmorgan commented 1 year ago

You can extend a base class, that's enough to mock it using code generation and overrides. So do that, if you really want to mock.

You cannot extend a final class. If you want to, you must provide a non-final subclass. (Which can be an empty abstract base mixin class FooForMock implements Foo {} that is not exported in the public API, and which can be used precisely like an interface would, just using with instead of implements, and carrying a transitive base requirement).

If the only realistic way to mock things becomes generating every member, it seems like the easier new pattern (for anything that doesn't require passing instances of the class outside of the package boundary) would be what I raised in one of the other issues: for the generator to generate an interface and an implementation that's a passthrough to the real class. That gives people a mockable interface to use in their own code, without special steps being taken by every package author.

eernstg commented 1 year ago

This will not happen.