Migrating analyzer_plugin to null safety #45236

incendial · 2021-03-07T20:19:56Z

Hi, are there any plans for migrating analyzer_plugin to null safety? Our package depends on it and we want to start migration too. I'd be happy to open a PR for analyzer_plugin null safety migration if it isn't already in the works.

devoncarew · 2021-03-08T17:23:21Z

Thanks for the inquiry; there aren't currently plans to migrate it to null safety (and no plans not to, it's just that getting package:analyzer null safe was a larger priority).

A contribution would be welcome; if you haven't contributed through the dart-lang/sdk process before, you'll want to take a look at https://github.com/dart-lang/sdk/blob/master/CONTRIBUTING.md.

scheglov · 2021-03-09T17:57:16Z

We are not working yet on migrating analyzed_plugin, but we would like to have it migrated. So, if you have time to do it, this is great. Send your PR :-)

As far as I can see, almost all dependencies of analyzer_plugin are migrated, and have null safe versions published. One exception is dart_style, which has dart-lang/dart_style#1000 waiting for now. I will ping it.

incendial · 2021-03-09T18:47:12Z

I have already started working on the migration (using dart_style from git for now). Thanks for pinging them!

Although all dependencies seems to have null safe version, I have a problem with resolving both analyzer package direct dependency and through analyzer_utilities path override (path: ../analyzer_utilities). Never faced that issue before. Are there some command to resolve those type of issues or I just simply missing something?

scheglov · 2021-03-09T19:16:51Z

I have not tried exactly this, but you might need to use dependency overrides while working on this.

incendial · 2021-03-09T19:32:47Z

Oh, will try, thanks!

incendial · 2021-03-16T19:25:23Z

So yeah, I was doing it wrong and there is no need to call pub get inside a package.

I have two main questions:

Some classes used from the analyzer has fields that marked as non-nullable, but have old comments that suggest that they might be. Is it intensional or a mistake?
If I understood it correctly, protocol is generated from the html spec. Right now I have migrated those files by hand just to make sure that all tests pass. Do I need to migrate the generating tool (it seems like a harder task for now)? Will it also require analyzer_server and analyzer_server_client migrations?

I actually don't know if it's the right place to ask questions. If it's not, do you have a place where I can ask them?

scheglov · 2021-03-16T19:33:24Z

Ask any question here.

If there are any fields that have non-nullable type, but the comment says that they are nullable, please tell us about these fields. This most probably means that I did not update the comment.

It is fine if the generating tool looks too complicated for you, I can do this portion a bit later.

incendial · 2021-03-16T20:02:49Z

Thanks for the quick response!

Three places so far: first, second, third.

As for generating tool - I will take a look at it once more, but if I decide to leave it, should I revert changes to generated files? And it seems that migration is kinda huge (like ~85 changed files or so). How can I help you to review it (I can split it to a commit per file or group several files to a commit)?

bwilkerson · 2021-03-16T20:04:27Z

Do I need to migrate the generating tool (it seems like a harder task for now)?

If you mean migrating the code in the generator, then we ought to be able to do that separately. If you mean update the generator to produce null safe code, that might be necessary, even if it's just to add a // @dart= comment. I know that for the analysis_server package we have a test to ensure that we never forget to run the generator, and I think we did the same for the analyzer_plugin package. If we did, and if you hand-edit the generated code, then the test will fail.

Will it also require analyzer_server and analyzer_server_client migrations?

No. Those packages depend on the analyzer_plugin package, not the other way around.

incendial · 2021-03-16T20:25:39Z

If you mean update the generator to produce null safe code

Yeah, I meant produced code. But won't a // @dart= comment actually still block external packages (that depend on the analyzer_plugin) from null safety migration?

No. Those packages depend on the analyzer_plugin package, not the other way around.

Oh, thats great.

scheglov · 2021-03-16T20:28:07Z

Thank you, I will update these comments.
https://dart-review.googlesource.com/c/sdk/+/191481

Whatever is easier for you - if you have already done migrating the generated code, keep it.
Then me, or someone else, can update the generator to produce this or similar migrated code.

The generator itself is not a production code, so it will not prevent tests or clients from being null safe.

bwilkerson · 2021-03-16T20:49:07Z

But won't a // @dart= comment actually still block external packages (that depends on the analyzer_plugin) from null safety migration?

It shouldn't prevent it. The semantics of writing null safe code that depends on un-migrated code are well defined, and it shouldn't be a requirement to migrate all dependent packages first, but it is strongly advised for two reasons:

waiting to migrate means that your code won't be broken by unforeseen changes to the packages you depend on when they're migrated, and
depending on un-migrated code means that the VM can't assume soundness when running your code, which leads to less efficient execution performance.

So I think it would be reasonable to update the generator to generate those comments and then let someone else update the generator and regenerate the code before we publish the package.

Then me, or someone else, can update the generator to produce this or similar migrated code.

@scheglov Won't pkg/analyzer_plugin/tool/spec/check_all_test.dart fail?

scheglov · 2021-03-16T21:39:05Z

Yes, the check for generator will fail, we would have to disable it for a short time while we are working on migrating the generator.

#45236 (comment) Change-Id: Ic9aa0b120cb0796ad1796c6f34aef8fc5360700a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191481 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

incendial · 2021-03-17T20:23:58Z

Thank you, I will update these comments.

Great, I will remove null checks for those cases then.

