Sdk: Customer-reported issue with NNBD migration tool

[escamoteur]

I'm trying to migrate my get_it package but I have some issues. The migrate tool want's to make types nullable that are not necessary. Because the tool does not excepts code that has already null safe operators like 'late' or '!' you can't help the tool.

It would be good if you could deselect proposed changes but still be able to let the other changes be done.
I also observed several added casts, that are not really necessary.

If you want to try it with get_it yourself, the type 'T' of get, getAsync and call could be defined as 'extends Object` which I tried but still the tool wants to change the return type into a nullable T

If you are interested, we could do a walkthrough together. Just ping me on Twitter @thomasburkhartb

Dart SDK version: 2.12.0-29.10.beta

Thanks for filing!

[stereotype441]

Hi @escamoteur, I cloned https://github.com/fluttercommunity/get_it/commit/dd56ba992994eb91838d12a9191115066782327a and was able to reproduce your problem.

Thanks so much for your bug report! It looks like there are a number of coding patterns in the get_it package that the migration tool stumbles on. I'll spend some time investigating them and file separate issues for each one.

In the meantime, your best bet to work around the issues is probably to use the migration tool's "hint" feature to tell it where it's come to incorrect conclusions. You can use /*!*/ to mark a type as non-nullable or /*?*/ to mark a type as nullable, and you can use /*!*/ to mark an expression that needs to be null checked. For example, if you modify the GetIt class like this, then the migration tool does a better job:

T/*!*/ get<T>({String instanceName, dynamic param1, dynamic param2});
Future<T>/*!*/ getAsync<T>({String instanceName, dynamic param1, dynamic param2});
T/*!*/ call<T>({String instanceName, dynamic param1, dynamic param2});

You can either add those /*!*/s yourself, or you can add them directly in the tool; click on something the tool migrated incorrectly, look in the lower right window of the tool to see why it did what it did, and use one of the Add /*!*/ hint buttons to correct its wrong assumptions. Then click the "rerun" button, and the migration tool will re-evaluate things taking the new hint into account; this should hopefully fix incorrect migrations elsewhere.

I hope that helps!

[escamoteur]

Thanks, I was pretty sure that my get_it is a good test candidate :-)

[escamoteur]

@stereotype441 there should be an explanation in the tool what the /!/ and /?/ are for. I had no idea that I can set hints this way.

[escamoteur]

in get_it_impl.dart

I found this here

I know why, because instance is a field but it's totally confusing after you just made null check in the surrounding if

[escamoteur]

Why did it insert this casts that make no sense at all

[escamoteur]

why is param1 and param2 not nullable?

[escamoteur]

after I did apply refactoring.

In VS code I get always errors from the analyser. it seems that there is something wrong.

!! PLEASE REVIEW THIS LOG FOR SENSITIVE INFORMATION BEFORE SHARING !!

Dart Code extension: 3.16.0
Flutter extension: 3.16.0 (activated)

App: Visual Studio Code
Version: 1.51.1
Platform: win

Workspace type: Dart, Flutter
Analyzer type: LSP
Multi-root?: false

Dart SDK:
    Loc: C:\Entwicklung\Flutter\bin\cache\dart-sdk
    Ver: 2.12.0-29.10.beta
Flutter SDK:
    Loc: C:\Entwicklung\Flutter
    Ver: 1.24.0-10.2.pre

HTTP_PROXY: undefined
NO_PROXY: undefined

Logging Categories:
    General, Analyzer

Mon Nov 23 2020 [23:37:21 GMT+0100 (Mitteleuropäische Normalzeit)] Log file started
[23:37:24] [Analyzer] [Info] ==> Content-Length: 191
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":4,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"position":{"line":703,"character":37}}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 203
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":5,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"position":{"line":703,"character":37}}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 62
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","method":"$/cancelRequest","params":{"id":4}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 118
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"method":"$/progress","params":{"token":"ANALYZING","value":{"kind":"begin","title":"Analyzing…"}},"jsonrpc":"2.0"}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"id":4,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 266
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":6,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"range":{"start":{"line":703,"character":37},"end":{"line":703,"character":37}},"context":{"diagnostics":[]}}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"id":5,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8

