Emulating tearoffs hack doesn't work under Dart 2 semantics about mockito

[aam]

However, under Dart 2 semantics, since getSound(String _) is part of the interface for MockCat, a forwarder function for getSound is created, which will call Mock.noSuchMethod when invoked. Evaluation of cat.getSound now creates a tearoff of this forwarder. Mock.noSuchMethod isn't called, and the Mock class doesn't get a chance observe the method name and arguments.

Shouldn't presence of forwarder function be transparent to the developer? Isn't it a problem that cat.getSound now refers to synthesized tearoff, rather than results in noSuchMethod call?

Does Dart 2 have special provisions for noSuchMethod forwarders, which were not in Dart 1?

[sjindel-google]

No, the presence of the forwarder is very much visible, by design of the language feature.
The forwarders exist for two reasons:

1. When you tear off an abstract method from a class with NSM, rather than call NSM with a getter Invocation, you get a closure which calls NSM with a regular Invocation when invoked.
2. Type-checking argument and return types for abstract methods before/after the NSM call to preserve soundness.

[srawlins]

Thank you for identifying this, @sjindel-google !! I wish we could capture and block this behavior within mockito. "Error: you cannot mock a synthetic method tear off." but I can't see a simple fix. We'll just have to document it. ☹️

[aam]

No, the presence of the forwarder is very much visible, by design.

Clearly, synthetic forwarders get in the programmer's way if programmer's intention was to get noSuchMethod invoked when non-existent method is called(but for which forwarder was automatically synthesized).

[sjindel-google]

It's not my place to comment on the design of this language feature, but note that the forwarders do not change the behavior of a regular call to a non-existent method; only the behavior of tearing it off.

[aam]

@eernstg I don't think this scenario was identified in https://github.com/dart-lang/sdk/blob/master/docs/language/informal/nosuchmethod-forwarding.md as something that gets broken, was it?

[aam]

It's not my place to comment on the design of this language feature,

Hopefully @eernstg can weigh in on this.

but note that the forwarders do not change the behavior of a regular call to a non-existent method; only the behavior of tearing it off.

Yeah, but from layman's perspective it's not like regular calls are "better" than tearoffs, are they? :-)

[eernstg]

Have to go now (have a nice weekend!), will look at it Monday.

[srawlins]

Ouch, this conflicts with Dart 1 mode as well:

import 'package:mockito/mockito.dart';

// Real class
class Cat {
  String sound() => "Meow";
  bool eatFood(String food, {bool hungry}) => true;
  int walk(List<String> places);
  void sleep() {}
  void hunt(String place, String prey) {}
  int lives = 9;
}

// Mock class
class MockCat extends Mock implements Cat {}

// mock creation
var cat = new MockCat();

void main() {
  var tearOff = cat.eatFood;
  verifyNoMoreInteractions(cat);
}

$ dart a.dart
Unhandled exception:
No more calls expected, but following found: MockCat.eatFood
#0      fail (package:test/src/frontend/expect.dart:164:30)
#1      verifyNoMoreInteractions (package:mockito/src/mock.dart:716:7)
#2      main (file:///Users/srawlins/code/dart-mockito/a.dart:21:3)
#3      _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:279)
#4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:165)

😲 😮 😵

[eernstg]

I generally agree with @sjindel-google's comments above. Also, I can see that the techniques used in Mockito are affected by the new NSM semantics.

In order to respond to the following, I think it's worth revisiting the design decisions:

Shouldn't presence of forwarder function be transparent to the developer?

No, this change is known to be breaking in certain ways. For the underlying motivation, there is a soundness part, a performance part, and a convenience part.

Taking the convenience part first, the generated forwarders will ensure that an NSM implementation can focus on the behavior of the emulated members and ignore tear-offs of the emulated methods, because the generated forwarders will "automatically" ensure that those methods can be torn off. It's always been a very common bug that NSM would handle a method invocation, but forget the tear-off.

This point of view implies that NSM should be used to emulate a particular interface (that is, it should support the methods that are expected to exist for the given type, no less and no more). But that matches quite well with the semantics of Dart (even Dart 1.x in checked mode, but certainly Dart 2): An object which is intended to play the role as an instance of a type T must in fact have type T if it is ever to be passed around in typed code (T t = myMock; fails if myMock does not have type T). It isn't very useful to be able to masquerade as a T if this will only work as long as the static type is dynamic (or the concrete class, e.g., MyMockingClassForT). So the support for emulating "an arbitrary type" basically never worked in Dart, anyway. Hence, Dart 2 makes it even more strict: NSM can be used to emulate the statically known set of members. For statically checked invocations you can't call anything outside of that set, and you can't call it with a wrong argument list shape.

This takes us to the soundness and performance parts: If we were to allow a statically typed instance method invocation to go directly to NSM (that is, not via a forwarder) then we would have an actual return type of dynamic (that is very likely, at least, and we cannot in general prevent the actual return type from being a top type like dynamic). This means that every call site for every instance method in every program must include a type check, unless we can use global static analysis to say things like "this call site will never hit NSM". So we can get soundness or performance, but not both.

In contrast, when every statically checked instance method invocation (that is, every call which is justified by the receiver type) is guaranteed to hit an actual method (a developer-written method implementation or a generated forwarder), we can rely on that method to enforce its declared return type, so there is no need for general caller-side checks. Moreover, it allows for things like a vtable based implementation, and in general it allows techniques where we "blindly" call method implementations from statically checked call sites, because it is guaranteed that said method exists and supports the given argument list shape.

In summary, I think the motivation for having forwarders is deeper than it may look, e.g., if only the 'convenience' part is mentioned, and it is known that this change is breaking.

However, it would obviously be really nice if we could keep everything working, including Mockito.

"Error: you cannot mock a synthetic method tear off."

I'm a little surprised about this because it uses when(cat.getSound) whereas examples on the Mockito github site seems to use a method invocation to indicate that we are setting up the behavior of a method. So why isn't it when(cat.getSound())?

That wouldn't work, of course, because it is a compile-time error to call getSound on a Cat with zero arguments, but (cat as dynamic).getSound() and cat.getSound(null) should work.

It may or may not be OK to use any of these two forms, such that Dart 2 is happy. On the other hand, you could say that it makes sense to require a proper invocation in order to set up the mocking behavior for "such an invocation", or to mark cat as dynamic when the invocation is not statically OK. So I really don't know whether that can be considered to be an acceptable solution.

Another idea is more tentative, but it might be possible to use the forwarders: when(cat.getSound) will pass the tear-off of the getSound forwarder to when. It's not easy to get hold of the receiver cat at that point, but it is easy to store the tear-off, and it might not be so hard to bring together that tear-off and the desired action (like thenReturn('Woof')) later on. For instance, when or NSM could invoke the tear-off with the receiver in a special status in order to obtain the symbol which will be the memberName when that method is invoked (in the normal "running" status), and then (if my random guesstimations about how Mockito works are reasonably accurate) it should be possible to set up the mocking behavior in a similar way as today. Similarly, an invocation of the tear-off could be handled in such a way that the returned value is the receiver object (`noSuchMethod(..) {... if (namedArgument == #getReceiver) return this; ...}), etc..

[aam]

Thank you for elaborated response, @eernstg.

It does sound that given language change forwarders is the best thing Dart2 can do to try to give original Dart1 NSM functionality to the developer. One thing that seems to be sticking out is the fact that forwarders-based solution will not provide original Dart1 NSM handling of tearoffs. You are saying it's a good thing, it might be folks who relied on NSM for tearoffs in Dart1(like Mockito) disagree, but what I think is missing is being clear in the spec or NSM-handling proposal that in Dart2 specifically NSM won't be invoked when accessing non-existent tearoff - you will always get a forwarder instead.

[eernstg]

Thanks! Note that I have a TODO on the table about going over the feature specification nosuchmethod-forwarding.md and checking how it deals with getter, setters, and operators (they all need forwarders in order to allow super-invocations, and in order to ensure type checks on any returned results). So that will be clarified/fixed soon.

NSM won't be invoked when accessing non-existent tearoff - you will always get a forwarder instead

Actually, a tear-off "exists" if and only if there is a corresponding method, which may be a generated forwarder or a developer-written method. So the only situation where we can even attempt to tear off a non-existing method is when the receiver has type dynamic. In that case we will get an NSM invocation.

But maybe you meant "NSM won't be invoked when tearing off a method which is in the interface of the class of the receiver, but which has no developer-written implementation" (that is, exactly when there is a generated forwarder)? That is true, and the feature specification doesn't seem to mention this case explicitly. I'll add a sentence to cover that.

[matanlurey]

I'm guessing this issue is resolved.