gopls/internal/lsp/cache: split metadata and loading in PackagesForFile
This change factors PackagesForFile{,s} so that they query the
metadata, perform filtering, then load the remaining packages,
rather than load all the packages before filtering.
This is another small step towards operating on metadata
instead of fully type-checked packages where possible.
Change-Id: I4c67c93df8012a8a7233703c7fac04991502dc62
Reviewed-on: https://go-review.googlesource.com/c/tools/+/455615
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 9650655..28d6fff 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -637,10 +637,100 @@
func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]source.Package, error) {
ctx = event.Label(ctx, tag.URI.Of(uri))
- phs, err := s.packageHandlesForFile(ctx, uri, mode, includeTestVariants)
+ metas, err := s.containingPackages(ctx, uri)
if err != nil {
return nil, err
}
+
+ var ids []PackageID
+ for _, m := range metas {
+ // Filter out any intermediate test variants.
+ // We typically aren't interested in these
+ // packages for file= style queries.
+ if m.IsIntermediateTestVariant() && !includeTestVariants {
+ continue
+ }
+ ids = append(ids, m.ID)
+ }
+
+ return s.loadPackages(ctx, mode, ids...)
+}
+
+func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, pkgPolicy source.PackageFilter) (source.Package, error) {
+ ctx = event.Label(ctx, tag.URI.Of(uri))
+ metas, err := s.containingPackages(ctx, uri)
+ if err != nil {
+ return nil, err
+ }
+ if len(metas) == 0 {
+ return nil, fmt.Errorf("no package metadata for file %s", uri)
+ }
+ switch pkgPolicy {
+ case source.NarrowestPackage:
+ metas = metas[:1]
+ case source.WidestPackage:
+ metas = metas[len(metas)-1:]
+ }
+ pkgs, err := s.loadPackages(ctx, mode, metas[0].ID)
+ if err != nil {
+ return nil, err
+ }
+ return pkgs[0], err
+}
+
+// containingPackages returns a new slice of metadata records for
+// packages that contain the Go file specified by uri. The results are
+// ordered "narrowest" to "widest".
+func (s *snapshot) containingPackages(ctx context.Context, uri span.URI) ([]*source.Metadata, error) {
+ // TODO(rfindley): why can't/shouldn't we awaitLoaded here? It seems that if
+ // the caller of this function is going to
+ // ask for package handles for a file, we should wait for pending loads.
+ // Else we will reload orphaned files before the initial load completes.
+
+ // Check if we should reload metadata for the file. We don't invalidate IDs
+ // (though we should), so the IDs will be a better source of truth than the
+ // metadata. If there are no IDs for the file, then we should also reload.
+ //
+ // TODO(adonovan): I can't relate the GetFile call to the
+ // previous comment, but tests rely on its effects.
+ // It does more than provide fh to the FileKind check. But what?
+ fh, err := s.GetFile(ctx, uri)
+ if err != nil {
+ return nil, err
+ }
+
+ // Check that the file kind is Go (not, say, C or assembly).
+ // (This check is perhaps superfluous since the go/packages "file="
+ // metadata query in MetadataForFile requires .go files files.)
+ if kind := s.view.FileKind(fh); kind != source.Go {
+ return nil, fmt.Errorf("no packages for non-Go file %s (%v)", uri, kind)
+ }
+
+ metas, err := s.MetadataForFile(ctx, uri) // ordered narrowest to widest
+ if err != nil {
+ return nil, err
+ }
+ return metas, err
+}
+
+// loadPackages type-checks the specified packages in the given mode.
+func (s *snapshot) loadPackages(ctx context.Context, mode source.TypecheckMode, ids ...PackageID) ([]source.Package, error) {
+ // Build all the handles...
+ var phs []*packageHandle
+ for _, id := range ids {
+ parseMode := source.ParseFull
+ if mode == source.TypecheckWorkspace {
+ parseMode = s.workspaceParseMode(id)
+ }
+
+ ph, err := s.buildPackageHandle(ctx, id, parseMode)
+ if err != nil {
+ return nil, err
+ }
+ phs = append(phs, ph)
+ }
+
+ // ...then await them all.
var pkgs []source.Package
for _, ph := range phs {
pkg, err := ph.await(ctx, s)
@@ -652,84 +742,6 @@
return pkgs, nil
}
-func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, pkgPolicy source.PackageFilter) (source.Package, error) {
- ctx = event.Label(ctx, tag.URI.Of(uri))
-
- // TODO(adonovan): opt: apply pkgPolicy to the list of
- // Metadatas before initiating loading of any package.
- phs, err := s.packageHandlesForFile(ctx, uri, mode, false)
- if err != nil {
- return nil, err
- }
-
- if len(phs) < 1 {
- return nil, fmt.Errorf("no packages")
- }
-
- ph := phs[0]
- for _, handle := range phs[1:] {
- switch pkgPolicy {
- case source.WidestPackage:
- if ph == nil || len(handle.CompiledGoFiles()) > len(ph.CompiledGoFiles()) {
- ph = handle
- }
- case source.NarrowestPackage:
- if ph == nil || len(handle.CompiledGoFiles()) < len(ph.CompiledGoFiles()) {
- ph = handle
- }
- }
- }
- if ph == nil {
- return nil, fmt.Errorf("no packages in input")
- }
-
- return ph.await(ctx, s)
-}
-
-func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, withIntermediateTestVariants bool) ([]*packageHandle, error) {
- // TODO(rfindley): why can't/shouldn't we awaitLoaded here? It seems that if
- // we ask for package handles for a file, we should wait for pending loads.
- // Else we will reload orphaned files before the initial load completes.
-
- // Check if we should reload metadata for the file. We don't invalidate IDs
- // (though we should), so the IDs will be a better source of truth than the
- // metadata. If there are no IDs for the file, then we should also reload.
- fh, err := s.GetFile(ctx, uri)
- if err != nil {
- return nil, err
- }
- if kind := s.view.FileKind(fh); kind != source.Go {
- return nil, fmt.Errorf("no packages for non-Go file %s (%v)", uri, kind)
- }
- metas, err := s.MetadataForFile(ctx, uri)
- if err != nil {
- return nil, err
- }
-
- var phs []*packageHandle
- for _, m := range metas {
- // Filter out any intermediate test variants. We typically aren't
- // interested in these packages for file= style queries.
- if m.IsIntermediateTestVariant() && !withIntermediateTestVariants {
- continue
- }
- parseMode := source.ParseFull
- if mode == source.TypecheckWorkspace {
- parseMode = s.workspaceParseMode(m.ID)
- }
-
- ph, err := s.buildPackageHandle(ctx, m.ID, parseMode)
- if err != nil {
- return nil, err
- }
- phs = append(phs, ph)
- }
- return phs, nil
-}
-
-// MetadataForFile returns the Metadata for each package associated
-// with the file uri. If no such package exists or if they are known
-// to be stale, it reloads metadata for the file.
func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source.Metadata, error) {
s.mu.Lock()
@@ -796,6 +808,12 @@
if !unloadable && len(ids) == 0 {
s.unloadableFiles[uri] = struct{}{}
}
+
+ // Sort packages "narrowed" to "widest" (in practice: non-tests before tests).
+ sort.Slice(metas, func(i, j int) bool {
+ return len(metas[i].CompiledGoFiles) < len(metas[j].CompiledGoFiles)
+ })
+
return metas, nil
}
