Import auto-complete should not suggest non-dependencies

[matanlurey]

In this particular case I only depend on package:test, which in turn I guess transitively depends on these?

name: example
version: 0.0.0
environment:
  sdk: ">=2.10.0 <3.0.0"

dev_dependencies:
  test: ^1.15.0

It seems wrong to suggest them at all.

If it would be too breaking to change this behavior might I suggest something in analysis_options.yaml?:

analyzer:
  ide:
    strict-import-completion: true

[matanlurey]

(An added 👍 if this feature hinted if packages were imported that were not dependencies)

[bwilkerson]

That sounds reasonable to me. At the very least completion ought to be ranking suggestions for direct dependencies higher than suggestions for implicit dependencies. But I definitely agree that we don't want to encourage the practice of importing from packages that are only implicitly depended on.

[modulovalue]

It would be much more pragmatic to have dependencies themselves tell the analyzer which dependencies they want to expose with for example a transitive entry in their pubspec:

name: package_a

# private_dependency is excluded from search results by default in all packages that depend on package_a
private_dependency: some_version
# public_dependency is included in search results in packages that depend on package_a
public_dependency: some_version
  transitive: true  

It is difficult to track down which dependencies must be explicitly depended on if a user wants to be able to use any given package without having to unnecessarily make transitive dependencies explicit. The deeper the deepest path in the package dependency graph, the worse this issue becomes.

Example:

Think of the following package dependency graph

(A > B means package B depends on package A)

vector_math > geometry > business_logic > ui_base > macos_application

1. A user writes tests in macos_application and needs to create an instance of a type X.
2. User has to track down where type X was declared.
3. If X is an interface he needs to look for implementations of X (e.g. XImpl)
4. Once he has found XImpl he needs to add the package where XImpl was declared to the pubspec.

instead, if all packages that the user might need would be marked as transitive and therefore included in autocompletion search results (directives as well as type autocompletion), he'd only need to type XIm... and press enter to get what he needs.

TLDR: Excluding transitive dependencies from search results without the ability to opt-out/in per package is the wrong approach.

[srawlins]

@modulovalue Sorry to miss this comment for almost a year. This suggestion has a lot of features and benefits to it, but I think improved auto-completions would be near the smallest of them. If you're interested in doing so, I recommend filing a new issue, likely in this repo, to add this to the pubspec format, which would lead to changes in pub.

[srawlins]

Unfortunately, the package_config.json file today does not indicate which packages are direct dependencies, and which are indirect. We could look at pubspec.yaml to tell, but that seems hacky; could lead to more bugs when pubspec.yaml and package_config.json get out of sync. Perhaps not a big ask to change the package_config.json format to indicate if a package is a direct dependency of "this" package.

[shinayser]

As @DanTup mentioned on another issue, I would like to add another interesting point to this question:

In dart 2.17 we had the implementation of code completion and quickfix working for extension functions. But, it does not work for transient packages as I mentioned here #38894 (comment)

Maybe, as @modulovalue speced, this implementation could also serve as for the code completion machine to also look for exported-transient packages

[matanlurey]

@srawlins:

Unfortunately, the package_config.json file today does not indicate which packages are direct dependencies, and which are indirect. We could look at pubspec.yaml to tell, but that seems hacky; could lead to more bugs when pubspec.yaml and package_config.json get out of sync. Perhaps not a big ask to change the package_config.json format to indicate if a package is a direct dependency of "this" package.

Sorry to bump this (October 2020) issue, but so I understand correctly - package_config.json is what is used to tell the analyzer what packages are available, but the problem is there is no such state for direct versus indirect dependencies?

Would it be possible to use pubspec.yaml here? Or is there a hard requirement that auto-complete suggestions of imports must strictly only know about package_config.json?

(If it's the latter, either by decree or effect - i.e. too complicated otherwise, should I file an issue on the language repo to ask for augmenting the package_config.json schema?)

[DanTup]

Would it be possible to use pubspec.yaml here? Or is there a hard requirement that auto-complete suggestions of imports must strictly only know about package_config.json?

I think code completion is using pubspec.yaml in some places now - for example NotImportedContributor uses FileStateFilter to only show unimported suggestions only from direct dependencies. Perhaps UriContributor could use some variation of this (it'll probably work as-is to filter the files, but the current contributor also suggests "package:foo" which probably isn't possible from just a file filter).

@bwilkerson how much do you expect contributors to change with the refactors you're planning? Would changes in this area be better left until afterwards?

[bwilkerson]

... package_config.json is what is used to tell the analyzer what packages are available, but the problem is there is no such state for direct versus indirect dependencies?

Correct. The package_config.json file is where the mapping lives that tells the analyzer which version of the package was chosen by pub and where on disk to find that version.

Not only does the package_config.json not tell the analyzer whether a given dependency is direct or indirect, it also doesn't tell it whether the dependency is normal or dev (so we can't easily tell where a use of the dependency is allowed to occur.

Would it be possible to use pubspec.yaml here?

As Danny noted, I believe that we are using the pubspec.yaml file in some places, but not everywhere.

Sam's concern, which I think is valid, is that if the user has edited the pubspec.yaml file but hasn't yet run pub, then the two sources of information might be out of sync, which could lead to some confusing behavior. But we need to weigh the potential confusion of that situation against the potential confusion of not taking the additional information into account when offering completions, fixes, etc. So ...

Or is there a hard requirement that auto-complete suggestions of imports must strictly only know about package_config.json?

No, there's not hard requirement. We just need to consider all of the above when deciding what would make for the best UX.

My suggestion is that we should start with the packages known through the package_config.json file, and classify those packages based on the content of the pubspec.yaml file, with a sensible default for packages in the first that aren't mentioned in the second (and ignoring packages found only in the second).

... how much do you expect contributors to change with the refactors you're planning? Would changes in this area be better left until afterwards?

Most of the code in the existing contributors will be rewritten to some extent, but the NotImportedContributor and UriContributor are likely to remain fairly stable (by comparison to some of the rest) because they're not particularly dependent on the portions of the framework that are changing.

If this is something that someone wanted to address relatively soon I wouldn't be concerned about wasted effort or merge conflicts.