-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
null-safety annotations on non-migrated packages #2403
Comments
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. |
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. |
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. |
@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) |
@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. |
Is it commonly possible to stumble out of a null-safe package into something that isn't null-safe? On
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. |
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. |
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. |
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. |
Reproduction steps:
Notice that the
pubspec.yaml
forpackage:retry
has:And
.dart_tool/package_config.json
specifies language version 2.0 forpackage:retry
:Unexpected result:
While it is correct that the
retry
function cannot returnnull
, the package is not migrated to null-safety, so should we really be adding any sort of null-safety tag here?The text was updated successfully, but these errors were encountered: