Fail early when running dart migrate with too-high min SDK constraint

[kwalrath]

If you try to run dart migrate when your minimum SDK constraint is 2.12+, you get confusing analysis errors. Instead, you should get a message like this:

This package is opted into null safety, but it has null safety errors.
Try using a lower minimum SDK constraint.
More information: https://dart.dev/go/null-safety-migration

Here's the output of dart --version:

Dart SDK version: 2.12.0-31.0.dev (dev) (Mon Nov 9 16:07:01 2020 -0800) on "macos_x64"

---

Details

Migrating a very simple package, I updated my minimum SDK to my Dart version (flutter master channel's dart version, 2.12.0-31.0.dev) so I could get null-safe dependencies (pedantic & test). dart pub outdated --mode=null-safety was happy. But when I tried dart migrate, it warned me about analysis issues:

$ dart migrate
Migrating <path/to>/examples/util

See https://dart.dev/go/null-safety-migration for a migration guide.

Analyzing project...
[----------------------------------------------------------------------------------------/]
3 analysis issues found:
  error • An expression whose value can be 'null' must be null-checked before it can be dereferenced at lib/dart_version.dart:6:15 • (unchecked_use_of_nullable_value)
  error • The argument type 'String?' can't be assigned to the parameter type 'String' at lib/dart_version.dart:6:15 • (argument_type_not_assignable)
  error • A value of type 'Null' can't be returned from function 'ellipsis' because it has a return type of 'T' at lib/ellipsis.dart:2:20 • (return_of_invalid_type)

Note: analysis errors will result in erroneous migration suggestions.

Please fix the analysis issues (or, force generation of migration suggestions by re-running with --ignore-errors).

[mit-mit]

@stereotype441 I think we should consider this for beta. If you can get this landed in master, I can handle cherry picking.

[stereotype441]

Unfortunately we can't simply check the user's min SDK constraint, because there are reasonable scenarios where it makes sense to run migration over a tool whose min SDK constaint has already been updated (namely, when resuming migration of a package that has already been partially migrated).

What we can do is detect the situation where the package contains errors and appears to be completely migrated; I think that should address this use case.

I'm currently testing https://dart-review.googlesource.com/c/sdk/+/171624 on trybots. It causes the following text to be printed after the list of errors:

Note: analysis errors will result in erroneous migration suggestions.

All files appear to have null safety already enabled.  Did you update the
language version prior to running "dart migrate"?  If so, you need to un-do this
(and re-run "pub get") prior to performing the migration.


Please fix the analysis issues (or, force generation of migration suggestions by re-running with --ignore-errors).

@kwalrath @mit-mit does this seem like it would address the problem? I'm happy to change the wording if needed.

[stereotype441]

https://dart-review.googlesource.com/c/sdk/+/171624 has code review and passes trybots. I'm going to land it now so that it can get cherry-picked to Beta ASAP. If we want to change the wording I'm happy to do that as a follow-up CL.

[kwalrath]

How about a message like this:

The migration tool didn't start, due to analysis errors.

If your code doesn't use null-safe features yet, then
set the lower SDK constraint (in pubspec.yaml) to a
version before 2.12, and then run `dart pub get`.

If you use null-safe features, then we recommend fixing the analysis issues.
Or, although you might get erroneous migration suggestions,
run `dart migrate --ignore-errors`.

More information: https://dart.dev/go/null-safety-migration

[stereotype441]

@kwalrath that sounds fine to me, but I have a clarifying question. Currently the tool has the ability to output a different message for the scenario where there are analysis errors but no opted out code (in which case the user probably just needs to set their lower SDK constraint back) and the scenario where there are analysis errors and some opted out code (in which case the user probably just has errors in their code, and it's unrelated to their lower SDK constraint). Are you proposing that message for both scenarios or just one of them? If you're proposing it for just one of them, do you have a recommendation for the error message we should display in the other scenario?

[kwalrath]

If you can differentiate, I'd love different messages for the two cases.

For the first case, something like this:

The migration tool didn't start, due to analysis errors.

The following steps might fix your problem:
1. Set the lower SDK constraint (in pubspec.yaml) to a version before 2.12.
2. Run `dart pub get`.
3. Try running `dart migrate` again.

More information: https://dart.dev/go/null-safety-migration

And in the second case:

The migration tool didn't start, due to analysis errors.

We recommend fixing the analysis issues before running `dart migrate`.
Alternatively, you can run `dart migrate --ignore-errors`, but you might
get erroneous migration suggestions.

More information: https://dart.dev/go/null-safety-migration

[stereotype441]

@kwalrath thanks! I'll make a CL that updates the text. Re-opening the issue until that CL lands.

[stereotype441]

Text update sent for review: https://dart-review.googlesource.com/c/sdk/+/171643

[stereotype441]

@mit-mit this is now landed in master. 2 CLs will need to be cherry-picked:

• https://dart-review.googlesource.com/c/sdk/+/171624
• https://dart-review.googlesource.com/c/sdk/+/171643

[leafpetersen]

Cherry pick filed here.