Linter: Lint for used package not defined in pubspec

[kevmoo]

[pq]

Silly question: but wouldn't this result in a compilation error?

[kevmoo]

If you specify X which depends on Y you can depend on Y.

But if X later removes its dependency on Y, you'll be borked.

[sethladd]

Are we supposed to list all transitive dependencies in our pubspec ?

[kevmoo]

@sethladd: absolutely not.

We should have an explicit dependency for libraries with explicitly use.

[sethladd]

ah, yes! gotcha :)

[kevmoo]

Is this doable? @pq @alexeieleusis @a14n ?

T'would make many folks very happy

[bwilkerson]

We can certainly compute the information you're interested in. When linting each compilation unit, look for explicit references to elements defined in packages that are not explicitly depended on. We might need to cache the list of explicit dependencies somewhere to improve performance.

What about implicit references? For example, given a.b.c, if a is defined in package A, and b returns a class from a package B, do you want to create a lint for the use of c if there is no explicit dependency on B?

[kevmoo]

@bwilkerson – let's keep things crazy simple

If

import 'package:a/a.dart';
// or
export 'package:a/a.dart';

And a is not a dependency defined in pubspec.yaml – dependencies or dev_dependencies – then generate a lint.

[kevmoo]

...if the import/export directive is in a file in lib then the dependency had better be in dependencies (not just dev_dependencies)

[zoechi]

Is there a way to exclude code from that? For example when I want to share code between test/ and example or as I had it in Flutter between test/ and test_driver/
I don't want to add dependencies used there to dependencies
It's not super important, but worth a thought.

[kevmoo]

I would assume this would only be for package: imports...

On Tue, Mar 6, 2018 at 1:32 PM, Günter Zöchbauer [e-mail]
wrote:

Is there a way to exclude code from that? For example when I want to share
code between test/ and example or as I had it in Flutter between test/
and test_driver/
I don't want to add dependencies used there to dependencies
It's not super important, but worth a thought.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/linter/issues/29#issuecomment-370935700,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABCisO2_wzByeZU5KpzpZIhQeK6ATQNks5tbwBWgaJpZM4Dgjlo
.

[kevmoo]

...although it might be good to discuss an overall "lints for directives"
approach.

For instance, folks should NOT use relative directives in the lib direction
to things outside the lib directory...

On Tue, Mar 6, 2018 at 1:36 PM, Kevin Moore [e-mail] wrote:

I would assume this would only be for package: imports...

On Tue, Mar 6, 2018 at 1:32 PM, Günter Zöchbauer <[email protected]

wrote:

Is there a way to exclude code from that? For example when I want to
share code between test/ and example or as I had it in Flutter between
test/ and test_driver/
I don't want to add dependencies used there to dependencies
It's not super important, but worth a thought.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/linter/issues/29#issuecomment-370935700,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABCisO2_wzByeZU5KpzpZIhQeK6ATQNks5tbwBWgaJpZM4Dgjlo
.

[zoechi]

so if I have import 'package:uuid/uuid.dart'; in lib/test/id_generator.dart
and I import that in test/foo_test.dart and test_driver/foo_integration_test.dart
using import 'package:my_package/test/id_generator.dart';
would this need to be a dependency, or would be dev_dependency be enough?

[bwilkerson]

Every library inside of lib can be referenced by any package that depends on your package via a package: URI. That means that someone else could import id_generator.dart in their production code. In order for that to not break, you'd need to include a non-dev dependency on uuid in your pubspec. If you don't want other packages to reference id_generator.dart, then you should move the file outside of lib.

[zoechi]

@bwilkerson but there is no valid way to import it from somewhere else, or is there?
Otherwise I would if course put it somewhere else.
Is import '../test_shared/id_generator.dart'; ok in test/foo_test.dart?

[bwilkerson]

but there is no valid way to import it from somewhere else, or is there?

If you publish a package foo, that contains a file at the path foo/lib/test/bar.dart, then any other package can add your package to their pubspec and can import 'package:foo/test/bar.dart';. Everything inside lib is potentially accessible, even src.

It's only by convention that other developers are expected to not reference code inside lib/src, and I've never seen any convention for code inside lib/test. Code that is outside of lib cannot be accessed via a package: URI.

Is import '../test_shared/id_generator.dart'; ok in test/foo_test.dart?

Yes. Relative paths like this can reach any file and will work correctly for anyone that clones your repository as long as the path doesn't include directories outside your repository. We strongly discourage using relative paths inside of lib that include directories outside of lib. (And I personally discourage using relative paths anywhere inside of lib because it minimizes the possibility of subtle bugs related to mixing import styles.)

[zoechi]

Yes. Relative paths like this can reach any file and will work correctly

It's only for importing from other top-level directories inside the same package like test, test_driver, example, ...
It's a bit ugly, but should do because it's not too common.

[DanTup]

This came up in Dart-Code too:

https://github.com/Dart-Code/Dart-Code/issues/1978

