Consider depend_on_referenced_packages for core lints inclusion

[jakemac53]

This is a new lint which hasn't shipped in the sdk yet but will in the next lint release.

What it does is ensure that you have a dependency (or dev dependency) in your pubspec if you import a package. There should be no valid situation where you import a package and don't depend on it in your pubspec, and there should be no false positives for this lint.

Not depending on a package but importing it means you are relying on it being available as a transitive dep which is risky (it could disappear at any time). It also means that you have no dependency constraint on it and could be broken by the package if it has a breaking change.

Note that pub lish already warns about this and may negatively score already, so we would want to avoid a duplicate negative score - @jonasfj might know more about the existing behavior.

[jakemac53]

cc @natebosch @munificent @pq @mit-mit @lrhn

[mit-mit]

sgtm

@goderbauer wdyt?

[lrhn]

LGTM.
Do we have a "don't depend on unreferenced packages" lint too, to avoid dependencies that you don't need?
Or to ensure you don't have a dependency on something which should just be a dev-dependency?

[jakemac53]

Do we have a "don't depend on unreferenced packages" lint too, to avoid dependencies that you don't need?

We don't, I would be happy to write it but I am not sure exactly how to in the current linting framework - you need to collect all the referenced deps by visiting all the dart files and then lint on the pubspec. cc @pq

Note however that I would not suggest adding this particular lint to the core set, maybe not even recommended, because there are valid reasons to have a dep on a package that you do not import in Dart files. You may be depending on some other assets it exposes (fonts, images, etc). Or in the case of the build_config package you are depending on the format of the build.yaml file.

[bwilkerson]

No, we don't. We can see when an import is referencing a package that isn't listed in the dependencies by examining two files. To catch the case where a dependency isn't referenced we'd need to analyze all of the files in the package. Neither the analyzer nor the linter are architected to make that efficient.

[devoncarew]

In terms of the schedule for adding this to package:lints, I would delay shipping it in a non-dev version of the package for a little while. It was just authored, and it would be good to shake out the long tail of any issues.

[jakemac53]

In terms of the schedule for adding this to package:lints, I would delay shipping it in a non-dev version of the package for a little while. It was just authored, and it would be good to shake out the long tail of any issues.

There is a bit of chicken/egg situation in that it probably won't actually get much adoption (and thus validation) until it is included in some default (non-prerelease) lint set. I definitely agree with the sentiment, but I am not sure what the best way is to actually get the most validation before shipping it.

We could certainly validate it on our own repos, and that might be enough? I have tested it on a few of the larger ones (build & test), and some packages in the SDK.

[devoncarew]

I think we can get good feedback before this is added to lints. That might look like:

• rolling the version of package:linter w/ the impl. into the SDK
• shipping a new beta SDK release
• emailing flutter announce about the lint / tweeting
• seeing if we get any issues reported

I think a lot of people will be interested in this lint - I'm sure we'll get some traction in a beta release. We could also validate through google3 for some of the lints.

[devoncarew]

For a lint which had been implemented for a while, I wouldn't have concerns about just adding it to the set of recommended lints. For a newly authored lint though, I think we want to find a few more click stops before turning it on for everyone.

[jakemac53]

• emailing flutter announce about the lint / tweeting

Ya that is a good idea, I am not on social media really myself so I would have to rely on others for this.

I think a lot of people will be interested in this lint - I'm sure we'll get some traction in a beta release. We could also validate through google3 for some of the lints.

Unfortunately this lint doesn't apply to google3, but in general ya that is I think a good strategy.

For a lint which had been implemented for a while, I wouldn't have concerns about just adding it to the set of recommended lints. For a newly authored lint though, I think we want to find a few more click stops before turning it on for everyone.

Ya I definitely agree, not pushing back on the idea. What I want is just to figure out how to actually get it in the hands of people so we can get that validation we want :).

[goderbauer]

@goderbauer wdyt?

I think this lint would be a good addition.

I also agree with @devoncarew that we should wait a little until the lint has matured a little. We should also enable it for the flutter repositories (flutter/flutter, flutter/engine, flutter/packages, flutter/plugins) and verify that it doesn't cause any issues there.

[jakemac53]

This should likely be blocked on fixing dart-lang/sdk#58425

[mit-mit]

Approved assuming we fix dart-lang/sdk#58425 first

[goderbauer]

@pq Would it be possible to add an auto-fix for that? (i.e. should this issue be labeled "should have a dart fix"?)

[pq]

We should investigate. I'm not 100% sure if we have enough information to generate a sensible version constraint (though I guess we could default to any with a TODO to tighten?)

/fyi @bwilkerson

[jakemac53]

Not sure if you can run pub commands, but you could just do pub add <package-name>. That just adds a ^ constraint on the latest version afaik, but its fine for most use cases and its the "safe" thing to do.

[jonasfj]

Calling dart pub add would be ideal, as it will create ^a.b.c constraint on the latest version compatible with your SDK and other dependencies. This reduces risk that you get a conflict.

But last I talked to @bwilkerson about it, we would likely have to give dart pub add a dry-run mode with machine readable output, so that the diff can be sent over the LSP.

But maybe just adding ^a.b.c for the latest version is fine.

[jakemac53]

latest version compatible with your SDK and other dependencies.

Oh nice I didn't know that was this smart 👍

[pq]

So we do call pub apis for completions (see PubPackageService) and the results it provides should be consistent with pub outdated which I think ensures version compatibility?

[jonasfj]

So we do call pub apis for completions (see PubPackageService) and the results it provides should be consistent with pub outdated which I think ensures version compatibility?

pub outdated won't have versions for packages not added yet :D

But I could be convinced to add a --dry-run --json options to dart pub add.

[srawlins]

If this lint is reported, but errors are not present, then the package is available as a transitive dep. This should mean it is available in PubPackageService.cachedPackages, yes? So using this info we could add depName: ^version.already.in.use to the pubspec.

We could also just add depName: any.

We could also wait for dart pub add --dry-run --json.

[jonasfj]

If there is a desire for dart pub add --dry-run --json, I think we'd be happy to add it, filed: dart-lang/pub#3333