new NSM forwarding logic breaks Mockito

[srawlins]

Open items to move forward:

• Landing Mockito fix [@srawlins]
• Rolling Mockito in Flutter [@srawlins] (Bump mockito to 3.0.0-alpha+5 flutter/flutter#17442)
• Fixing ~10 internal tests [@srawlins]
• Landing CFE implementation [@sjindel-google]
• Landing DDC implementation. [@jmesserly]
• Communicating / updating the spec [@lrhn @leafpetersen]

@sjindel-google 's 7d5025e breaks how Mockito works. Given

class Cat {
  bool eatFood(String food, {bool hungry}) => true;
}

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

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

The user should now be able to stub out eatFood like so:

when(cat.eatFood("bird")).thenReturn(true);

However, Mock's noSuchMethod receives something different after 7d5025e: it used to receive an Invocation with 1 positional argument, and 0 named arguments. Now it receives the 1 positional argument, and the 1 named argument (with the default value specified by the parameter). The presence of arguments or lack thereof, in the Invocation passed to noSuchMethod is core to Mockito's implementation.

This is blocking the current Flutter roll (flutter/flutter#17230), as Flutter has some affected tests. This will also block the SDK roll into internal Google, as we use Mockito.

I also imagine this is just... a big breaking change to how NSM works. I wouldn't be surprised if this broke other tools that depend on NSM functionality.

I think this change must be rolled back ASAP, to unblock Flutter. We can then discuss whether the change is possible for Dart 2.

Related to #33019

CC @sjindel-google @keertip @tvolkert @yjbanov

[keertip]

cc @kmillikin @mraleph

[aam]

Samir actually reverted 7d5025e on 24dd9b4, but that broke the buildbots blocking further commits and there was no clear reason for the revert, so to unblock the buildbots I re-reverted it on 9a7e1f6.

[aam]

Closed via 2765fcf

[srawlins]

Thanks, @aam

[lrhn]

The behavior of the noSuchMethod forwarder forwarding all declared parameters is correct and what we want.

I'm not sure what causes Mockito's issue here - it should get equivalent Invocations both when configuring and when using the mock, so it should still be able to match them up and return the expected value.

It is correct that you can no longer make a mock act differently depending on whether an optional parameter is omitted, or it is provided with the same value as the default value. A real Dart method could never detect that (apart from a short period where we didn't know better), so the mock should still be perfectly capable of emulating any valid behavior.

So, we will have to reapply this fix to make nSM forwarders do the correct thing.

[kmillikin]

This is complicated to follow. I think this is what happened in order:

1. 7d5025e landed giving us the intended behavior but breaking Mockito
2. 24dd9b4 reverted that for <INSERT REASONING HERE>, giving us the incorrect behavior that Mockito relies on
3. 9a7e1f6 reverted that because it broke the presubmit buildbots, giving us the intended behavior again and breaking Mockito
4. 2765fcf reverted that because it broke Mockito, giving us the incorrect behavior again

We do intend to implement the intended behavior (almost by definition) so I think we will have to change Mockito.

[srawlins]

I'll look into how Mockito can handle this fix. I expect that the code of Mockito users will have to change, which means we'll need a few weeks or months runway. I'll comment here when I know more.

[dgrove]

Also note that such a breaking change would block rolls of the SDK into google3 (and hence Flutter) until those changes in user code could be made (ie, I'm not sure at all that this change can be made for Dart 2).

[srawlins]

For discussion on how/whether Mockito can work with this proposed change, I've opened dart-lang/mockito#122.

[srawlins]

Honest question, was 7d5025e written because like, some package that uses noSuchMethod benefits from the change, or did we just notice that nSM was being handled wrong, and we don't want to be wrong?

I think we have a workable solution for Mockito, but it definitely makes Mockito less usable. Is there a net benefit? Does some other package become more usable, or possible?

[matanlurey]

I did some sleuthing internally. noSuchMethod is referenced about 1400 times of which:

• 1200 times do not reference package:mockito
• 860 times do not just forward to super.noSuchMethod(invocation)

Of the remaining cases, I mostly see stuff like:

noSuchMethod(_) {}
noSuchMethod(_) => null;
noSuchMethod(_) => throw new UnimplementedError();

So I agree with @dgrove and @srawlins this change should be, at best, suspended until after Dart2 (Dart3). Mockito is basically the most used case for nSM, followed by the following three patterns above.

[matanlurey]

And, I see Mockito used over 30K times. So... Not volunteering for that migration :)

[tvolkert]

My 2c: this change seems an obviously correct one that makes things noticeably better. The only question is how to feasibly roll it out.

[matanlurey]

@tvolkert Correct behavior aside if this will block us for weeks/months it isn't the right change today.

[mraleph]

This should be classified as a language change and the decision is then a language team decision.

If migration is unlikely - then we should amend Dart 2 specification making the current behavior "the specified" behavior.

/cc @leafpetersen @lrhn

[jmesserly]

I also figured out what was wrong with my CL (forgot to pass isConst: true after much refactoring to DDC) and am testing the cleaned up version.

edit: thanks for the test case @leafpetersen showing the problem :)

[tvolkert]

Just to make sure I understand the current state of things: it sounds like we're going to be able to re-land 7d5025e unmodified because we found viable workarounds in mockito (that comes with a workable migration path) and DDC?

[jmesserly]

Just to make sure I understand the current state of things: it sounds like we're going to be able to re-land 7d5025e unmodified because we found viable workarounds in mockito (that comes with a workable migration path) and DDC?

I think it will be reverting https://dart-review.googlesource.com/c/sdk/+/53540 ... it looks like there maybe have been some small tweaks to the CL to keep buildbots happy, but otherwise I presume it's almost the same code as 7d5025e, yes. mockito has a workaround and DDC has an implementation (we're still testing, but it seems like it's close)

[sjindel-google]

Unfortunately we can't land 7d5025e just yet due to a few failures on precompiled bots. Please let me know when I can re-land it with fixes.

[vsmenon]

@sjindel-google - is your fix ready to go if we decide to go forward with the new semantics?

The overnight internal run looks promising - ~10 test failures. @srawlins - what do you think?

[sjindel-google]

Yes, the fix is ready and the revision is approved. I'm just wait for the go-ahead.

[vsmenon]

The internally tested CL (195908155) includes @srawlins's Mockito fix and @jmesserly's DDC implementation. So, rolling forwarding means:

• Landing @sjindel-google 's CFE implementation.
• Landing @jmesserly 's DDC implementation.
• Landing @srawlins 's Mockito fix.
• Fixing ~10 internal tests (@srawlins )
• Communicating / updating the spec (@lrhn @leafpetersen )

Are we missing anything? Is there a chance something will break in Flutter or internal Flutter apps? Are the VM AOT/JIT and Dart2JS all covered by Samir's fix?

[sjindel-google]

I don't know if Dart2JS will be covered, but I expect so since we inject concrete methods. In your list above, the Mockito fix and updating internal tests needs to be done first, right?

[vsmenon]

Yes, I think we'll need the Mockito fix landed and rolled into Flutter and internal.

[srawlins]

Agree with everything above. @tvolkert or @yjbanov might correct the Flutter bit; it could be that all of Flutter was cleaned up in that fun roll last week. Otherwise, we should land and release mockito fix, and bump mockito in flutter. I can do these things today.

[vsmenon]

Thanks! I added check boxes to the initial comment at top so we can track.

[vsmenon]

@srawlins - are we still waiting on the flutter roll?

[srawlins]

Yeah flutter/flutter#17487

[srawlins]

Landed in flutter. nSM Forwarding is cool to land in the SDK.

[vsmenon]

Thanks, Sam!

@sjindel-google and @jmesserly - can you please land your impl CLs?

[jmesserly]

@jmesserly - can you please land your impl CLs?

yeah I tried, it failed for a status issue (it needs test fixes from @sjindel-google 's CL). Trying CQ again w/ that test temporarily marked failing in DDC (we can mark passing once CFE CL lands).

[jmesserly]

FYI the DDC tracking bug is #33019 for status updates.

[keertip]

@jmesserly , see quite a few test failures in DDC internally after your commit, testing at commit 4b239de, previous tested at 4a323f8.

Invalid argument(s): A typed argument was passed in as a named argument named "Symbol("cancelOnError")", but did not pass a value fornamed. Each typed argument that is passed as a named argument needs to specify the namedargument. For example:when(obj.fn(x: typed(any, named: "x"))).

@srawlins, do we need an update to mockito package to fix these?

[srawlins]

@keertip Yes we need to update mockito to fix those.

[sjindel-google]

Should I still land the VM side?

[vsmenon]

@sjindel-google - yes, please. :-)

[vsmenon]

@sjindel-google and @jmesserly - are we done on the implementation side?

[sjindel-google]

Yes.

[vsmenon]

@srawlins - are you unblocked from fixing those ~10 tests?

@leafpetersen @lrhn - how should this change be communicated?

[srawlins]

Yes, I can fix them when we roll >=dev.56 internally.

[vsmenon]

@srawlins - thanks!

@leafpetersen is working on how to communicate this as part of overall NSM changes. I'll go ahead and close this bug.