Flutter framework null safety migration

leafpetersen commented 4 years ago

This tracks the migration of the Flutter framework code itself.

cc @a14n @Hixie

leafpetersen commented 4 years ago

@a14n My understanding is that you will be primary owner of this work. We'll be doing exploratory ports of packages over the next few weeks to get ready for this. Once we're ready to go with this, my understanding is that the process will look something like the following:

The core packages that the framework depends on need to be ported.
- These ports can either be done by landing a ported copy in a branch, or possibly by publishing versions with the appropriate migration tool hints and migrating against that. cc @stereotype441
@a14n and possibly others will iteratively work with the migration tool to add sufficient migration hints to the framework code that the migration tool can on demand produce a correctly migrated flutter framework.
- Correctly migrated here means no static errors and all tests pass.
We may try to run internal pre-submits with the generated trial port to shake out additional issues before landing. cc @athomas @davidmorgan
We will validate performance by running the trial port against benchmarks before landing. cc @athomas @a-siva
When we see functional and performance parity and believe that we are ready to land the port:
- The main Dart repo will turn the experiment on by default and increase the minor version (probably to 2.9).
- The ported framework (as generated by the migration tool) rolls to Flutter master
- The next dev release from Flutter and/or Dart is the first release with Null Safety available to users.
- Ported versions of the core set of packages required for a small Flutter app are landed and published. cc @kevmoo @srawlins

Does this seem reasonable? Issues or concerns about this? Things that I'm missing?

stereotype441 commented 4 years ago

Sounds good to me

a14n commented 4 years ago

Sounds good to me

leafpetersen commented 4 years ago

@a14n , @ferhatb brought up the question of the flutter engine code this morning. Are you expecting to port that code as well? Note that there are two versions of it: the native version which is largely API surface that calls into C++, and the web_ui version which is largely written in Dart. These would need to be exactly in sync at the API level.

Hixie commented 4 years ago

@jason-simmons or @cbracken are probably the right people to do engine changes if there's engine changes needed by NNBD.

a14n commented 4 years ago

I hadn't thought about the engine that I'm not used to.

leafpetersen commented 4 years ago

engine changes if there's engine changes needed by NNBD.

I don't know that there are any functional changes required, but the APIs do need to be updated with nullability annotations where needed, and as I understand it web_ui has a fair bit of Dart code in it. I can file a separate issue for this.

Hixie commented 4 years ago

For the web_ui engine, @yjbanov or @ferhatb are probably the right contacts.

yjbanov commented 4 years ago

Have dart:html, dart:js, and dart:js_util been migrated? Other than that, there shouldn't be anything on the Web side that would prevent the migration (assuming all other dart: libraries are ready).

@leafpetersen Is it possible to migrate one part file at a time? We use them a lot (example).

leafpetersen commented 4 years ago

Have dart:html, dart:js, and dart:js_util been migrated?

@srujzs is working on dart:html. I think it's close (or maybe functionally complete but pending some optimization?) dart:js_util is done.

@leafpetersen Is it possible to migrate one part file at a time? We use them a lot (example).

No. The minimum granularity of the migration is a library.

yjbanov commented 4 years ago

@leafpetersen Is it possible to migrate one part file at a time? We use them a lot (example).

No. The minimum granularity of the migration is a library.

Ok, at least our tests (web_ui/tests) and dev tools (web_ui/dev, web_ui/tool) are separate libraries. It will still be a bit of a big bang migration. I suggest the following sequence:

Prototype the changes for web_ui/lib in a branch, show that it passes all tests. This can take as long as necessary.
Stop-the-world GC: everyone stops touching code in web_ui/lib and lets the null safety PR land (otherwise it could be a merge conflict nightmare). I'm assuming this part doesn't require more than a day.
Upgrade web_ui/tests, web_ui/dev, and web_ui/tool one library at a time. These are quite fine-grained.

Hixie commented 4 years ago

The recommended way to migrate is to add migration tool annotations bit by bit until the tool can migrate the entire library in one go automatically. This prevents the branch/merge nightmare without requiring a freeze.

cbracken commented 4 years ago

I'm more than happy to help out with dart:ui migration in the engine codebase. Most of the Dart code in the engine lives under the lib/ dir, but there are Dart test fixtures that live in a few places in the repo as well.

If you've got a doc that goes over migration steps and tooling, I'm happy to help co-ordinate.

srujzs commented 4 years ago

Have dart:html, dart:js, and dart:js_util been migrated?

@srujzs is working on dart:html. I think it's close (or maybe functionally complete but pending some optimization?) dart:js_util is done.

It's the latter, it's been migrated otherwise.

leafpetersen commented 4 years ago

The recommended way to migrate is to add migration tool annotations bit by bit until the tool can migrate the entire library in one go automatically.

If possible, I'd like to follow this workflow, because I think it will make it easier to keep the two API surfaces in sync. We'll have to validate the the tool works with dart: libraries - there are some oddities in dealing with those. cc @stereotype441

If you've got a doc that goes over migration steps and tooling, I'm happy to help co-ordinate.

Great, thanks. We have a doc, but I think it's out of date with the latest tooling changes. I'll get it up to date before we start this.

