avoid casts on calls to mock methods

[jmesserly]

For example:

  class Monster {
    // Returns current health after applying [hp] damage to this monster.
    int damage(int hp);

    // Returns current health after applying [f()] damage to this monster.
    int damageFromClosure(int f());
  }
  class MockMonster extends Mock implements Monster {}

  var mock = new MockMonster();
  when(mock.damage(argThat(isPositive))).thenReturn(100);
  when(mock.damageFromClosure(argThat(isFunction))).thenReturn(200);

We'll want to allow mock.damage to avoid the cast to int. This is safe because when using nSM to implement abstract methods, the method is conceptually implemented by a method that forwards to nSM, and that method can widen the argument types to Object.

[jmesserly]

Hey @leafpetersen ... I just thought of an interesting complication here. Continuing my example above, we'd have to know to disallow this kind of override:

class DerivedMockMonster extends MockMonster {
  // override error: int -> int is not a subtype of Object -> int from MockMonster
  int damage(int hp) => hp.isEven ? 0 : 1000;
}

... because that would tighten up the parameter type.

If we say nSM "implements" all abstract methods and widens parameter types to Object, we could do it, but it disallows overrides later on.

Implementation wise, we'd want to track the induced members explicitly in the element model as synthetic ones. So a bit tricky.

But conceptually I'm not sure how I feel about it. Is it too restrictive? Using nSM becomes kind of dangerous as it will silently "widen" your abstract members.

Perhaps a better pattern is to alter the mock package API:

verify(mock).damage(argThat(isPositive)).thenReturn(100);
verify(mock).damageFromClosure(argThat(isFunction)).thenReturn(200);

By wrapping the mock object it's possible to accept different signatures. I would assume Java Mockito has a similar issue, for the same reasons we do. edit: I suppose in Java they can generate a class on the fly & use overloads, which we don't have.

[leafpetersen]

My initial (somewhat tentative) thinking was that this wouldn't be the default behavior, but something you opted into via an annotation or syntax at the NSM declaration site. So a standard NSM codegens with the right signature, but if you put the annotation there (@Widen?) then it codegens with the widest possible signature. This does forbid narrowing it again - does that matter for our use case? My thinking was not. If it does, then we could basically use the same mechanism as our planned covariant override support to allow re-narrowing.

[thso]

For some context, the DerivedMockMonster

class DerivedMockMonster extends MockMonster {
  // override error: int -> int is not a subtype of Object -> int from MockMonster
  int damage(int hp) => hp.isEven ? 0 : 1000;
}

is an example of mixing faking and mocking, which is a highly discouraged pattern. The damage override will take precedence on any stubbing, which can lead to hard to debug tests. The recommended way would be to set common behavior in the setUp of the test:

setUp(() {
  when(mockMonster.damage(any)).thenAnswer((int hp) => hp.isEven ? 0: 1000);
});

For the mocking use-case, I don't believe preventing narrowing again is an issue.

[jmesserly]

@thso @leafpetersen do y'all like the opt-in annotation idea? It would only need to appear on the nSM, so derived classes don't need to worry about it:

class Mock {
  // ...
  @SomeAnnotationHere()
  noSuchMethod(Invocation invocation) { /* ... */ }
}

Any naming ideas? Since it's not going to occur very frequently something that feels more explanatory might be preferable to a really short name, e.g. @allowAnyArgumentType or @noCastsOnArguments ...

Alternatively we could make it the default, and let the covariant overrides feature be used for narrowing, if that's ever needed. It sounds like it isn't needed for mockito, and I think they are one of the main users of nSM.

[leafpetersen]

Yes, I'm in favor. Perhaps @WidenParameters, or just @Widen?

[thso]

I'm in favor of making it optional, I don't think it's a behavior one would expect in other circumstances. @WidenParameters looks good to me!

[jmesserly]

sounds good to me!

fyi, this is kind of blocked for the same reason as:
#25578 (comment)

... but as soon as we get that resolved (one way or the other) I can take a look

[jmesserly]

sent out patch for 25578 so picking this one back up

[jmesserly]

actually ... going to wait for that patch to land so I can build on it. The CL for it is https://codereview.chromium.org/2336503003/

[thso]

Thanks @jmesserly! Would you have some vague ETA on this?

[jmesserly]

thanks for the ping. I need to think it over again to see if there's a nice way of implementing it. Even after the 25578 fix, it didn't seem clear how we can fit it in nicely. But in the meantime I've been trying to (slowly) get things into a more hackable state. Stay tuned.

[leafpetersen]

@thso This is proving a bit complex to implement, I'd like to get a sense of how much this is a blocker for things we need, and how much this is a nice to have (and if the former, how urgently).

[jmesserly]

ping, are we still interested in a feature like this? I think it would be challenging to have to implement it in both CFE and Analyzer at this point. Assuming stale, but let me know if we should reopen