LibraryElement.featureSet has non-nullable enabled for libraries in unmigrated packages when experiment flag is on

[jcollins-g]

This is a problem at head and at analyzer 0.40.4.

If inside an analysis context you refer to a LibraryElement outside of a migrated package (which you presumably are using if you passed the experiment flag to analyzer), this LibraryElement will declare that it is non-nullable when it in fact is not. This makes dartdoc believe unmigrated packages are migrated resulting in dart-lang/dartdoc#2403.

This is particularly a problem for multi-package documentation sets such as Flutter which may have a mix of migrated and unmigrated packages in their documentation sets.

[franklinyow]

@leafpetersen @mit-mit
Is this a Beta blocker?

[mit-mit]

Yes; we need the documentation to be accurate wrt. null safety.

[devoncarew]

An update on this issue:

• @scheglov has a PR into dartdoc that we expect will fix this: Switch to AnalysisContextCollection. dartdoc#2309
• we do want the fix - correctly documenting libraries that have not opt-ed in - for the nnbd release
• the fix does not need to land in the sdk by the nnbd branch; it will land asynchronously from the sdk release cycle (Konstantin's PR lands; we verify the fix; Janice publishes a new package:dartdoc; that gets brought into the pub.dev/documentation release process)

[scheglov]

This test shows that we get Null Safety for one package, and legacy for another using AnalysisContextCollection.

@reflectiveTest
class NonNullableTest2 extends PubPackageResolutionTest {
  @override
  String get testPackageLanguageVersion => '2.11';

  solo_test_class_hierarchy() async {
    newFile('$workspaceRootPath/aaa/lib/a.dart', content: '');
    newFile('$workspaceRootPath/test/lib/a.dart', content: '');

    writeTestPackageConfig(
      PackageConfigFileBuilder()
        ..add(name: 'aaa', rootPath: '$workspaceRootPath/aaa'),
    );

    writeTestPackageAnalysisOptionsFile(
      AnalysisOptionsFileConfig(
        experiments: [EnableString.non_nullable],
      ),
    );

    var result = await resolveFile('$workspaceRootPath/test/lib/a.dart');
    print(
      '[path: ${result.path}]'
      '[isNullSafe: ${result.libraryElement.isNonNullableByDefault}]',
    );

    var result2 = await resolveFile('$workspaceRootPath/aaa/lib/a.dart');
    print(
      '[path: ${result2.path}]'
      '[isNullSafe: ${result2.libraryElement.isNonNullableByDefault}]',
    );
  }
}

Output:

[path: /home/test/lib/a.dart][isNullSafe: true]
[path: /home/aaa/lib/a.dart][isNullSafe: false]

[jcollins-g]

@scheglov is there a reason to believe this behavior is different between AnalysisDriver and AnalysisContextCollection?

[scheglov]

No, it should work similar with AnalysisDriver and AnalysisContextCollection (which is based on AnalysisDriver instances).

A slightly modified test to show specifying languageVersion to an external package, and using the same session (read AnalysisDriver) to resolve files of this external package.

Note that languageVersion: '2.9' is necessary, otherwise the default language version is the current language version (2.11 today), which will keep the non-nullable experiment enabled, because this experiment has 2.10 as experimentalReleaseVersion.

@reflectiveTest
class NonNullableTest2 extends PubPackageResolutionTest {
  @override
  String get testPackageLanguageVersion => '2.11';

  solo_test_class_hierarchy() async {
    newFile('/outer/aaa/lib/a.dart', content: '');
    newFile('$workspaceRootPath/test/lib/a.dart', content: '');

    writeTestPackageConfig(
      PackageConfigFileBuilder()
        ..add(
          name: 'aaa',
          rootPath: '/outer/aaa',
          languageVersion: '2.9',
        ),
    );

    writeTestPackageAnalysisOptionsFile(
      AnalysisOptionsFileConfig(
        experiments: [EnableString.non_nullable],
      ),
    );

    var result = await resolveFile('$workspaceRootPath/test/lib/a.dart');
    print(
      '[path: ${result.path}]'
      '[isNullSafe: ${result.libraryElement.isNonNullableByDefault}]',
    );

    var result2 = await result.session.getResolvedUnit('/outer/aaa/lib/a.dart');
    print(
      '[path: ${result2.path}]'
      '[isNullSafe: ${result2.libraryElement.isNonNullableByDefault}]',
    );
  }
}

Output:

[path: /home/test/lib/a.dart][isNullSafe: true]
[path: /outer/aaa/lib/a.dart][isNullSafe: false]

[jcollins-g]

Well, that is actually the entire issue. We don't know that it is languageVersion 2.9 despite the test package being 2.11, I am depending on the analyzer to know this.

[jcollins-g]

i will build a test case for you.

[scheglov]

You don't need to "know" that a package is 2.9, the package says the language version it is written in with pubspec.yaml. And when you run pub get, it will generate .dart_tool/package_config.json with the corresponding languageVersion for this package.

So far it seems to me that there are two solutions:

1. Use different analysis context, so different sets of experiments enabled for different packages.
2. Set the language version for legacy packages to 2.9 max, to make sure that these package are opted out from the non-nullable experiment.

[mit-mit]

Note: we don't need to solve this for experiments (whether a --enable-experiment flag was passed or not). But we must solve this for language versioning (whether a package is using a language version with null safety or not).

[jcollins-g]

@scheglov I am not sure I understand the meaning of your number 2.

I don't think number 1 is needed. A single analysis context should be able to know this for libraries in different packages. After building package_config.json we already know the language versions for every package. I think it would be reasonable to map that to available feature sets for libraries in those packages.

Maybe that is the same thing (or along the same lines) as your number 2?

[scheglov]

(1) is necessary if you enable an experiment, but your package_config.json for external packages does not specify language versions that opt you out from this experiment. Specifically, for non_nullable the language version must be 2.9 or earlier.

[jcollins-g]

@scheglov My package_config.json for dartdoc seems to do so. e.g.:

    {
      "name": "collection",
      "rootUri": "file:///Users/jcollins/.pub-cache/hosted/pub.dartlang.org/collection-1.14.13",
      "packageUri": "lib/",
      "languageVersion": "2.3"
    },
    {
      "name": "convert",
      "rootUri": "file:///Users/jcollins/.pub-cache/hosted/pub.dartlang.org/convert-2.1.1",
      "packageUri": "lib/",
      "languageVersion": "1.17"
    },

[scheglov]

Do you provide the packages argument when create AnalysisDriver instance?
That's what AnalysisContextCollection would do for you :-)

[jcollins-g]

@scheglov Well, sadly, that's a big fat no.

That would explain a great many things.

[jcollins-g]

Closing, assuming that the problem is we are initializing AnalysisDriver instances incorrectly. Will reopen if doing that right still results in the same problem (seems very unlikely).