Incorrect type in mocks with analyzer 5.x

[stuartmorgan-g]

We're having a strange issue with mockito generating the wrong types in webview_flutter_android when trying to update analyzer. I have tried to make a minimal repro case, but haven't been able to recreate the problem so far so filing with what I have for now.

STR:

1. git clone https://github.com/flutter/packages
2. cd packages/packages/webview_flutter/webview_flutter_android
3. Edit pubspec.yaml to change the pigeon dependency to pigeon: ^9.0.0. (This allows analyzer to resolve to 5.7.1 instead of 4.x.) then flutter pub get
4. flutter pub run build_runner build --delete-conflicting-outputs
5. flutter test test/android_webview_test.dart

The test fails to compile:

 Error: The return type of the method 'MockWebView.getScrollPosition' is
'Future<dynamic>', which does not match the return type, 'Future<Offset>', of the overridden method,
'WebView.getScrollPosition'.
 - 'Future' is from 'dart:async'.
 - 'Offset' is from 'dart:ui'.
Change to a subtype of 'Future<Offset>'.
  _i4.Future<dynamic> getScrollPosition() => (super.noSuchMethod(
                      ^
lib/src/android_webview.dart:331:18: Context: This is the overridden method ('getScrollPosition').
  Future<Offset> getScrollPosition() {
                 ^

There are similar problems with a Future<Size>.

If you skip step 3, everything is fine, and the generated code has the correct type. Comparing the lock files, it's just analyzer that changes between them (and pigeon of course, but I'm not re-running Pigeon generation, so its version is irrelevant).

I tried adding Offset to the @GenerateMocks list in android_webview_test.dart to see if that would help, and that causes (with 5.x only): Invalid @GenerateMocks annotation: The GenerateMocks "classes" argument is missing, includes an unknown type, or includes an extension.

I made a simple project that just has a class with a Future<Offset> method in it, and couldn't reproduce this, but I haven't yet been able to figure out what the important difference is.

[jakemac53]

This worked fine for me following these instructions - I confirmed I got the latest verison of all packages as well.

What flutter version are you on, and are all your other deps fully upgraded?

Fwiw I tested with:

Flutter 3.9.0-1.0.pre.34 • channel master • [email protected]:flutter/flutter.git
Framework • revision c590086bcc (29 minutes ago) • 2023-03-03 12:24:17 -0500
Engine • revision 569c62d110
Tools • Dart 3.0.0 (build 3.0.0-290.0.dev) • DevTools 2.22.2

[scheglov]

I also cannot reproduce, with an older version of Flutter, and with the latest on master.

[samyakkkk]

Reproducible for us on:

Flutter 3.7.3 • channel stable • [email protected]:flutter/flutter.git
Framework • revision 9944297138 (3 weeks ago) • 2023-02-08 15:46:04 -0800
Engine • revision 248290d6d5
Tools • Dart 2.19.2 • DevTools 2.20.1

mockito: ^5.3.2

[stuartmorgan-g]

I'm on

Flutter 3.8.0-13.0.pre.61 • channel main • https://github.com/flutter/flutter
Framework • revision df13ea248a (2 weeks ago) • 2023-02-16 14:18:50 -0500
Engine • revision e4cb80e22e
Tools • Dart 3.0.0 (build 3.0.0-244.0.dev) • DevTools 2.21.1

[jakemac53]

I tried on 3.7.3 and it didn't reproduce for me. Can you confirm you are also running a flutter pub upgrade?

[stuartmorgan-g]

Okay, it turns out I thought I'd run exactly those steps but hadn't, and it is something about stale artifacts. The real STR is to do: 1, 2, 4, 3, 4, 5

That will give the bad output.

git clean -xfd ., then re-running flutter pub get and flutter pub run build_runner build --delete-conflicting-outputs, gives correct output.

Sorry for the confusion. I'm not sure if this still qualifies as a bug, or if "delete absolutely everything" is a required step when updating analyzer.

[jakemac53]

It should not be a required step, no, so we should leave this open.

[jakemac53]

Ok, I was finally able to reproduce this - the critical difference is when updating the pigeon package, if you only run a flutter pub get, then it will reproduce. If you run a flutter pub upgrade it does work as expected, you will get a newer version of build_resolvers, which is likely what is making the difference?

Given it does work with the latest versions I am going to close the issue, if you can confirm that works for you as well.

[jakemac53]

In particular build_resolvers 2.1.0 is probably necessary, it has a changelog entry that reads "Return all SDK libraries in Resolver.libraries, making the implementation consistent with the documentation.". That seems likely to be the fix you need.

[stuartmorgan-g]

We can close it; I do wonder if other people will do the same thing since flutter pub get after updating is common (in particular, it's what VS Code's Flutter plugin does automatically when editing pubspec.yaml).

Maybe mockito should add an otherwise-unnecessary build_resolver dependency of 2.1.0+ to avoid it? But that could cause more issues long term if there are major version bumps to build_resolver.

[jakemac53]

I feel very strongly that packages should not update their constraints on other packages just to try and ensure that users get a certain bug fix. Especially if that package has no pre-existing dependency on the package. This is the path towards version lock and it would be a death spiral for the ecosystem.

I am not sure why running flutter pub get is the default thing people do - there is probably some sort of failing in our documentation, tutorials, etc here that is encouraging the wrong thing. The first step when hitting a bug in a package dependency should be to try and upgrade all your dependencies, as this is how you get bug fixes. I would actually go as far as to say that there is only exactly one case that somebody should ever use get over upgrade - immediately after pulling from (or cloning) a repo which checks in their pubspec.lock file (this should only apply to application packages). And dart run etc already do this for you anyways, so that means effectively no user should ever type flutter pub get.

Essentially, get is for tools (to avoid unexpectedly upgrading things), upgrade is for users.

[stuartmorgan-g]

I feel very strongly that packages should not update their constraints on other packages just to try and ensure that users get a certain bug fix.

I generally agree, and we mostly don't do this in federated plugins. I thought it might be worth considering here since the failure is so bizarre and hard to understand, but I don't feel strongly about it.

that means effectively no user should ever type flutter pub get

I suspect (based on what I see in flutter/flutter issue reports) that a lot of users never type either, and just let VS Code run it for them when they modify their pubspec.yaml. (People ~never getting updates to transitive dependencies is definitely a real problem in the broader community.)

[jakemac53]

I suspect (based on what I see in flutter/flutter issue reports) that a lot of users never type either, and just let VS Code run it for them when they modify their pubspec.yaml. (People ~never getting updates to transitive dependencies is definitely a real problem in the broader community.)

Yeah that does make sense, and it is probably the right thing for a tool to do. But we probably are missing some tooling that would notify users when their versions are getting out of date?

[natebosch]

since flutter pub get after updating is common (in particular, it's what VS Code's Flutter plugin does automatically when editing pubspec.yaml)

Perhaps this is the bug that we should try to fix?

[stuartmorgan-g]

People ~never getting updates to transitive dependencies is definitely a real problem in the broader community.

As an illustrative example that just came in, in case anyone is curious: flutter/flutter#121928 is a case where someone has transitive dependencies (implementation packages of a federated plugin) that are at least two years old.

[jakemac53]

I do think there is definitely some piece of tooling here that is missing, to nudge people to upgrade their deps.