gopls/internal/lsp/source: remove 2nd last call to CachedImportPaths
The Snapshot.CachedImportPaths method is called twice, both from
completions. This change eliminates one use by using Metadata instead;
and reduces the method (and renames it to CachedPackages) by
moving most of the logic into the last caller, which will be
removed in a (much more difficult) follow-up.
Change-Id: I58e14c2be9f77a0a39ab553d42eb790f77af5b7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/471637
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index d0885f0..f93247a 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -1092,7 +1092,7 @@
return meta, nil
}
-func (s *snapshot) CachedImportPaths(ctx context.Context) (map[PackagePath]*types.Package, error) {
+func (s *snapshot) CachedPackages(ctx context.Context) []source.Package {
// Don't reload workspace package metadata.
// This function is meant to only return currently cached information.
s.AwaitInitialized(ctx)
@@ -1100,42 +1100,15 @@
s.mu.Lock()
defer s.mu.Unlock()
- pkgs := make(map[PackagePath]*syntaxPackage)
-
- // Find all cached packages that are imported a nonzero amount of time.
- //
- // TODO(rfindley): this is pre-existing behavior, and a test fails if we
- // don't do the importCount filter, but why do we care if a package is
- // imported a nonzero amount of times?
- imported := make(map[PackagePath]bool)
+ var pkgs []source.Package
s.packages.Range(func(_, v interface{}) {
ph := v.(*packageHandle)
- for dep := range ph.m.DepsByPkgPath {
- imported[dep] = true
- }
- if ph.m.Name == "main" {
- return
- }
pkg, err := ph.cached()
- if err != nil {
- return
- }
- if old, ok := pkgs[ph.m.PkgPath]; ok {
- if len(pkg.compiledGoFiles) < len(old.compiledGoFiles) {
- pkgs[ph.m.PkgPath] = pkg
- }
- } else {
- pkgs[ph.m.PkgPath] = pkg
+ if err == nil {
+ pkgs = append(pkgs, &Package{ph.m, pkg})
}
})
- results := make(map[PackagePath]*types.Package)
- for pkgPath, pkg := range pkgs {
- if imported[pkgPath] {
- results[pkgPath] = pkg.types
- }
- }
-
- return results, nil
+ return pkgs
}
// TODO(rfindley): clarify that this is only active modules. Or update to just
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index b78699f..e43ebc9 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -359,6 +359,7 @@
// candidate represents a completion candidate.
type candidate struct {
// obj is the types.Object to complete to.
+ // TODO(adonovan): eliminate dependence on go/types throughout this struct.
obj types.Object
// score is used to rank candidates.
@@ -1137,9 +1138,37 @@
func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error {
// Try loaded packages first. They're relevant, fast, and fully typed.
- known, err := c.snapshot.CachedImportPaths(ctx)
- if err != nil {
- return err
+ //
+ // Find all cached packages that are imported a nonzero amount of time.
+ //
+ // TODO(rfindley): this is pre-existing behavior, and a test fails if we
+ // don't do the importCount filter, but why do we care if a package is
+ // imported a nonzero amount of times?
+ //
+ // TODO(adonovan): avoid the need for type-checked packges entirely here.
+ pkgs := make(map[source.PackagePath]source.Package)
+ imported := make(map[source.PackagePath]bool)
+ for _, pkg := range c.snapshot.CachedPackages(ctx) {
+ m := pkg.Metadata()
+ for dep := range m.DepsByPkgPath {
+ imported[dep] = true
+ }
+ if m.Name == "main" {
+ continue
+ }
+ if old, ok := pkgs[m.PkgPath]; ok {
+ if len(m.CompiledGoFiles) < len(old.Metadata().CompiledGoFiles) {
+ pkgs[m.PkgPath] = pkg
+ }
+ } else {
+ pkgs[m.PkgPath] = pkg
+ }
+ }
+ known := make(map[source.PackagePath]*types.Package)
+ for pkgPath, pkg := range pkgs {
+ if imported[pkgPath] {
+ known[pkgPath] = pkg.GetTypes()
+ }
}
var paths []string
@@ -1481,21 +1510,28 @@
count := 0
- // TODO(adonovan): strength-reduce to a metadata query.
- // All that's needed below is Package.{Name,Path}.
- // Presumably that can be answered more thoroughly more quickly.
- known, err := c.snapshot.CachedImportPaths(ctx)
+ // Search packages across the entire workspace.
+ all, err := c.snapshot.AllMetadata(ctx)
if err != nil {
return err
}
+ pkgNameByPath := make(map[source.PackagePath]string)
var paths []string // actually PackagePaths
- for path, pkg := range known {
- if !strings.HasPrefix(pkg.Name(), prefix) {
- continue
+ for _, m := range all {
+ if m.ForTest != "" {
+ continue // skip all test variants
}
- paths = append(paths, string(path))
+ if m.Name == "main" {
+ continue // main is non-importable
+ }
+ if !strings.HasPrefix(string(m.Name), prefix) {
+ continue // not a match
+ }
+ paths = append(paths, string(m.PkgPath))
+ pkgNameByPath[m.PkgPath] = string(m.Name)
}
+ // Rank candidates using goimports' algorithm.
var relevances map[string]float64
if len(paths) != 0 {
if err := c.snapshot.RunProcessEnvFunc(ctx, func(opts *imports.Options) error {
@@ -1506,7 +1542,6 @@
return err
}
}
-
sort.Slice(paths, func(i, j int) bool {
if relevances[paths[i]] != relevances[paths[j]] {
return relevances[paths[i]] > relevances[paths[j]]
@@ -1518,22 +1553,22 @@
})
for _, path := range paths {
- pkg := known[source.PackagePath(path)]
- if _, ok := seen[pkg.Name()]; ok {
+ name := pkgNameByPath[source.PackagePath(path)]
+ if _, ok := seen[name]; ok {
continue
}
imp := &importInfo{
importPath: path,
}
- if imports.ImportPathToAssumedName(path) != pkg.Name() {
- imp.name = pkg.Name()
+ if imports.ImportPathToAssumedName(path) != name {
+ imp.name = name
}
if count >= maxUnimportedPackageNames {
return nil
}
c.deepState.enqueue(candidate{
// Pass an empty *types.Package to disable deep completions.
- obj: types.NewPkgName(0, nil, pkg.Name(), types.NewPackage(path, string(pkg.Name()))),
+ obj: types.NewPkgName(0, nil, name, types.NewPackage(path, name)),
score: unimportedScore(relevances[path]),
imp: imp,
})
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 1bfca02..7623b60 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -156,13 +156,13 @@
// excluding id itself.
ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*Metadata, error)
- // CachedImportPaths returns all the imported packages loaded in this
- // snapshot, indexed by their package path (not import path, despite the name)
- // and checked in TypecheckWorkspace mode.
+ // CachedPackages returns a new, unordered array of all
+ // packages currently cached in this snapshot, which is a
+ // poorly defined set that depends on the history of
+ // operations up to this point. Do not use it.
//
- // To reduce latency, it does not wait for type-checking to complete.
- // It is intended for use only in completions.
- CachedImportPaths(ctx context.Context) (map[PackagePath]*types.Package, error)
+ // TODO(adonovan): get rid of the last call from completions.
+ CachedPackages(ctx context.Context) []Package
// ActiveMetadata returns a new, unordered slice containing
// metadata for all packages considered 'active' in the workspace.