{"id":6,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 93
Content-Type: application/vscode-jsonrpc; charset=utf-8

{"method":"$/progress","params":{"token":"ANALYZING","value":{"kind":"end"}},"jsonrpc":"2.0"}
Mon Nov 23 2020 [23:37:30 GMT+0100 (Mitteleuropäische Normalzeit)] Log file ended

you can find this state of the sources here
https://github.com/fluttercommunity/get_it/tree/null_safety

[escamoteur]

When trying to run a get_it_test.dart I get this errors here

Which are somewhat surprising because the tool made the change to firstWhereOrNull

Good night!

[stereotype441]

When trying to run a get_it_test.dart I get this errors here

Which are somewhat surprising because the tool made the change to firstWhereOrNull

Good night!

Aha, it looks like the tool added the necessary import to make this work, but it erroneously added it to the top of lib/get_it_impl.dart; that's wrong because that file is a part file. It should have added it to lib/get_it.dart instead. I filed https://github.com/dart-lang/sdk/issues/44309 to track this problem.

(The tool should have also adjusted your pubspec to add a dependency on the collection package. So before you run your tests you'll need to run pub get in order to bring in that dependency).

[stereotype441]

in get_it_impl.dart

I found this here

I know why, because instance is a field but it's totally confusing after you just made null check in the surrounding if

Agreed, this is definitely something the tool could do better. This is covered by https://github.com/dart-lang/sdk/issues/38470.

[stereotype441]

@stereotype441 there should be an explanation in the tool what the /*!*/ and /*?*/ are for. I had no idea that I can set hints this way.

We did provide an explanation, but I guess it wasn't easy enough to find. If you click on the "help" button in the upper right corner of the tool, it takes you to https://github.com/dart-lang/sdk/blob/master/pkg/nnbd_migration/README.md, which explains about the hints.

@kwalrath do you think we should make some updates to https://github.com/dart-lang/sdk/blob/master/pkg/nnbd_migration/README.md to link it up better with other public-facing documentation of the migration tool?

[escamoteur]

I think it wasn't clear what the hint does. Also what happens if a hint is turned red?

Thanks for your reply. Would be awesome if you could have a look at the other issues above especially the continously crashing analyser. Which makes further fixing of problems almost impossible

[stereotype441]

I think it wasn't clear what the hint does. Also what happens if a hint is turned red?

The hint /*?*/ on a type tells the migration tool "please ensure that this type is nullable; i.e. after migration, the code should have a ? here".

The hint /*!*/ on a type tells the migration tool "please ensure that this type is not nullable; i.e. after migration, the code should not have a ? here".

(/*!*/ hints can also be applied to expressions but let's not complicate our discussion with that right now).

Hopefully @kwalrath (who is in charge of documentation) will have further ideas about how to make this clearer so that other users don't stumble on it.

The migration tool uses red to indicate text that is going to be removed when the migration is performed. So when you add a /*!*/ hint to a type, it's completely red because the hint is only there for the benefit of helping the migration tool make better decisions; once you accept the migration it won't be needed anymore, so the migration tool will remove it. Similarly, when you add a /*?*/ hint to a type, the /* and */ are red because once you accept the migration, the /* and */ are going to be removed and just the ? will remain.

Perhaps some of the reason this was confusing is that when you first click on the "add hint" buttons, the text isn't red yet; it only becomes red later after you've re-run migration. I've filed https://github.com/dart-lang/sdk/issues/44311 to track this issue.

[stereotype441]

Thanks for your reply. Would be awesome if you could have a look at the other issues above especially the continously crashing analyser. Which makes further fixing of problems almost impossible

@scheglov @devoncarew @srawlins could one of you look at the analyzer crash?

[escamoteur]

The migration tool uses red to indicate text that is going to be removed when the migration is performed. So when you add a /*!*/ hint to a type, it's completely red because the hint is only there for the benefit of helping the migration tool make better decisions; once you accept the migration it won't be needed anymore, so the migration tool will remove it. Similarly, when you add a /*?*/ hint to a type, the /* and */ are red because once you accept the migration, the /* and */ are going to be removed and just the ? will remain.

Perhaps some of the reason this was confusing is that when you first click on the "add hint" buttons, the text isn't red yet; it only becomes red later after you've re-run migration. I've filed #44311 to track this issue.

exactly that. Maybe also think about using another color than red, because I associate it with an error

[escamoteur]

Aha, it looks like the tool added the necessary import to make this work, but it erroneously added it to the top of lib/get_it_impl.dart; that's wrong because that file is a part file. It should have added it to lib/get_it.dart instead. I filed #44309 to track this problem.

No, that isn't a problem, it's not a part file.

(The tool should have also adjusted your pubspec to add a dependency on the collection package. So before you run your tests you'll need to run pub get in order to bring in that dependency).
Bu I just checked pubspec and it hasn't added a ´collections´ entry.

[scheglov]

@escamoteur Regarding the crash. I don't see any stack trace in the messages. Could you point at one, or (better) provide more details to to reproduce one?

[escamoteur]

after I did apply refactoring.

In VS code I get always errors from the analyser. it seems that there is something wrong.

!! PLEASE REVIEW THIS LOG FOR SENSITIVE INFORMATION BEFORE SHARING !!

Dart Code extension: 3.16.0
Flutter extension: 3.16.0 (activated)

App: Visual Studio Code
Version: 1.51.1
Platform: win

Workspace type: Dart, Flutter
Analyzer type: LSP
Multi-root?: false

Dart SDK:
    Loc: C:\Entwicklung\Flutter\bin\cache\dart-sdk
    Ver: 2.12.0-29.10.beta
Flutter SDK:
    Loc: C:\Entwicklung\Flutter
    Ver: 1.24.0-10.2.pre

HTTP_PROXY: undefined
NO_PROXY: undefined

Logging Categories:
    General, Analyzer

Mon Nov 23 2020 [23:37:21 GMT+0100 (Mitteleuropäische Normalzeit)] Log file started
[23:37:24] [Analyzer] [Info] ==> Content-Length: 191
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":4,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"position":{"line":703,"character":37}}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 203
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":5,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"position":{"line":703,"character":37}}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 62
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","method":"$/cancelRequest","params":{"id":4}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 118
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"method":"$/progress","params":{"token":"ANALYZING","value":{"kind":"begin","title":"Analyzing…"}},"jsonrpc":"2.0"}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"id":4,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] ==> Content-Length: 266
[23:37:24] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":6,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///c%3A/Entwicklung/packages/get_it/lib/get_it_impl.dart"},"range":{"start":{"line":703,"character":37},"end":{"line":703,"character":37}},"context":{"diagnostics":[]}}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8
[23:37:24] [Analyzer] [Info] <== {"id":5,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 144
Content-Type: application/vscode-jsonrpc; charset=utf-8

{"id":6,"jsonrpc":"2.0","error":{"code":-32003,"message":"Invalid file path","data":"c:\\Entwicklung\\packages\\get_it\\lib\\get_it_impl.dart"}}
[23:37:24] [Analyzer] [Info] <== Content-Length: 93
Content-Type: application/vscode-jsonrpc; charset=utf-8

{"method":"$/progress","params":{"token":"ANALYZING","value":{"kind":"end"}},"jsonrpc":"2.0"}
Mon Nov 23 2020 [23:37:30 GMT+0100 (Mitteleuropäische Normalzeit)] Log file ended

you can find this state of the sources here
https://github.com/fluttercommunity/get_it/tree/null_safety
@scheglov clone the above repository on master or beta and open it in VS code.
I don't know where I should get a stack trace from is that the observatory log?

[scheglov]

Thank you, I can reproduce it.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/174220

[escamoteur]

https://dart-review.googlesource.com/c/sdk/+/174220

Attention, in this case I don't use any part directive, the problem was that 'collection' wasn't added to the pubspec.yaml

[scheglov]

@escamoteur Yes, my change does not fix this migration issue, it only fixes the analyzer crash.

But the code does use parts. I'm looking into get_it/lib/get_it.dart:

library get_it;

import 'dart:async';

import 'package:async/async.dart';
import 'package:meta/meta.dart';

part 'get_it_impl.dart';

And the part file get_it/lib/get_it_impl.dart has:

import 'package:collection/collection.dart' show IterableExtension;
part of 'get_it.dart';

And this import directive in the part causes the crash - we don't set any element to import/export in parts, and a part of code the analyzer missed this fact.

[escamoteur]

@scheglov Sorry, I really forgot that I added the part stuff some time ago.
Thanks. will move the import and see how it looks like then.