Analyzer doesn't error on legacy libraries

[mit-mit]

Repro steps with current main:

$ dart create testapp
$ cd testapp
$ nano lib/testapp.dart

Add // @dart=2.9 in that file

$ dart analyze .
Analyzing ....                         1.2s

   info • bin/app2.dart:1:8 • The library 'package:app2/app2.dart' is legacy, and shouldn't be imported into a null safe library. Try
          migrating the imported library. • import_of_legacy_library_into_null_safe
   info • test/app2_test.dart:1:8 • The library 'package:app2/app2.dart' is legacy, and shouldn't be imported into a null safe
          library. Try migrating the imported library. • import_of_legacy_library_into_null_safe

2 issues found.

Expected result: an error
Actual result: two infos

[bwilkerson]

I am about to go on vacation, so I wanted to provide a status update.

I have a CL that will cause an error to be produced, but it turns out to be fairly breaking.

• There are several places in pkg/compiler that are using soon-to-be invalid language override comments.
• There are a small number of other places in the sdk repo that are also using such comments.
• There is at least one place in Flutter where the new diagnostic is produced:
  Soon-to-be invalid language override comment flutter/flutter#117476
• There are a lot of co19 test failures.

I am working to resolve the issues. Advice for how to handle the co19 failures is welcome.

[mit-mit]

I've started creating some creating issues for those failures, and have added links in the comment above.

[mit-mit]

One more issue: Don't we want to remove the import_of_legacy_library_into_null_safe info message? It seems noisy and doesn't really add anything if I understand things correctly?

Analyzing ....                         0.6s

  error • lib/testapp.dart:1:1 • The language version must be >=2.12.0. Try removing the language version override and migrating the
          code. • illegal_language_version_override
   info • bin/testapp.dart:1:8 • The library 'package:testapp/testapp.dart' is legacy, and shouldn't be imported into a null safe
          library. Try migrating the imported library. • import_of_legacy_library_into_null_safe

[mit-mit]

I wonder if we might need temporary support for downgrading this to a warning when --no-sound-null-safety is passed. WDYT @leafpetersen @a-siva @jacob314 ? We'd need to add that support again before 3.0 stable, but it would buy some time.

That's what we had proposed in the original tracking issue #49925

The CFE already supports it:

$ dart compile kernel bin/testapp.dart
Compiling bin/testapp.dart to kernel file bin/testapp.dill.
Info: Compiling with sound null safety.
lib/testapp.dart:1:1: Error: Library doesn't support null safety.
// @dart=2.9
^^^^^^^^^^^^

$ dart compile kernel --no-sound-null-safety bin/testapp.dart
Compiling bin/testapp.dart to kernel file bin/testapp.dill.
Info: Compiling without sound null safety!
Dart 3 will only support sound null safety, see https://dart.dev/null-safety

[mit-mit]

It looks like we might be able to temporarily disable these just using ignores?

// ignore: illegal_language_version_override
// @dart=2.9

[bwilkerson]

I've started creating some creating issues for those failures, and have added links in the comment above.

Thanks. I'd already created one for Flutter and have talked to @sigmundch about the pkg/compiler issues.

Don't we want to remove the import_of_legacy_library_into_null_safe info message?

Yes. I wanted to address the breaking changes in one CL and other changes in a follow-on CL.

I wonder if we might need temporary support for downgrading this to a warning when --no-sound-null-safety is passed.

The analysis server (and hence dart analyze and dart fix) doesn't support that flag. For internal support we created a top-level variable whose value internally is going to be hard coded to mimic the behavior specified by that flag.

It looks like we might be able to temporarily disable these just using ignores?

Yes, and I did that for the set of diagnostics that prevented the SDK from being built, but then I found many more.

An alternative that Phil and I came up with is to change dartanalyze (the deprecated tool that we're still using on the bots) so that it hard-codes the value of the aforementioned top-level variable to behave as if the flag had been passed in. We could also add the flag to dartanalyze if that would be better. Anyone using an IDE to edit the pkg/compiler source would need to ignore the illegal_language_version_override diagnostics, but otherwise should be able to work as normal, at least for a while.

[leafpetersen]

Advice for how to handle the co19 failures is welcome.

These failures can be approved, just file an issue (maybe assign to me or @eernstg ) for us to figure out how to deal with them.

For internal support we created a top-level variable whose value internally is going to be hard coded to mimic the behavior specified by that flag.

Is the support in place to patch this on the roll? Otherwise this CL will break our internal roll, right?

An alternative that Phil and I came up with is to change dartanalyze (the deprecated tool that we're still using on the bots) so that it hard-codes the value of the aforementioned top-level variable to behave as if the flag had been passed in.

This seems like a reasonable alternative.

[itsjustkevin]

@bwilkerson did we come to approach to this issue that you feel comfortable with?

[bwilkerson]

I think so. I tried to run the bots this morning to see where we were on this issue, but they failed with a "patch failure" (what ever that means), so I'll need to try again later.

[itsjustkevin]

Thanks @bwilkerson. @whesse do you know anything about the "patch failure" error?

[mit-mit]

@bwilkerson I think the problem is that there is a merge conflict, so you need to rebase the CL to get the tests to run?

[whesse]

Patch failure error is something different, that just @bwilkerson and others are working on.

Patch support for internal rolls refers to our tip-of-tree builds of Dart, engine, and framework together, called monorepo (and the legacy is called HHH). This is running on both our Dart CI and Golem, and will break when a change to the SDK breaks engine and needs a manual roll with changes to engine. There was a legacy system to register these engine patches, but it is not working well any more.

The patch support on monorepo is not there - only deps changes get patched into tip-of-tree monorepo builds.
I would try and make a non-breaking fix-forward on flutter engine, and if that isn't possible, just prepare a manual roll of the Dart SDK change into engine ahead of time, and land it as soon as you can after the SDK change lands.

The patch support of the old system may be working slightly, but I wouldn't rely on it. I would suggest getting a tip-of-tree monorepo checkout, and testing the SDK and engine CL and PR together on it.

[whesse]

OK, the patch failure on the bots is just about a rebase being required, unless it is down in the guts of the SDK build where library patches are applied to the SDK core libraries.

[itsjustkevin]

Thank you as always @whesse and @mit-mit. Hope this helps unblock you @bwilkerson!

[bwilkerson]

I have a CL that passes all the checks (https://dart-review.googlesource.com/c/sdk/+/276800). I need a couple more reviews, so I added some reviewers that are still within their work hours.

@mit-mit If the added reviewers don't have a chance to review the CL before the end of the day here, and if there are folks in AAR that could look at it, then we could potentially get it landed sooner.

[bwilkerson]

The CL has landed. As long as it doesn't need to be reverted, we should be able to close this issue.

[mit-mit]

Confirmed working in a local build:

mit-macbookpro6:oldapp mit$ ~/dev/dartsdk/sdk/xcodebuild/ReleaseARM64/dart-sdk/bin/dart analyze .
Analyzing ....                         0.5s

  error • lib/old.dart:1:1 • The language version must be >=2.12.0. Try removing the language version override and
          migrating the code. • illegal_language_version_override