Sdk: Add a hint if a package uses Future but does not import it from dart:async, and pubspec SDK constraint is too old

[stereotype441]

This would address a small, targeted subset of #34978 that we believe will be particularly troublesome for customers. Tasks to do:

• [x] Verify that the earliest version of the SDK in which dart:core began exporting Future was 2.1.0-dev.5.0.
• [x] Create a hint that fires if the user's code contains a reference to Future that would not have worked prior to the above SDK version, and the user's pubspec SDK constraint includes a lower-bound version of the SDK prior to 2.1.0-dev.5.0
• [x] If time allows, add logic to the analyzer to ensure that a change to the pubspec triggers re-analysis of the hint
• [x] If time allows, add quick fixes to allow the user to correct the hint by either:
  
  
  
  • [x] adding an import of 'dart:async'
  
  
  • [x] adjusting the pubspec as appropriate
  
  

Ideally, all these changes should be made in such a way that they can be cherry-picked to the dev branch of the SDK repo, so that they can be released as part of 2.1.1. However, if it's not feasible to do this for the "if time allows" items that's ok.

Yet to be determined (suggestions welcome):

• What to do if the pubspec does not contain an SDK constraint? Does pub have a standard interpretation of this situation that we could follow?
• What to do if no pubspec could be found?
• What to do if analysis is being performed in "build mode" (i.e. when run under bazel)? (Note that bazel doesn't allow the analyzer to access files that aren't explicitly specified as sources in the BUILD declaration, so simply looking for a pubspec file won't work)
• Do we need to address weird corner cases with re-exports (e.g. a.dart imports b.dart and uses Future; b.dart exports dart:async. Theoretically this is ok but it may not be worth trying to make the analyzer handle this case correctly)
• If the user opts for the quick fix that changes the pubspec, should we adjust the pubspec to require 2.1.0-dev.5.0, or simply 2.1.0?
• Pub accepts a wide variety of formats for version constraints. Does the quick fix need to be able to handle all of them?

[stereotype441]

@bwilkerson or @scheglov, can one of you look into this?

[bwilkerson]

Did you intend to include @pq?

What to do if no pubspec could be found?

If there's no pubspec, then I think analyzer should assume that the package is intended to work with the version of the SDK from which the analyzer is being run (that is, the latest, which means don't produce any hints).

What to do if analysis is being performed in "build mode" (i.e. when run under bazel)?

We don't use pubspec files in Bazel, so I think it should default to the behavior described above.

Do we need to address weird corner cases with re-exports ...

I think we should. I think analyzer should create a hint if Future is imported from 'core', which means that it isn't imported from any other library. I don't think it's that much harder to check.

Pub accepts a wide variety of formats for version constraints. Does the quick fix need to be able to handle all of them?

It needs to only make changes if it understands the format and can do so correctly. We might be able to look at a few well known packages on Github and see what formats are currently being used. It's possible that there's not that many being used for the SDK constraint.

[stereotype441]

Confirmed that the earliest version of the SDK in which dart:core began exporting Future was 2.1.0-dev.5.0.

[stereotype441]

Did you intend to include @pq?

What to do if no pubspec could be found?

If there's no pubspec, then I think analyzer should assume that the package is intended to work with the version of the SDK from which the analyzer is being run (that is, the latest, which means don't produce any hints).

What to do if analysis is being performed in "build mode" (i.e. when run under bazel)?

We don't use pubspec files in Bazel, so I think it should default to the behavior described above.

Do we need to address weird corner cases with re-exports ...

I think we should. I think analyzer should create a hint if Future is imported from 'core', which means that it isn't imported from any other library. I don't think it's that much harder to check.

Pub accepts a wide variety of formats for version constraints. Does the quick fix need to be able to handle all of them?

It needs to only make changes if it understands the format and can do so correctly. We might be able to look at a few well known packages on Github and see what formats are currently being used. It's possible that there's not that many being used for the SDK constraint.

SGTM

[pq]

the user's pubspec SDK constraint includes a lower-bound version of the SDK prior to 2.1.0-dev.5.0

@bwilkerson : are you thinking of enhancing the existing PubspecVisitor for this? Regardless it should be updated to make environment details available to pub-related lints... Needless to say, happy to help with that.

[bwilkerson]

are you thinking of enhancing the existing PubspecVisitor for this?

No, I wasn't. I don't think we need to visit the whole file to extract the sdk constraints. We should probably parse it as YAML to account for all the valid syntaxes, but we just need to extract the one value.

Ideally we'd piggy-back on top of the work that @srawlins is doing to better support a notion of packages. The constraints are specified by the package, and this would allow us to do the right thing for packages that don't use a pubspec file. Unfortunately, I don't know that we can wait that long before implementing this feature.

[stereotype441]

Ideally we'd piggy-back on top of the work that @srawlins is doing to better support a notion of packages. The constraints are specified by the package, and this would allow us to do the right thing for packages that don't use a pubspec file. Unfortunately, I don't know that we can wait that long before implementing this feature.

Yeah, since we want to cherry-pick the fix onto 2.1.1, it can't depend on @srawlins' work. I think it's ok if the bug gets fixed in a somewhat kludgy way for now, and then as part of #34978 we can clean it up to take advantage of Sam's work.

[bwilkerson]

Hint creation implemented by:
https://dart-review.googlesource.com/c/sdk/+/84501
https://dart-review.googlesource.com/c/sdk/+/84547

[stereotype441]

I did some experiments and found that cherry-picking these CLs to the dev branch leads to test failures, due to the fact that they relied on code that was in the analyzer branch but not yet landed on master. So @bwilkerson is going to try cherry-picking them to master and fixing up the resulting failures.

[stereotype441]

There is now a fix for the first two checkboxes on master that can be cherry-picked. I've submitted a cherry-pick request here: #35232

[stereotype441]

Remaining work is P2

[bwilkerson]

The quick fixes are implemented by https://dart-review.googlesource.com/c/sdk/+/84691.

[Hixie]

Isn't this kind of thing happening all the time? I mean, we keep adding features to Dart, right? Do we have a lint for each one to ask that the SDK constraint be moved up?

[natebosch]

Isn't this kind of thing happening all the time? I mean, we keep adding features to Dart, right? Do we have a lint for each one to ask that the SDK constraint be moved up?

This situation was somewhat unique in the wider opportunity for misuse.

It's true that there are always new features going in and using them requires an SDK constraint. This feature looks a little different from most:

• It's a usage by omission, you don't see a new API called in your code.
• Future and Stream are very widespread. Adding a new constructor to StreamTransformer is seen by a tiny fraction of code compared to using Future and Stream.
• It's triggered by common behavior of developers. It's typical to add a reference to Future and only add the import to dart:async when the the analyzer complains.

That said, I think even other changes are things we'd like to help catch, this was the most critical one.