Migration tool is a noop (`Suggestor.generatePatches()` threw unexpectedly)

[creativecreatorormaybenot]

Description

When running riverpod migrate in a project I work on, the migration tool only suggest to change the dependency to flutter_riverpod: ^1.0.0 and then completes.

When running pub get afterwards, I get about 600 analysis errors (while there are 0 errors before the migration).

Details

In detail, this is what the logs show:

riverpod migrate
[INFO] Setting up analysis contexts...
[INFO] done
searching...
[SEVERE] Suggestor.generatePatches() threw unexpectedly.
Bad state: No element
[INFO] Setting up analysis contexts...
[INFO] Setting up analysis contexts...
[INFO] done
[INFO] done
searching...
pubspec.yaml:119
-   flutter_riverpod: ^0.14.0+3
+   flutter_riverpod: ^1.0.0

Accept change (y = yes, n = no [default], A = yes to all, q = quit)? 
y
Migration finished successfully please run `flutter pub upgrade`

Versions

Flutter 2.5.3 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 18116933e7 (5 weeks ago) • 2021-10-15 10:46:35 -0700
Engine • revision d3ea636dc5
Tools • Dart 2.14.4

I am not sure how to create a minimal sample for this right now. I am glad to follow up with any details if you have any idea regarding how to further debug Suggestor.generatePatches().

[rrousselGit]

There's not much we can do about without extra informations.

Could you share some of those analysis errors?

[creativecreatorormaybenot]

@rrousselGit There are none - the migration tool does nothing and then obviously there are errors after changing to 1.0.0.

[rrousselGit]

But we need a way to reproduce this, otherwise there's not much we can do.

Try extracting some of those unmigrated code in a separate project and see if they are properly migrated.

[TimWhiting]

If you can give me access to the code it would be much easier to diagnose the error. Like Remi said, we probably need more context than just #937 and this issue.

You said that the project is using pre-null safe code in #937. I would recommend migrating to null-safety first, and then using the migration tool. Also, can you try accepting the patches one at a time, rather than all at once? (i.e. press y until it fails rather than A). That would also tell us more about the issue.

The error shown here is from the codemod package, not from our specific migration, so I'm not sure what issue we are running into here. The specific issue in #937 I might be able to figure out a bit easier because I print out the stacktrace of our codemod failure, but I'm suspecting it is related to null-safety like you mentioned.

Something to note is that the try / catch for migration is on a per-file basis, so if you can separate things out into more files, you might get more files migrated. If it fails due to an exception in one file, it will skip migrating that file.

[creativecreatorormaybenot]

@TimWhiting @rrousselGit

I would recommend migrating to null-safety first, and then using the migration tool.

We will do so and let you know the outcome.

Also, can you try accepting the patches one at a time, rather than all at once?

There is only one patch, i.e. the 1.0.0 patch.

Something to note is that the try / catch for migration is on a per-file basis, so if you can separate things out into more files, you might get more files migrated.

There are hundreds of files in this project, using Riverpod.

[TimWhiting]

Also, can you try accepting the patches one at a time, rather than all at once?

There is only one patch, i.e. the 1.0.0 patch.

Patches are one line / couple of lines at a time, not a full file or all at once. You can accept individual 'small changes' that are part of a larger migration (the migration to 1.0.0). For example this suggested patch only changes this single line in the pubspec.

-   flutter_riverpod: ^0.14.0+3
+   flutter_riverpod: ^1.0.0

Accepting patches one at a time will help diagnose the problem.

[creativecreatorormaybenot]

@TimWhiting I am not sure I am following what you are saying. I am fully aware of how the migration tool works, but there is literally only that one suggested patch, so after pressing y once:

Migration finished successfully please run flutter pub upgrade

This message appears, which means that the hundreds of files with Riverpod usage are ignored.

[TimWhiting]

That's interesting. It definitely shouldn't be skipping all of those files, and should be printing more exception related info. You are running it in the directory with the pubspec and there aren't any analysis errors to begin with right?

I've been able to reproduce some issues that might be related that I'm working on. But missing a whole project I can't reproduce.

[creativecreatorormaybenot]

You are running it in the directory with the pubspec and there aren't any analysis errors to begin with right?

Yes.

[kmod-midori]

We are experiencing the same issue, some suggestions were provideded before hitting [SEVERE] Suggestor.generatePatches() threw unexpectedly. The error messages does not provide exactly where do the errors come from (we have ~30 places).

However, we are using this on a fairly large internal codebase, and we are not able to share further details. I am suggesting our internal code to move away from riverpod given what I've seen in the RFC and the ineffective migration tool.

[TimWhiting]