It was slightly confusing because the add-import quick-fix only considers things in your pubspec - so if you type @experimental, you won't see a quick-fix to import meta if it's not explicitly listed in your pubspec. However if you add the import manually (still without it being in the pubspec) then the annotation is fine to use. This makes it look like the quick-fix isn't working correctly.

[pq]

fyi @scheglov

Thanks for the added detail @DanTup!

[scheglov]

I think this works as intended.
Transitive dependencies are not your dependencies.
So neither completion, nor quick-fix suggest you symbols from transitive dependencies.

But your .packages allows you to resolving these package: URIs and symbols.
The hint here might be a good idea, to lower expectations :-)

[pq]

The hint here might be a good idea, to lower expectations :-)

Agreed!

Thanks for following up. 👍

[ditman]

We've recently received at least a bug report in Flutter web where the user was using a transitive dependency (which was web only) in their Flutter app, and was wondering why their application was not compiling on Android.

We're not sure how the "include" ended up in their code, but it might have been the user manually inputting it.

I think a hint on the import statement letting you know that you're using a package which you don't depend on directly would be great.

What needs to be done to write an efficient hint/lint? I have some experience with the AST visitors in @pq's tools; I expect something similar?

[pq]

Thanks for the added context @ditman. Very useful!

@bwilkerson: putting aside API implications for a moment, is this something we can derive from an imported library's Namespace?

[bwilkerson]

No, the namespace for an imported library contains all of the top-level names that are either defined in that library or are exported from other libraries by that library.

What we'll need to do is parse the pubspec.yaml file to find the immediate dependencies and then look for any package: URIs that reference a package that isn't in that list. In order to be efficient we'll want to cache the results of the parse so that we don't need to re-parse it for every file in a package.

While the import certainly might have been hand entered, it's also possible for it to have been added automatically by server. I can think of several possibilities, and there are probably several I'm not thinking of:

• code completing a symbol from a library in such a package,
• adding an override of a method defined in such a package, and
• using an assist like "add type annotation".

If we add this lint we should consider adding a quick fix that adds an explicit dependency to the pubspec.yaml file (though getting the version constraints correct might be tricky).

[ditman]

If we add this lint we should consider adding a quick fix that adds an explicit dependency to the pubspec.yaml file (though getting the version constraints correct might be tricky).

@bwilkerson If it were possible to know the version of the transient dependency, it'd probably be nice to set the version to that (or ^version).

[bwilkerson]

Agreed. We might be able to scrape the current version from the path in the .pub-cache (assuming that's where the package lives).

[pq]

@bwilkerson and I chatted and I see a path forward that would add support to LinterContext to query for dependencies. This would benefit at least avoid_web_libraries_in_flutter and make other analyses possible.

[ditman]

Please, do reach out to me if I can be of any help! Thanks for considering this!

[pq]

https://dart-review.googlesource.com/c/sdk/+/134861

(Adds a pubspec getter to PubWorkspacePackages.)

[pq]

Having thought about this some more, are we sure we want to limit this to pubspec dependencies?

That is, are we likely to want similar behavior for BazelWorkspacePackages or do we not have the same transitive closure issue there? (cc @srawlins, @davidmorgan)

/fyi @jacob314

[bwilkerson]

We do have the same issues, but they're handled by the build system. It would be better to have a development time diagnostic rather than a build time error, but at least it gets caught, unlike in the pub case.

[jakemac53]

I am curious what the status of this is - it is somewhat common to forget to add deps until you try to pub lish which has a warning for it. I would much prefer to catch this before that point, so an analyzer hint/lint would be great.

[pq]

This is one of I think many opportunities for improvement wrt pub and analysis.

/fyi @jonasfj

[jakemac53]

One idea for how this could be implemented would be to have pub add its own field to the package_config.json with this information (list of direct deps) - and then internally we could do the same based on the build rules. Although I think that wouldn't help the IDE experience still internally.

[FufferKS]

Hi, any update on this issue?

[pq]

Sorry, no real progress.

@FufferKS: if you have any other context or experience to share, it'd be welcome. Is this an issue that you hit frequently? How does it impact you?

Thanks for chiming in!

[FufferKS]

I'm running into this issue pretty much on a daily bases.

I'm working on a fairly large project that's separated into a number of packages. All of those depend on a "common" package with some core logic. The "common" package also depends on a number of public packages - bloc, etc. When working on any of the concrete packages, there is no hinting of e.q. bloc related classes.
This is a bit simplified, in reality the dependency tree of ours is even deeper.

This forces me to make imports by hand, which is quite cumbersome.

[pq]

This is useful, thanks! Is the project open-source? (No worries if not but if it is, it would be useful for us to look at the structure to better understand how this and related issues are impacting folks.)

[FufferKS]

Thanks for the answers :)

The project is not open source, here is an obscured project dependency tree. Each entry is a separate flutter project.

[pq]

Awesome. Thanks for this @FufferKS!

/fyi @jakemac53