null-safety annotations on non-migrated packages

[jonasfj]

Reproduction steps:

# Extract package:retry version 3.0.1
$ curl -L https://storage.googleapis.com/pub-packages/packages/retry-3.0.1.tar.gz | tar -xz
$ pub get

# Get dartdoc from git
$ mkdir /source/dartdoc
$ git clone https://github.com/dart-lang/dartdoc.git /source/dartdoc
$ cd /source/dartdoc; pub get; cd -;

# Generate docs
$ dart /source/dartdoc/bin/dartdoc.dart 

Notice that the pubspec.yaml for package:retry has:

environment:
  sdk: ">=2.0.0 <3.0.0"

And .dart_tool/package_config.json specifies language version 2.0 for package:retry:

{
  "configVersion": 2,
  "packages": [
    /* various other dependencies, mostly test and it's transitive dependencies, omitted here */
    {
      "name": "retry",
      "rootUri": "../",
      "packageUri": "lib/",
      "languageVersion": "2.0" // <-- language version 2.0, hence, no library in package:retry supports null-safety
    }
  ],
  "generated": "2020-10-21T10:44:27.912220Z",
  "generator": "pub",
  "generatorVersion": "2.10.0"
}

---

Unexpected result:

While it is correct that the retry function cannot return null, the package is not migrated to null-safety, so should we really be adding any sort of null-safety tag here?

[jonasfj]

Note. it's is correct that individual libraries can opt-out of null-safety, and thus, it can be argued that it's valuable to annotate whether a function or class supports null-safety.

But for most packages that are migrated to null-safety, all libraries will be migrated to null-safety.

Edit: I'm not entirely sure what we should be doing here. Just that adding "null-safety" to libraries in packages that do not have language-version >= 2.12.0 is probably wrong.

[jcollins-g]

Agree that this is probably wrong. Not sure what's happening here but will try to look into it tomorrow; if it is widespread (i.e. we're now marking EVERYTHING like this for some reason) should probably elevate to P1.

[jcollins-g]

I know that the analyzer still knows the truth that these aren't null-safe packages, however now we are sprinkling null safety tags all over the place.

[mit-mit]

@jcollins-g do we ever want this tag at the member level? Elsewhere, e.g. on pub.dev, we're going to treat null safety as a package-level property (either the whole package has it, or it doesn't)

[jcollins-g]

@mit-mit I don't have a strong opinion on it either way. There's an argument to be made that when browsing the corpus of API docs and crossing package boundaries, it might be nice to know when you've stumbled into (or out of) a null safe package, and a member tag is one way to do that.

[mit-mit]

We could put it here?

[jonasfj]

There's an argument to be made that when browsing the corpus of API docs and crossing package boundaries, it might be nice to know when you've stumbled into (or out of) a null safe package, and a member tag is one way to do that.

Is it commonly possible to stumble out of a null-safe package into something that isn't null-safe?

On pub.dev the analysis in pana will only classify a package as supports null-safety, if:

• Language version is >=2.12,
• No library in the package opts-out of null-safety using a language version marker (// @dart=2.5),
• All dependencies (excluding dev_dependencies) in the current pub upgrade resolution supports null-safety (following this recursive definition).

Hence, if you start reading documentation for a package that supports null-safety, it's unlikely that you accidentally stumble into a package/library that isn't null-safe.

But you can go the other way, if you start from a package that isn't null-safe. And if you end up on a documentation page by finding it in a google search I suppose it's also nice to know. So a library-level label could be nice.

[jcollins-g]

This turns out to be a hard problem to solve due to dart-lang/sdk#43903 which I understand to also be a hard problem to solve due to the experiment flag and how it is used for non-nullable. This issue may "go away" when the flag becomes default. My inclination is to allow the tags to be wrong for a short period, assuming the flag becomes default-on in the analyzer soon, as working around this will take significant effort. Will discuss with the analyzer team Monday, most of them are out today.

[jcollins-g]

Some bad news. Defaulting the flag to on won't actually solve the problem. So we need the extra work involved to correct this issue. The good news is that @scheglov already did the heavy lifting in the analyzer and dartdoc to solve this problem in #2309, though it needs to be updated and have some windows compatibility problems fixed. Once that work lands it should be possible to fix this bug.

Working now to start a PR based on #2309 with fixes for the issues he ran into and resolving merge conflicts.

[jcollins-g]

The attempt to use an updated version of #2309 to fix this issue ran into trouble. Short version is that dartdoc allows some things requiring non-default resolvers and so some of our tests fail. This is probably solvable but not in the timeframe.

My second attempt to "fix" it just reimplemented the pubspec reading logic inside dartdoc. This was silly, but would have worked. @scheglov pointed out that this could be caused by not passing packages into the AnalysisDriver and indeed, we had failed to do this.

#2411 corrects the issue by passing the correct package structure to AnalysisDriver, resulting in a fix.

Filed bug #2410 to deal with the port to AnalysisContextCollection as a failure to address that debt will no doubt result in more confusing problems like this in the future.

Filed bug #2409 to implement @mit-mit's comment about changing the positioning of the null safety tag.

Will close and move to publish a new version after #2411 lands.