@chengyuhui Are you sure it didn't migrate anything? This error is expected, maybe I should clarify that. We cannot handle every possibility, and there are bound to be some things that cannot be accounted for. What was unexpected as reported by @creativecreatorormaybenot is that it did nothing to his code. However, it should have migrated most of your code. Remember to run flutter pub get after the migration, since the changes will be analyzed as errors until you upgrade your dependencies. Also, there was recently an update to the migration tool. Try reverting any changes that the migration tool did, and then rerunning the migration tool. flutter pub global activate riverpod_cli && flutter pub get && riverpod migrate. The migration tool is well tested, but ultimately can only be as good as the error reports we get because automatically migrating code is hard, and the way people use it is not always as expected. The migration tool is being continually improved, the 1.0.0 migration is still being worked on based on error reports. If you can wait, I would suggest keep trying the migration tool and reporting issues until it is able to migrate 95% of your code.

If you want to blame someone, you can blame me for the ineffective migration tool. I contribute to open-source because I want to help improve the tools I use. You should not base your decision to use / not-use this great library from Remi because of it. The library was pre-1.0.0 and there was no guarantee of no major changes, so you can't blame the RFC and breaking changes for making the library more fully featured and consistent. This is an open-source project, and I don't get paid to volunteer my spare time.

[kmod-midori]

I have hooks_riverpod: ^1.0.1 in my pubspec.yml, the first run of riverpod migrate corrected this to hooks_riverpod: ^1.0.0 before erroring out with [SEVERE] Suggestor.generatePatches() threw unexpectedly. (which is better than nothing)

The second run corrected some of the standalone hooks that uses useProvider before giving [SEVERE] Suggestor.generatePatches() threw unexpectedly.

After that, the migration tool does nothing but giving out that error.

One of the failed case looks like this:

bool useIsCurrentItem(String songId) {
  return useProvider(mediaItemProvider.select((m) {
    final v = m.data?.value;
    return v != null && v.id == songId;
  }));
}

None of the StatefulHookConsumerWidget nor HookConsumerWidget cases were corrected and I ended up manually migrating everything else.

The simplest one looks like this (logcatProvider is a StreamProvider.autoDispose<List<String>>):

class LogcatPage extends HookWidget {
  @override
  Widget build(BuildContext context) {
    final logcatStream = useProvider(logcatProvider);

    final lines = (logcatStream.data?.value ?? []).reversed.toList();

    return Scaffold(
        body: ListView.builder(
            itemBuilder: (context, index) => _buildLine(lines[index]),
            itemCount: lines.length));
  }

  Widget _buildLine(String line) {
    var spacePos = line.indexOf(" ", min(line.length, 6));
    if (spacePos == -1) {
      return Container();
    }
    final time = line.substring(0, spacePos);
    final content = line.substring(spacePos + 1);

    return ListTile(
      title: Text(time),
      subtitle: Text(content),
    );
  }
}

I appreciate this project and everyone's efforts, but am quite frustrated since in #335 I kept expressing my concerns about the amount of changes that needs to be done in a project that uses riverpod heavily. Some of the participants kept saying that automated migration tools would solve my concerns about having to change everything related to useProvider.

[TimWhiting]

The main problem, then is that you should not change the pubspec to a newer version before migrating.

From the docs: Now that the command line is installed, we can start using it.

First, open the project you want to migrate in your terminal. Do not upgrade Riverpod. The migration tool will upgrade the version of Riverpod for you. Make sure that your project does not contain errors. Execute: riverpod migrate

