Customer-reported issue with NNBD migration tool #44280

escamoteur · 2020-11-21T22:48:57Z

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!

The text was updated successfully, but these errors were encountered:

stereotype441 · 2020-11-23T17:06:12Z

Hi @escamoteur, I cloned fluttercommunity/get_it@dd56ba9 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 · 2020-11-23T17:56:14Z

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

escamoteur · 2020-11-23T21:15:03Z

@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 · 2020-11-23T21:32:03Z

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 · 2020-11-23T21:38:00Z

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

escamoteur · 2020-11-23T21:59:21Z

why is param1 and param2 not nullable?

escamoteur · 2020-11-23T22:42:05Z

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 · 2020-11-23T22:46:00Z

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 · 2020-11-24T19:29:47Z

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 #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 · 2020-11-24T19:33:05Z

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 #38470.

stereotype441 · 2020-11-24T19:36:09Z

@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 · 2020-11-24T20:49:38Z

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 · 2020-11-24T22:10:00Z

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 #44311 to track this issue.

stereotype441 · 2020-11-24T22:10:47Z

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 · 2020-11-25T09:39:42Z

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 · 2020-11-25T20:06:37Z

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 · 2020-11-26T05:24:27Z

@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 · 2020-11-26T12:24:36Z

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 · 2020-11-27T01:03:48Z

Thank you, I can reproduce it.

scheglov · 2020-11-27T08:20:55Z

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

escamoteur · 2020-11-27T08:26:23Z

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 · 2020-11-27T20:30:56Z

@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.

…LIBRARY_INTO_NULL_SAFE. Bug: #44280 Change-Id: I6a8ef25deaedc55ef33e83ba7614f23fd56fe788 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174220 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

escamoteur · 2020-11-28T19:36:31Z

@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.

stereotype441 · 2023-11-21T21:57:26Z

As of 1c7fe71, the null safety migration tool has been removed from active development and retired. No further work on the tool is planned.

If you still need help, or you believe this issue has been closed in error, please feel free to reopen.

lrhn added the area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). label Nov 22, 2020

This was referenced Nov 23, 2020

Pubspec apparently allows \ as path separator under windows but not linux dart-lang/pub#2769

Closed

Consider doing custom flow analysis during migration tool's EdgeBuilder stage. #40566

Closed

stereotype441 added the P2 A bug or feature request we're likely to work on label Nov 23, 2020

This was referenced Nov 23, 2020

Migration tool should assume assertions enabled. #44292

Closed

Migration tool sometimes generates (expr as type?)! #44293

Closed

Migration tool bizzarely blames ! expression as a source of nullability #44294

Closed

stereotype441 mentioned this issue Nov 24, 2020

Migration: add imports to main library files, not part files #44309

Closed

stereotype441 mentioned this issue Nov 24, 2020

In the migration tool, it's confusing that added hints don't turn red until after the migration is re-run. #44311

Closed

stereotype441 closed this as completed Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customer-reported issue with NNBD migration tool #44280

Customer-reported issue with NNBD migration tool #44280

escamoteur commented Nov 21, 2020

stereotype441 commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

escamoteur commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

escamoteur commented Nov 25, 2020

escamoteur commented Nov 25, 2020

scheglov commented Nov 26, 2020

escamoteur commented Nov 26, 2020

scheglov commented Nov 27, 2020

scheglov commented Nov 27, 2020

escamoteur commented Nov 27, 2020

scheglov commented Nov 27, 2020

escamoteur commented Nov 28, 2020

stereotype441 commented Nov 21, 2023

Customer-reported issue with NNBD migration tool #44280

Customer-reported issue with NNBD migration tool #44280

Comments

escamoteur commented Nov 21, 2020

stereotype441 commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

escamoteur commented Nov 23, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

escamoteur commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

stereotype441 commented Nov 24, 2020

escamoteur commented Nov 25, 2020

escamoteur commented Nov 25, 2020

scheglov commented Nov 26, 2020

escamoteur commented Nov 26, 2020

scheglov commented Nov 27, 2020

scheglov commented Nov 27, 2020

escamoteur commented Nov 27, 2020

scheglov commented Nov 27, 2020

escamoteur commented Nov 28, 2020

stereotype441 commented Nov 21, 2023