yjbanov commented 4 years ago

I'm happy to help on the web engine side.

The recommended way to migrate is to add migration tool annotations bit by bit until the tool can migrate the entire library in one go automatically.

Oh, I didn't know that's how the tool worked. That sounds great!

We'll have to validate the the tool works with dart: libraries

The web engine source is a plain Dart package. It is converted to dart:ui by a script. Would we still need dart: support in the migration tool?

yjbanov commented 4 years ago

Is there a link to documentation about the migration tool and annotations?

kevmoo commented 4 years ago

Is there a link to documentation about the migration tool and annotations?

@MichaelRFairhurst @stereotype441 @jcollins-g ?

jcollins-g commented 4 years ago

https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/edit/nnbd_migration/README.md

This is the most current information we have on how to run the migration tool. There are a lot of things not fully documented yet and the correct way to run it is still changing -- please reach out to us if you have questions or run into trouble.

I am trying to set up the flutter engine build locally so I can try the tool myself and reproduce any problems you might run into.

jcollins-g commented 4 years ago

@yjbanov

Looking at the engine, I think it will be a prerequisite to somehow graft in the NNBD version of the Dart SDK into the engine build (at the very least, the .dart code itself -- binaries aren't needed to try the tool). I am still learning your build process and have not yet found the right place to apply some duct tape.

cbracken commented 4 years ago

Looking at the engine, I think it will be a prerequisite to somehow graft in the NNBD version of the Dart SDK into the engine build (at the very least, the .dart code itself -- binaries aren't needed to try the tool). I am still learning your build process and have not yet found the right place to apply some duct tape.

@jcollins-g shout if you need help figuring this out -- happy to help in whatever way I can.

leafpetersen commented 4 years ago

I think @vsmenon might know how to get an unforked Dart SDK (at least for DDC) into a flutter engine build.

vsmenon commented 4 years ago

For DDC testing, I've been updating an unfork CL daily:

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

It's messy, but if you want to play with it:

(1) Have a current Flutter engine build.

(2) cd third_party/dart

(3) git cl patch 134562

(4) cd ../..

(5) ./flutter/tools/gn --unopt --full-dart-sdk && ninja -C out/host_debug_unopt

Note, to opt dart:ui back in, you'll also need to undo my CL here to remove the @dart tags:

https://github.com/flutter/engine/commit/0ef67b5e74c6a43915b72fc7ec682deed762206c

jcollins-g commented 4 years ago

OK. Thanks to @cbracken's help and @vsmenon 's patch, it's possible to run the migration tool on the dart:ui code and get something that looks reasonable. It's a mess on top of a mess, but the UI comes up and the suggestions don't seem outlandish at first glance.

(1) Do everything in @vsmenon's comment above (but, no need to manually opt-back-in dart:ui) (2) Have a current SDK checkout (you will need a version newer than the one in third_party). (3) Run the migration tool:

 ../out/host_debug_unopt/dart-sdk/bin/dart ~/dart/sdk/sdk/pkg/dartdev/bin/dartdev.dart \
    migrate lib/ui

Edit: per https://github.com/dart-lang/sdk/issues/40146#issuecomment-594102472, web UI no longer crashes so you don't need --no-web-preview.

vsmenon commented 4 years ago

Thanks, @jcollins-g - your instructions work for me on lib/ui. The web version of dart:ui is separate, in lib/web_ui, so I run that as well. Here's the result on both:

https://github.com/flutter/engine/compare/master...vsmenon:vsm-migrate

cbracken commented 4 years ago

Yep -- didn't update but worked for me as well, though I didn't run on web_ui :) Thanks!

Question to @vsmenon and @jcollins-g at what version of Dart are we safe to actually land these patches?

vsmenon commented 4 years ago

Once an unforked version of Dart rolls into Flutter, we should be able to start landing. At that point, dart:core, etc., will be opted-in, and only dart:ui would be opted out.

vsmenon commented 4 years ago

Tried building the migrated code. Blocked on https://github.com/dart-lang/sdk/issues/40834 for now.

jcollins-g commented 4 years ago

https://dart-review.googlesource.com/c/sdk/+/138047/2 has landed. This means that you can now remove --no-web-preview from the instructions, above, and use the web preview without it crashing right away while browsing.

jcollins-g commented 4 years ago

With @Hixie's help we've created #hackers-nnbd in the flutter discord. The rest of the NNBD migration tool developers will shortly be joining the server. @vsmenon @cbracken we'd welcome you to join us there.

vsmenon commented 4 years ago

The parse error has been fixed. Here's the current set of engine build errors after migration:

dart-ui-nnbd.txt

leafpetersen commented 4 years ago

I believe that dart:ui is now unblocked for migration.

cc @yjbanov @cbracken @kevmoo @vsmenon

vsmenon commented 4 years ago

@yjbanov @cbracken - if dart:ui files are ever interpreted as packages (for testing ?), we may need to whitelist those packages here:

https://github.com/dart-lang/sdk/issues/41538

franklinyow commented 4 years ago

@leafpetersen shell we close this?

leafpetersen commented 4 years ago

Done, thanks all!

dart-lang / sdk

Flutter framework null safety migration #40146