You should not have changed to depend on hooks_riverpod: ^1.0.1 before running the migration tool. It is impossible to migrate a project that has analysis errors (and where we don't know which version we are migrating from).

It is strange that it changed the pubspec, since it should have done nothing if the version is newer than the version we are migrating to.

The tool is not intended to do anything after migrating the first time. It only migrates from one version to the next. Not from partial 1.0.0 to 1.0.0. The process is to try migrating from one version to the next, and then if it fails and you don't want to do a manual migration, report the error and revert until a fix for the migration is available.

I understand your frustration, I was concerned about the migration of useProvider as well, but I tried to do something about it by creating the migration tool. I have personally been concerned by the lack of feedback on the migration tool, it makes it very very hard to improve, when all of my tests are passing, but when migration is complex enough that I'm concerned it doesn't handle everything.

[rrousselGit]

About that, it's probably worth adding a warning if riverpod migrate is executed on an app with version ^1.0.0

[kmod-midori]

I reverted all changes (including my manual version upgrade, the project does not have any analysis error and builds successfully at this point) with Git and tried again, the CLI tool did the version upgrade from 0.14.0+5 to 1.0.0, and changed the function hook described above to

bool useIsCurrentItem(WidgetRef ref, String songId) {
  return ref.watch(mediaItemProvider.select<bool>((m) {
    final v = m.data?.value;
    return v != null && v.id == songId;
  }));
}

Based on Git diff, none of the code except pubspec.yml and the code above were changed.

After that, it gives

[INFO] Setting up analysis contexts...
[INFO] done
searching...
[SEVERE] Suggestor.generatePatches() threw unexpectedly.
Bad state: No element
[INFO] Setting up analysis contexts...
[INFO] Setting up analysis contexts...
[INFO] done
[INFO] done
searching...
[SEVERE] Empty patch suggested: <SourcePatch: on /m:/workspace/<redacted>/pubspec.yaml from 40:9 to 40:25>
[SEVERE] Empty patch suggested: <SourcePatch: on /m:/workspace/<redacted>/pubspec.yaml from 40:9 to 40:25>
Migration finished successfully please run `flutter pub upgrade`

That empty match error points to the line that declares hooks_riverpod

BTW, the documents on pub.dev does not mention that the user should not try to upgrade manually.

[rrousselGit]

The doc (https://riverpod.dev/docs/migration/0.14.0_to_1.0.0/#usage) explicitly says:

Do not upgrade Riverpod. The migration tool will upgrade the version of Riverpod for you.

[rrousselGit]

I'll make it clearer though. It could be highlighted better

[kmod-midori]

I forgot how I came to that, but I simply googled riverpod_cli after digging around, which took me to https://pub.dev/packages/riverpod_cli, and the Usage section on that page forgot to mention that, perhaps we should?

[rrousselGit]

Doc or not, the tool should be clearer about this anyway.

I'll update it to warn if the project uses a version 1.0.x already, since not everyone will read the doc

[TimWhiting]

This is strange, it seems like we are getting quite a few Bad state: No element, I'll see if I can reproduce, but it will be hard without knowing which part of the code it is working on when that happens. In the meantime, I think I can log more errors, where I thought I verified there could be no exceptions.

As far as the empty patch, that should be easier to fix. Are you just importing hooks_riverpod, not plain riverpod? Maybe I missed something when just importing one or the other... It is strange that the empty patch happens twice. How are you importing hooks_riverpod? Are you using an 'any' constraint or a ^0.14.0+5 constraint?

[kmod-midori]

I always import hooks_riverpod with explicit version constaint such as ^0.14.0+5.

import 'package:flutter_hooks/flutter_hooks.dart';
import 'package:hooks_riverpod/hooks_riverpod.dart';

It will certainly help if the tool can provide information about the location that it is working on when the error is thrown, since there are too many things to migrate in my project. If we are able to point this to a section of the code, I might be able to provide examples.

I'll revert all the changes for the time being and go back to the old version because of this and the refreshing problems with FutureProvider, but will come back to this once we are able to find out which file is the offender.

[TimWhiting]

I've got my Masters Defense in a couple of days, and am not sure how much time I'll have. However, I have PR #973 that will help us diagnose the issue better I think & will probably make the migration continue even if it errors in one file, the rest will migrate.

You can test it out yourself before it gets published by:

git clone https://github.com/TimWhiting/river_pod
cd river_pod
git checkout log_more_exceptions
cd packages/riverpod_cli
dart pub get
dart pub global activate -s path .

// go to your project
riverpod migrate

Just note that it might spit out errors that are unrelated to riverpod code, since it is visiting every function / method / class. The best way to tell how successful it was is by doing a git diff or doing a pub upgrade after migration, and seeing how much you have to do by hand.

[rrousselGit]

I've deployed a warning in care you're using the CLI on a project already using 1.0.0

I'll look at a better way of logging errors. There has to be a better solution than adding try/catch everywhere and hope that we got them all.

[TimWhiting]

The right thing to do is to not use non-null assertions, or list methods like .first or .last, but I'm often using those since I assume that for example the build method has a parameter list and the parameter list has a first parameter. I can document the reasoning behind the null-checks or the assumptions I'm making about build methods etc, but that is the reasoning behind the current try / catch system.

Probably we can create some wrappers like

expectNonNull(node.parameters?.parameters, (params) {
  expectHasFirst(params, (first) {

  });
});

Which would log warnings if that was not true.

I can do this after I'm done with my Thesis defense. (Probably wouldn't get to it till Saturday). There is a lot that I'd like to clean up about the migration tool, since there are so many migrations / changes that are intertwined. I'd like to create some helper utilities etc.

[rrousselGit]

I'll close this. I have no plan to fix the remaining bugs for the migration tool as it's been quite a while now since the breaking change happened.

[nglro]

How much memory shoud I have to be able to run this ? It fails on both a pc and mac, both with 32GB ram.

MacBook-M2 LOKOFOOD_V2 % riverpod migrate [INFO] Setting up analysis contexts... [INFO] done searching... Exhausted heap space, trying to allocate 449600 bytes. Exhausted heap space, trying to allocate 21506272 bytes. [SEVERE] Suggestor.generatePatches() threw unexpectedly. Out of Memory