It shouldn't prevent it. The semantics of writing null safe code that depends on un-migrated code are well defined, and it shouldn't be a requirement to migrate all dependent packages first, but it is strongly advised for two reasons:

waiting to migrate means that your code won't be broken by unforeseen changes to the packages you depend on when they're migrated, and

depending on un-migrated code means that the VM can't assume soundness when running your code, which leads to less efficient execution performance.

Oh, thanks for clarifying that. Am I correct, that if we assume that users of the analyzer_plugin are usually tools and used as dev dependencies which means that second statement about VM will not affect package prod build, only dev?

Here is the PR link https://dart-review.googlesource.com/c/sdk/+/191622

incendial · 2021-03-17T20:34:57Z

Should I disable checks for generator by myself? And do I need to update package version in pubspec? And is it intentional that generate_files files are a bit different, like here for server and here for plugin?

incendial · 2021-03-26T15:25:01Z

Could you please help with the next step after getting LGTM for the CL?

Bug: #45236 Change-Id: I01175a2b2b1eb3ce6cdf30ade794d8186c6e8ead Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191622 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

scheglov · 2021-04-05T16:42:18Z

Null safe analyzer_plugin 0.5.0 was published.

incendial · 2021-04-06T06:57:34Z

Thank you very much for helping me with the CL!

bwilkerson · 2021-04-06T14:34:25Z

Thanks for doing all the work to migrate it!

incendial · 2021-04-08T07:15:20Z

Hi, it seems that there is a small problem with null safety migration.

And as I can see, it was already fixed in main branch. Do you have a release date for that fix?

If it's more appropriate, I can create a separate issue for that.

bwilkerson · 2021-04-08T14:26:43Z

@scheglov

scheglov · 2021-04-08T20:47:06Z

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

scheglov · 2021-04-09T00:47:19Z

Published and landed.

Change-Id: Ibffb027251f1a6033e80f09dc1cb130d34262c70 Bug: #45236 (comment) Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194542 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

incendial · 2021-04-09T13:17:33Z

Thank you! Works like a charm.

srawlins added analyzer-plugin area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Mar 8, 2021

incendial mentioned this issue Mar 8, 2021

Null safety migration dart-code-checker/dart-code-metrics#216

Closed

16 tasks

devoncarew added the P3 A lower priority bug or feature request label Mar 8, 2021

scheglov closed this as completed Apr 5, 2021

incendial mentioned this issue Apr 8, 2021

[Bug] Using code_metrics for a plugin that has an example app causes the Dart analysis server not to pickup on metrics analysis dart-code-checker/dart-code-metrics#253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating analyzer_plugin to null safety #45236

Migrating analyzer_plugin to null safety #45236

incendial commented Mar 7, 2021 •

edited

Loading

devoncarew commented Mar 8, 2021

scheglov commented Mar 9, 2021

incendial commented Mar 9, 2021 •

edited

Loading

scheglov commented Mar 9, 2021

incendial commented Mar 9, 2021

incendial commented Mar 16, 2021 •

edited

Loading

scheglov commented Mar 16, 2021

incendial commented Mar 16, 2021 •

edited

Loading

bwilkerson commented Mar 16, 2021

incendial commented Mar 16, 2021 •

edited

Loading

scheglov commented Mar 16, 2021

bwilkerson commented Mar 16, 2021

scheglov commented Mar 16, 2021

incendial commented Mar 17, 2021 •

edited

Loading

incendial commented Mar 17, 2021 •

edited

Loading

incendial commented Mar 26, 2021

scheglov commented Apr 5, 2021

incendial commented Apr 6, 2021

bwilkerson commented Apr 6, 2021

incendial commented Apr 8, 2021

bwilkerson commented Apr 8, 2021

scheglov commented Apr 8, 2021

scheglov commented Apr 9, 2021

incendial commented Apr 9, 2021

Migrating analyzer_plugin to null safety #45236

Migrating analyzer_plugin to null safety #45236

Comments

incendial commented Mar 7, 2021 • edited Loading

devoncarew commented Mar 8, 2021

scheglov commented Mar 9, 2021

incendial commented Mar 9, 2021 • edited Loading

scheglov commented Mar 9, 2021

incendial commented Mar 9, 2021

incendial commented Mar 16, 2021 • edited Loading

scheglov commented Mar 16, 2021

incendial commented Mar 16, 2021 • edited Loading

bwilkerson commented Mar 16, 2021

incendial commented Mar 16, 2021 • edited Loading

scheglov commented Mar 16, 2021

bwilkerson commented Mar 16, 2021

scheglov commented Mar 16, 2021

incendial commented Mar 17, 2021 • edited Loading

incendial commented Mar 17, 2021 • edited Loading

incendial commented Mar 26, 2021

scheglov commented Apr 5, 2021

incendial commented Apr 6, 2021

bwilkerson commented Apr 6, 2021

incendial commented Apr 8, 2021

bwilkerson commented Apr 8, 2021

scheglov commented Apr 8, 2021

scheglov commented Apr 9, 2021

incendial commented Apr 9, 2021

incendial commented Mar 7, 2021 •

edited

Loading

incendial commented Mar 9, 2021 •

edited

Loading

incendial commented Mar 16, 2021 •

edited

Loading

incendial commented Mar 16, 2021 •

edited

Loading

incendial commented Mar 16, 2021 •

edited

Loading

incendial commented Mar 17, 2021 •

edited

Loading

incendial commented Mar 17, 2021 •

edited

Loading