Mixin solution for unknown generic return type

[srawlins]

Background

In #338 (and #339), @MaxAuer wants to stub a generic method with a return type which is tied to a type parameter on the method. Something like:

class C {
  int get m => 0;
  String s(String a) => a;
  /// 100 other methods you'd like to stub...
  T add<T>(T data) => data;
}

Mockito cannot generate a mock class for C because it must be able to return valid values in add, but cannot create values for unknown type variable T. I'd like to explore one solution here, allowing the user to write a mixin which is mixed into the generated class. The mixin would implement one or more methods, and the generated class would not override those implementations with its own.

Proof of Concept

I started a PoC API which would allow the following:

@GenerateMocks([], customMocks: [MockSpec<Foo>(as: #MockFoo, mixingIn: MixinSpec<M>())])
void main() {}

mixin M on Foo, Mock {}

(I think the mixin has to have both superclass constraints, Foo, and Mock, in order to call super.noSuchMethod inside; can explore that later.)

This generates:

abstract class MockFoo_ extends Mock implements C {}

class MockFoo extends MockFoo_ with M {
  /// method overrides.
}

(Mockito codegen awkwardly needs to generate two classes because declaring class MockFoo extends Mock with M implements C {} results in an error, "M can't be mixed onto Mock because Mock doesn't implement C.")

The big problem

OK. All well and good. I wrote the API with the MixinSpec<M>() so that a user could specify type arguments if they so chose. Something like mixingIn: MixinSpec<M<int>>(). The problem is that I don't see any way to mix a generic mixin onto a generic mock class. Take this simple example:

class Foo<T> {}
mixin M<T> on Foo<T>, Mock {}

// Hope to generate:
class MockFoo<T> extends MockFoo_<T> with M<T> {}

What API would let you specify, "Uh, just tie the type parameter on M to the type parameter on MockFoo?" What about a slightly more complicated arrangement:

class Foo<T> {}
mixin M<T> on Foo<Future<T>>, Mock {}

// or

class Foo<T, U> {}
mixin M<T> on Foo<String, T> {}

I don't see an easy way to specify generics. :( The good news is this should be unfathomably bizarre and rare, to want to specify your mixin like this.

Perhaps the best way forward is to just specify the mixin class with a symbol (mixingIn: #M), and never specify type arguments (instantiate to bounds).

CC @matanlurey @TedSander @natebosch

[eggnstone]

Is this a working workaround?
I don't see the param "mixingIn" in MockSpec.

[srawlins]

Mockito 5.1.0 contains a nice solution with a unsupportedMembers parameter on MockSpec.

[stereotype441]

In my work on private field promotion (dart-lang/language#2020), I've discovered a new use case for this feature, and I think we should consider implementing it after all.

First some background: the goal of private field promotion is to allow code like this to work:

class A {
  final int? _x;
  ...
}
void f(A a) {
  if (a._x != null) {
    print(a._x + 1); // Ok, because `a._x` is not null.
  }
}

In order for this to be sound, the compiler has to check if there's a class anywhere that implements A and provides an implementation of _x that's not a final field; if it finds such a class, it needs to disable field promotion for A._x. Naively, it seems like the compiler only needs to look in the same library as class A, since _x is a private name. But that's not actually true, because of noSuchMethod. Another library could contain a class like this:

class B implements A {
  bool _b = false;
  Object? noSuchMethod(_) => (_b = !_b) ? 1 : null;
}

In effect, noSuchMethod opens up a loophole allowing classes outside of a library to override the implementation of a library's private members in a way that might violate the assumptions made by the library author. We have to close this loophole in order for private field promotion to be sound, but we would like to close it anynow, because in essence, it's a flaw in Dart's privacy system. The way we plan to close the loophole is by ensuring that an attept to invoke _x on an instance of class B always throws an exception; it doesn't go through the noSuchMethod mechanism. More generally, any call to a private non-existent method, getter, or setter, will throw an exception, unless the private name is in the same library as the class.

The consequence to Mockito is that it's no longer possible for tests to use Mockito's returnNullOnMissingStub feature to ensure that calls to private members will silently succeed. Consider this code (based on a real-world example from Google's internal codebase):

// lib1.dart
class C {
  int get value => ...;
  Object? _other;
}
int foo(C? c) {
  c._other = ...;
  return c.value;
}

// test.dart
import 'package:mockito/mockito.dart';
import 'lib1.dart';

@GenerateMocks([], customMocks: [
  MockSpec<C>(returnNullOnMissingStub: true),
])

void main() {
  test('foo', () {
    final mockC = MockC();
    when(mockC.value).thenReturn(1);
    expect(foo(mockCurrentPlaceModel), 1);
  });
}

The purpose of the test is to verify that foo returns whatever value it got from C.value. The fact that foo also stores a value in C._other isn't particularly relevant. But it's important that this doesn't cause an exception to be thrown. Today, this works because when foo tries to call _other= on an instance of class MockC, that gets dispatched to Mock.noSuchMethod, which returns null (because it was configured to do so using returnNullOnMissingStub: true). But when we close the noSuchMethod privacy loophole, this will stop working, because when foo tries to call _other= on an instance of class MockC, an exception will be thrown.

The way I'm trying to fix this in the internal code base is to create a mixin in the library containing the private name, and use it in the mock class, e.g.:

// lib1.dart
class C {
  int get value => ...;
  Object? _other;
}
@visibleForTesting
mixin MockCMixin implements C {
  @override
  Object? _other;
}
int foo(C? c) {
  c._other = ...;
  return c.value;
}

// test.dart
import 'package:mockito/mockito.dart';
import 'lib1.dart';

class MockC extends Mock with MockCMixin implements C {
  @override
  int get value => (super.noSuchMethod(Invocation.getter(#value), returnValue: 0) as int;
}

void main() {
  test('foo', () {
    final mockC = MockC();
    when(mockC.value).thenReturn(1);
    expect(foo(mockCurrentPlaceModel), 1);
  });
}

But unfortunately, in order to do this, I can no longer take advantage of Mockito codegen. In particular, I have to override the value getter to ensure that when(mockC.value) doesn't crash. It would be much better if I could instead say:

@GenerateMocks([], customMocks: [
  MockSpec<C>(returnNullOnMissingStub: true, mixingIn: MockCMixin),
])

[natebosch]

The fact that foo also stores a value in C._other isn't particularly relevant. But it's important that this doesn't cause an exception to be thrown.

FWIW I consider it a bug that mockito allows this behavior today. A library that uses private members like this does not support mocking for those values, and it's an accident that it happens to work today. If we need to allow the continued bad pattern to unblock migrations that could be an OK tradeoff, but it's not a pattern I'm happy to support.

[srawlins]

I didn't see your comment there, @natebosch, but you bring up a good point.

I think I can prototype a breaking change where we define, in a generated mock class:

bool wasIGeneratedFromTheSameLibraryWhereTheClassToMockWasDeclared

and if that's false, then in noSuchMethod, I can just always throw if the Invocation's methodName starts with an underscore.

I can see how breaking this would be for internal google3. If it's expensive, we might have to kick that breaking change down the road...

[natebosch]

IIRC we've had discussions about removing this capability at the language level. Knowing how breaking it is for mockito users would be a good data point.

@leafpetersen - I can't seem to find a language issue but I think we've discussed how allowing a noSuchMethod implementations to create behavior for private members from other libraries, perhaps while discussing soundness for private final field promotion. Is that something we are still considering?

Edit: Is it perhaps dart-lang/sdk#47923 ? Do I understand correctly that it will change some noSuchMethod "forwarder" in to noSuchMethod "thrower"?

[leafpetersen]

The relevant discussion is centralized here.