[tools] gopls/internal/lsp/source: eliminate Snapshot.Package{,s}ForFile

[Alan Donovan (Gerrit)]

Alan Donovan submitted this change.

View Change

1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: gopls/internal/lsp/source/view.go
Insertions: 6, Deletions: 6.

@@ -224,10 +224,10 @@
 }
 
 // PackageForFile returns a single package that this file belongs to,
-// checked in mode and filtered by the package policy.
+// checked in mode and filtered by the package selector.
 //
 // TODO(adonovan): merge with GetTypedFile.
-func PackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI, mode TypecheckMode, policy PackageFilter) (Package, error) {
+func PackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI, mode TypecheckMode, pkgSel PackageSelector) (Package, error) {
 	metas, err := snapshot.MetadataForFile(ctx, uri)
 	if err != nil {
 		return nil, err
@@ -235,7 +235,7 @@
 	if len(metas) == 0 {
 		return nil, fmt.Errorf("no package metadata for file %s", uri)
 	}
-	switch policy {
+	switch pkgSel {
 	case NarrowestPackage:
 		metas = metas[:1]
 	case WidestPackage:
@@ -248,16 +248,16 @@
 	return pkgs[0], err
 }
 
-// PackageFilter sets how a package is filtered out from a set of packages
+// PackageSelector sets how a package is selected out from a set of packages
 // containing a given file.
-type PackageFilter int
+type PackageSelector int
 
 const (
 	// NarrowestPackage picks the "narrowest" package for a given file.
 	// By "narrowest" package, we mean the package with the fewest number of
 	// files that includes the given file. This solves the problem of test
 	// variants, as the test will have more files than the non-test package.
-	NarrowestPackage PackageFilter = iota
+	NarrowestPackage PackageSelector = iota
 
 	// WidestPackage returns the Package containing the most files.
 	// This is useful for something like diagnostics, where we'd prefer to
```
```
The name of the file: gopls/internal/lsp/source/util.go
Insertions: 2, Deletions: 2.

@@ -90,8 +90,8 @@
 // metadata.
 //
 // TODO(adonovan): add a Mode parameter and merge with PackageForFile.
-func GetTypedFile(ctx context.Context, snapshot Snapshot, fh FileHandle, pkgPolicy PackageFilter) (Package, *ParsedGoFile, error) {
-	pkg, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, pkgPolicy)
+func GetTypedFile(ctx context.Context, snapshot Snapshot, fh FileHandle, pkgSel PackageSelector) (Package, *ParsedGoFile, error) {
+	pkg, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, pkgSel)
 	if err != nil {
 		return nil, nil, err
 	}
```

Approvals: Alan Donovan: Run TryBots Robert Findley: Looks good to me, approved kokoro: gopls CI succeeded Gopher Robot: TryBots succeeded

gopls/internal/lsp/source: eliminate Snapshot.Package{,s}ForFile

The PackagesForFile (plural) method was called only once, from
qualifiedObjsAtLocation, which now inlines it. (It looks like
further simplification may be possible; another day.)

The PackageForFile (singular) method is now expressed as a
standalone function in terms of Snapshot primitives.

commandHandler.runTests is downgraded from type-checking
to a metadata operation.

Change-Id: I1ae5b95860217b1823feebd0572f689b0b4a0618
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459778
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Alan Donovan <adon...@google.com>
gopls-CI: kokoro <noreply...@google.com>
Reviewed-by: Robert Findley <rfin...@google.com>
---
M gopls/internal/lsp/cache/graph.go
M gopls/internal/lsp/cache/snapshot.go
M gopls/internal/lsp/code_action.go
M gopls/internal/lsp/command.go
M gopls/internal/lsp/diagnostics.go
M gopls/internal/lsp/semantic.go
M gopls/internal/lsp/source/diagnostics.go
M gopls/internal/lsp/source/highlight.go
M gopls/internal/lsp/source/identifier.go
M gopls/internal/lsp/source/implementation.go
M gopls/internal/lsp/source/util.go
M gopls/internal/lsp/source/view.go
12 files changed, 80 insertions(+), 77 deletions(-)

diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go
index aaabbdf..8e9e5d9 100644
--- a/gopls/internal/lsp/cache/graph.go
+++ b/gopls/internal/lsp/cache/graph.go
@@ -91,7 +91,7 @@
 		//  - Else, choose the first valid command-line-argument package, if it exists.
 		//
 		// TODO(rfindley): it might be better to track all IDs here, and exclude
-		// them later in PackagesForFile, but this is the existing behavior.
+		// them later when type checking, but this is the existing behavior.
 		for i, id := range ids {
 			// If we've seen *anything* prior to command-line arguments package, take
 			// it. Note that ids[0] may itself be command-line-arguments.
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index dc34208..f5c8594 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -636,50 +636,6 @@
 	return overlays
 }
 
-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))
-
-	metas, err := s.MetadataForFile(ctx, uri)
-	if err != nil {
-		return nil, err
-	}
-
-	// Optionally filter out any intermediate test variants.
-	// We typically aren't interested in these
-	// packages for file= style queries.
-	if !includeTestVariants {
-		metas = source.RemoveIntermediateTestVariants(metas)
-	}
-
-	ids := make([]PackageID, len(metas))
-	for i, m := range metas {
-		ids[i] = m.ID
-	}
-	return s.TypeCheck(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.MetadataForFile(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.TypeCheck(ctx, mode, metas[0].ID)
-	if err != nil {
-		return nil, err
-	}
-	return pkgs[0], err
-}
-
 // TypeCheck type-checks the specified packages in the given mode.
 func (s *snapshot) TypeCheck(ctx context.Context, mode source.TypecheckMode, ids ...PackageID) ([]source.Package, error) {
 	// Build all the handles...
diff --git a/gopls/internal/lsp/code_action.go b/gopls/internal/lsp/code_action.go
index 3b333ff..2c509e3 100644
--- a/gopls/internal/lsp/code_action.go
+++ b/gopls/internal/lsp/code_action.go
@@ -158,7 +158,7 @@
 
 		// Type-check the package and also run analysis,
 		// then combine their diagnostics.
-		pkg, err := snapshot.PackageForFile(ctx, fh.URI(), source.TypecheckFull, source.WidestPackage)
+		pkg, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.WidestPackage)
 		if err != nil {
 			return nil, err
 		}
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index aba9f02..2022325 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -432,15 +432,15 @@
 
 func (c *commandHandler) runTests(ctx context.Context, snapshot source.Snapshot, work *progress.WorkDone, uri protocol.DocumentURI, tests, benchmarks []string) error {
 	// TODO: fix the error reporting when this runs async.
-	// TODO(adonovan): opt: request only metadata, not type checking.
-	pkgs, err := snapshot.PackagesForFile(ctx, uri.SpanURI(), source.TypecheckWorkspace, false)
+	metas, err := snapshot.MetadataForFile(ctx, uri.SpanURI())
 	if err != nil {
 		return err
 	}
-	if len(pkgs) == 0 {
+	metas = source.RemoveIntermediateTestVariants(metas)
+	if len(metas) == 0 {
 		return fmt.Errorf("package could not be found for file: %s", uri.SpanURI().Filename())
 	}
-	pkgPath := pkgs[0].ForTest()
+	pkgPath := string(metas[0].ForTest)
 
 	// create output
 	buf := &bytes.Buffer{}
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index 6ac0ee6..863c142 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -195,8 +195,8 @@
 		if snapshot.FindFile(uri) == nil {
 			continue
 		}
-		// Don't call PackagesForFile for builtin.go, as it results in a
-		// command-line-arguments load.
+
+		// Don't request type-checking for builtin.go: it's not a real package.
 		if snapshot.IsBuiltin(ctx, uri) {
 			continue
 		}
@@ -204,7 +204,7 @@
 		// Find all packages that include this file and diagnose them in parallel.
 		metas, err := snapshot.MetadataForFile(ctx, uri)
 		if err != nil {
-			// TODO (findleyr): we should probably do something with the error here,
+			// TODO(findleyr): we should probably do something with the error here,
 			// but as of now this can fail repeatedly if load fails, so can be too
 			// noisy to log (and we'll handle things later in the slow pass).
 			continue
diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go
index 59d4fc3..5bc2f20 100644
--- a/gopls/internal/lsp/semantic.go
+++ b/gopls/internal/lsp/semantic.go
@@ -92,7 +92,7 @@
 	if kind != source.Go {
 		return nil, nil
 	}
-	pkg, err := snapshot.PackageForFile(ctx, fh.URI(), source.TypecheckFull, source.WidestPackage)
+	pkg, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.WidestPackage)
 	if err != nil {
 		return nil, err
 	}
diff --git a/gopls/internal/lsp/source/diagnostics.go b/gopls/internal/lsp/source/diagnostics.go
index 8024f46..dc9cc04 100644
--- a/gopls/internal/lsp/source/diagnostics.go
+++ b/gopls/internal/lsp/source/diagnostics.go
@@ -77,7 +77,7 @@
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	pkg, err := snapshot.PackageForFile(ctx, uri, TypecheckFull, NarrowestPackage)
+	pkg, err := PackageForFile(ctx, snapshot, uri, TypecheckFull, NarrowestPackage)
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
diff --git a/gopls/internal/lsp/source/highlight.go b/gopls/internal/lsp/source/highlight.go
index 2f0766b..ee5d9ed 100644
--- a/gopls/internal/lsp/source/highlight.go
+++ b/gopls/internal/lsp/source/highlight.go
@@ -24,7 +24,7 @@
 	// Don't use GetTypedFile because it uses TypecheckWorkspace, and we
 	// always want fully parsed files for highlight, regardless of whether
 	// the file belongs to a workspace package.
-	pkg, err := snapshot.PackageForFile(ctx, fh.URI(), TypecheckFull, WidestPackage)
+	pkg, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, WidestPackage)
 	if err != nil {
 		return nil, fmt.Errorf("getting package for Highlight: %w", err)
 	}
diff --git a/gopls/internal/lsp/source/identifier.go b/gopls/internal/lsp/source/identifier.go
index 64167af..739b511 100644
--- a/gopls/internal/lsp/source/identifier.go
+++ b/gopls/internal/lsp/source/identifier.go
@@ -80,13 +80,13 @@
 	ctx, done := event.Start(ctx, "source.Identifier")
 	defer done()
 
-	pkg, err := snapshot.PackageForFile(ctx, fh.URI(), TypecheckFull, NarrowestPackage)
+	pkg, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
 	if err != nil {
 		return nil, err
 	}
 	pgf, err := pkg.File(fh.URI())
 	if err != nil {
-		// We shouldn't get a package from PackagesForFile that doesn't actually
+		// We shouldn't get a package from PackageForFile that doesn't actually
 		// contain the file.
 		bug.Report("missing package file", bug.Data{"pkg": pkg.ID(), "file": fh.URI()})
 		return nil, err
diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go
index d3de373..460a9f7 100644
--- a/gopls/internal/lsp/source/implementation.go
+++ b/gopls/internal/lsp/source/implementation.go
@@ -283,8 +283,15 @@
 	// consider: the definition of the object referenced by the location. But we
 	// try to be comprehensive in case we ever support variations on build
 	// constraints.
-
-	pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckWorkspace, true)
+	metas, err := s.MetadataForFile(ctx, key.uri)
+	if err != nil {
+		return nil, err
+	}
+	ids := make([]PackageID, len(metas))
+	for i, m := range metas {
+		ids[i] = m.ID
+	}
+	pkgs, err := s.TypeCheck(ctx, TypecheckWorkspace, ids...)
 	if err != nil {
 		return nil, err
 	}
@@ -302,7 +309,7 @@
 		}
 	}
 	if !hasFullPackage {
-		pkg, err := s.PackageForFile(ctx, key.uri, TypecheckFull, WidestPackage)
+		pkg, err := PackageForFile(ctx, s, key.uri, TypecheckFull, WidestPackage)
 		if err != nil {
 			return nil, err
 		}
diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go
index 301e017..cfdfd97 100644
--- a/gopls/internal/lsp/source/util.go
+++ b/gopls/internal/lsp/source/util.go
@@ -88,8 +88,10 @@
 // Type-checking is expensive. Call snapshot.ParseGo if all you need
 // is a parse tree, or snapshot.MetadataForFile if all you only need
 // metadata.
-func GetTypedFile(ctx context.Context, snapshot Snapshot, fh FileHandle, pkgPolicy PackageFilter) (Package, *ParsedGoFile, error) {
-	pkg, err := snapshot.PackageForFile(ctx, fh.URI(), TypecheckWorkspace, pkgPolicy)
+//
+// TODO(adonovan): add a Mode parameter and merge with PackageForFile.
+func GetTypedFile(ctx context.Context, snapshot Snapshot, fh FileHandle, pkgSel PackageSelector) (Package, *ParsedGoFile, error) {
+	pkg, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, pkgSel)
 	if err != nil {
 		return nil, nil, err
 	}
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 0aeac54..aaf9329 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -164,17 +164,6 @@
 	// IsBuiltin reports whether uri is part of the builtin package.
 	IsBuiltin(ctx context.Context, uri span.URI) bool
 
-	// PackagesForFile returns an unordered list of packages that contain
-	// the file denoted by uri, type checked in the specified mode.
-	//
-	// If withIntermediateTestVariants is set, the resulting package set includes
-	// intermediate test variants.
-	PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode, withIntermediateTestVariants bool) ([]Package, error)
-
-	// PackageForFile returns a single package that this file belongs to,
-	// checked in mode and filtered by the package policy.
-	PackageForFile(ctx context.Context, uri span.URI, mode TypecheckMode, selectPackage PackageFilter) (Package, error)
-
 	// ReverseDependencies returns a new mapping whose entries are
 	// the ID and Metadata of each package in the workspace that
 	// directly or transitively depend on the package denoted by id,
@@ -234,16 +223,41 @@
 	return []label.Label{tag.Snapshot.Of(snapshot.SequenceID()), tag.Directory.Of(snapshot.View().Folder())}
 }
 
-// PackageFilter sets how a package is filtered out from a set of packages
+// PackageForFile returns a single package that this file belongs to,
+// checked in mode and filtered by the package selector.
+//
+// TODO(adonovan): merge with GetTypedFile.
+func PackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI, mode TypecheckMode, pkgSel PackageSelector) (Package, error) {
+	metas, err := snapshot.MetadataForFile(ctx, uri)
+	if err != nil {
+		return nil, err
+	}
+	if len(metas) == 0 {
+		return nil, fmt.Errorf("no package metadata for file %s", uri)
+	}
+	switch pkgSel {
+	case NarrowestPackage:
+		metas = metas[:1]
+	case WidestPackage:
+		metas = metas[len(metas)-1:]
+	}
+	pkgs, err := snapshot.TypeCheck(ctx, mode, metas[0].ID)
+	if err != nil {
+		return nil, err
+	}
+	return pkgs[0], err
+}
+
+// PackageSelector sets how a package is selected out from a set of packages
 // containing a given file.
-type PackageFilter int
+type PackageSelector int
 
 const (
 	// NarrowestPackage picks the "narrowest" package for a given file.
 	// By "narrowest" package, we mean the package with the fewest number of
 	// files that includes the given file. This solves the problem of test
 	// variants, as the test will have more files than the non-test package.
-	NarrowestPackage PackageFilter = iota
+	NarrowestPackage PackageSelector = iota
 
 	// WidestPackage returns the Package containing the most files.
 	// This is useful for something like diagnostics, where we'd prefer to

To view, visit change 459778. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I1ae5b95860217b1823feebd0572f689b0b4a0618

Gerrit-Change-Number: 459778

Gerrit-PatchSet: 4

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: kokoro <noreply...@google.com>

Gerrit-MessageType: merged