gopls/internal/lsp/source: filter intermediate test variants

This change ensures that intermediate test variant (ITV) packages
are filtered out before type checking in all calls to PackageForFile
or TypeCheck. There should never be any need to type check ITVs
in practice: they contain identical syntax to the regular package,
but differ in their import metadata. Technically this may affect
types of selectors (fields/methods) but we choose to ignore that
as it is bad programming and expensive to accommodate.

Details:
- MetadataForFile now sorts primarily by narrowest and
  secondarily by ITVs.
- source.NarrowestMetadataForFile is a convenience wrapper
  that selects the first non-ITV element.
- PackageForFile now always chooses the narrowest,
  and is renamed NarrowestPackageForFile.
  The sole use of widest, from renameOrdinary, was inlined.
  (One other was replaced by narrowest.)
- The PackageSelector enum (narrowes/widest) is gone.
- TODOs added to think about ITVs in the implicitly
  type checking methods.

Fixes golang/go#57795

Change-Id: Id1e95990bcbc830594d2d5acec7b4f93bc01a501
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487095
Reviewed-by: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>

gopls/internal/lsp/cache/snapshot.go

@@ -811,14 +811,24 @@ func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source
 		s.unloadableFiles[uri] = struct{}{}
 	}
 
-	// Sort packages "narrowest" to "widest" (in practice: non-tests before tests).
+	// Sort packages "narrowest" to "widest" (in practice:
+	// non-tests before tests), and regular packages before
+	// their intermediate test variants (which have the same
+	// files but different imports).
 	sort.Slice(metas, func(i, j int) bool {
-		return len(metas[i].CompiledGoFiles) < len(metas[j].CompiledGoFiles)
+		x, y := metas[i], metas[j]
+		xfiles, yfiles := len(x.CompiledGoFiles), len(y.CompiledGoFiles)
+		if xfiles != yfiles {
+			return xfiles < yfiles
+		}
+		return boolLess(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant())
 	})
 
 	return metas, nil
 }
 
+func boolLess(x, y bool) bool { return !x && y } // false < true
+
 func (s *snapshot) ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*source.Metadata, error) {
 	if err := s.awaitLoaded(ctx); err != nil {
 		return nil, err

gopls/internal/lsp/code_action.go

@@ -183,7 +183,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
 
 		// Type-check the package and also run analysis,
 		// then combine their diagnostics.
-		pkg, _, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+		pkg, _, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
 		if err != nil {
 			return nil, err
 		}

gopls/internal/lsp/command.go

@@ -438,15 +438,11 @@ func (c *commandHandler) RunTests(ctx context.Context, args command.RunTestsArgs
 
 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.
-	metas, err := snapshot.MetadataForFile(ctx, uri.SpanURI())
+	meta, err := source.NarrowestMetadataForFile(ctx, snapshot, uri.SpanURI())
 	if err != nil {
 		return err
 	}
-	metas = source.RemoveIntermediateTestVariants(metas)
-	if len(metas) == 0 {
-		return fmt.Errorf("package could not be found for file: %s", uri.SpanURI().Filename())
-	}
-	pkgPath := string(metas[0].ForTest)
+	pkgPath := string(meta.ForTest)
 
 	// create output
 	buf := &bytes.Buffer{}
@@ -704,17 +700,16 @@ func (c *commandHandler) ToggleGCDetails(ctx context.Context, args command.URIAr
 		progress:    "Toggling GC Details",
 		forURI:      args.URI,
 	}, func(ctx context.Context, deps commandDeps) error {
-		metas, err := deps.snapshot.MetadataForFile(ctx, deps.fh.URI())
+		meta, err := source.NarrowestMetadataForFile(ctx, deps.snapshot, deps.fh.URI())
 		if err != nil {
 			return err
 		}
-		id := metas[0].ID // 0 => narrowest package
 		c.s.gcOptimizationDetailsMu.Lock()
-		if _, ok := c.s.gcOptimizationDetails[id]; ok {
-			delete(c.s.gcOptimizationDetails, id)
+		if _, ok := c.s.gcOptimizationDetails[meta.ID]; ok {
+			delete(c.s.gcOptimizationDetails, meta.ID)
 			c.s.clearDiagnosticSource(gcDetailsSource)
 		} else {
-			c.s.gcOptimizationDetails[id] = struct{}{}
+			c.s.gcOptimizationDetails[meta.ID] = struct{}{}
 		}
 		c.s.gcOptimizationDetailsMu.Unlock()
 		c.s.diagnoseSnapshot(deps.snapshot, nil, false)
@@ -766,14 +761,11 @@ func (c *commandHandler) ListImports(ctx context.Context, args command.URIArg) (
 				})
 			}
 		}
-		metas, err := deps.snapshot.MetadataForFile(ctx, args.URI.SpanURI())
+		meta, err := source.NarrowestMetadataForFile(ctx, deps.snapshot, args.URI.SpanURI())
 		if err != nil {
 			return err // e.g. cancelled
 		}
-		if len(metas) == 0 {
-			return fmt.Errorf("no package containing %v", args.URI.SpanURI())
-		}
-		for pkgPath := range metas[0].DepsByPkgPath { // 0 => narrowest package
+		for pkgPath := range meta.DepsByPkgPath {
 			result.PackageImports = append(result.PackageImports,
 				command.PackageImport{Path: string(pkgPath)})
 		}

gopls/internal/lsp/diagnostics.go

@@ -245,10 +245,8 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps
 			// noisy to log (and we'll handle things later in the slow pass).
 			continue
 		}
+		source.RemoveIntermediateTestVariants(metas)
 		for _, m := range metas {
-			if m.IsIntermediateTestVariant() {
-				continue
-			}
 			toDiagnose[m.ID] = m
 		}
 	}
@@ -708,7 +706,7 @@ func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snaps
 
 	metas, _ := snapshot.MetadataForFile(ctx, fh.URI())
 	if len(metas) > 0 || ctx.Err() != nil {
-		return nil // no package, or cancelled
+		return nil // file has a package (or cancelled)
 	}
 	// Inv: file does not belong to a package we know about.
 	pgf, err := snapshot.ParseGo(ctx, fh, source.ParseHeader)

gopls/internal/lsp/link.go

@@ -119,8 +119,8 @@ func goLinks(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle
 		// This requires the import map from the package metadata. Ignore errors.
 		var depsByImpPath map[source.ImportPath]source.PackageID
 		if strings.ToLower(view.Options().LinkTarget) == "pkg.go.dev" {
-			if metas, _ := snapshot.MetadataForFile(ctx, fh.URI()); len(metas) > 0 {
-				depsByImpPath = metas[0].DepsByImpPath // 0 => narrowest package
+			if meta, err := source.NarrowestMetadataForFile(ctx, snapshot, fh.URI()); err == nil {
+				depsByImpPath = meta.DepsByImpPath
 			}
 		}
 

gopls/internal/lsp/semantic.go

@@ -91,7 +91,7 @@ func (s *Server) computeSemanticTokens(ctx context.Context, td protocol.TextDocu
 	if kind != source.Go {
 		return nil, nil
 	}
-	pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+	pkg, pgf, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, err
 	}

gopls/internal/lsp/source/call_hierarchy.go

@@ -27,7 +27,7 @@ func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle,
 	ctx, done := event.Start(ctx, "source.PrepareCallHierarchy")
 	defer done()
 
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, err
 	}
@@ -182,7 +182,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
 	ctx, done := event.Start(ctx, "source.OutgoingCalls")
 	defer done()
 
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, err
 	}
@@ -221,7 +221,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
 	}
 
 	// Use TypecheckFull as we want to inspect the body of the function declaration.
-	declPkg, declPGF, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+	declPkg, declPGF, err := NarrowestPackageForFile(ctx, snapshot, uri)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/lsp/source/code_lens.go

@@ -100,7 +100,7 @@ func TestsAndBenchmarks(ctx context.Context, snapshot Snapshot, fh FileHandle) (
 	if !strings.HasSuffix(fh.URI().Filename(), "_test.go") {
 		return out, nil
 	}
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return out, err
 	}

gopls/internal/lsp/source/completion/completion.go

@@ -446,7 +446,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan
 
 	startTime := time.Now()
 
-	pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+	pkg, pgf, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil || pgf.File.Package == token.NoPos {
 		// If we can't parse this file or find position for the package
 		// keyword, it may be missing a package declaration. Try offering

gopls/internal/lsp/source/definition.go

@@ -22,7 +22,7 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position
 	ctx, done := event.Start(ctx, "source.Definition")
 	defer done()
 
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, err
 	}

gopls/internal/lsp/source/diagnostics.go

@@ -60,7 +60,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon
 // as used by the "gopls check" command.
 //
 // TODO(adonovan): factor in common with (*Server).codeAction, which
-// executes { PackageForFile; Analyze } too?
+// executes { NarrowestPackageForFile; Analyze } too?
 //
 // TODO(adonovan): opt: this function is called in a loop from the
 // "gopls/diagnoseFiles" nonstandard request handler. It would be more
@@ -71,7 +71,7 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (File
 	if err != nil {
 		return nil, nil, err
 	}
-	pkg, _, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+	pkg, _, err := NarrowestPackageForFile(ctx, snapshot, uri)
 	if err != nil {
 		return nil, nil, err
 	}

gopls/internal/lsp/source/fix.go

@@ -55,7 +55,7 @@ var suggestedFixes = map[string]SuggestedFixFunc{
 // singleFile calls analyzers that expect inputs for a single file
 func singleFile(sf singleFileFixFunc) SuggestedFixFunc {
 	return func(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
-		pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+		pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 		if err != nil {
 			return nil, nil, err
 		}

gopls/internal/lsp/source/format.go

@@ -72,9 +72,9 @@ func Format(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]protocol.T
 		// Can this, for example, result in inconsistent formatting across saves,
 		// due to pending calls to packages.Load?
 		var langVersion, modulePath string
-		mds, err := snapshot.MetadataForFile(ctx, fh.URI())
-		if err == nil && len(mds) > 0 {
-			if mi := mds[0].Module; mi != nil {
+		meta, err := NarrowestMetadataForFile(ctx, snapshot, fh.URI())
+		if err == nil {
+			if mi := meta.Module; mi != nil {
 				langVersion = mi.GoVersion
 				modulePath = mi.Path
 			}

gopls/internal/lsp/source/highlight.go

@@ -23,7 +23,7 @@ func Highlight(ctx context.Context, snapshot Snapshot, fh FileHandle, position p
 
 	// We always want fully parsed files for highlight, regardless
 	// of whether the file belongs to a workspace package.
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, fmt.Errorf("getting package for Highlight: %w", err)
 	}

gopls/internal/lsp/source/hover.go

@@ -88,7 +88,7 @@ func Hover(ctx context.Context, snapshot Snapshot, fh FileHandle, position proto
 // hovering at the position, it returns _, nil, nil: an error is only returned
 // if the position is valid but we fail to compute hover information.
 func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Position) (protocol.Range, *HoverJSON, error) {
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return protocol.Range{}, nil, err
 	}

gopls/internal/lsp/source/implementation.go

@@ -88,6 +88,7 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p
 	if err != nil {
 		return nil, err
 	}
+	RemoveIntermediateTestVariants(declMetas)
 	if len(declMetas) == 0 {
 		return nil, fmt.Errorf("no packages for file %s", declURI)
 	}
@@ -170,6 +171,7 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p
 		}
 		globalIDs = append(globalIDs, m.ID)
 	}
+	// TODO(adonovan): filter out ITVs?
 	indexes, err := snapshot.MethodSets(ctx, globalIDs...)
 	if err != nil {
 		return nil, fmt.Errorf("querying method sets: %v", err)
@@ -244,7 +246,7 @@ func offsetToLocation(ctx context.Context, snapshot Snapshot, filename string, s
 func typeDeclPosition(ctx context.Context, snapshot Snapshot, uri span.URI, ppos protocol.Position) (token.Position, error) {
 	var noPosn token.Position
 
-	pkg, pgf, err := PackageForFile(ctx, snapshot, uri, WidestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, uri)
 	if err != nil {
 		return noPosn, err
 	}

gopls/internal/lsp/source/inlay_hint.go

@@ -82,7 +82,7 @@ func InlayHint(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng proto
 	ctx, done := event.Start(ctx, "source.InlayHint")
 	defer done()
 
-	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, fmt.Errorf("getting file for InlayHint: %w", err)
 	}

gopls/internal/lsp/source/known_packages.go

@@ -6,7 +6,6 @@ package source
 
 import (
 	"context"
-	"fmt"
 	"go/parser"
 	"go/token"
 	"sort"
@@ -28,15 +27,10 @@ func KnownPackagePaths(ctx context.Context, snapshot Snapshot, fh FileHandle) ([
 	// This algorithm is expressed in terms of Metadata, not Packages,
 	// so it doesn't cause or wait for type checking.
 
-	// Find a Metadata containing the file.
-	metas, err := snapshot.MetadataForFile(ctx, fh.URI())
+	current, err := NarrowestMetadataForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return nil, err // e.g. context cancelled
 	}
-	if len(metas) == 0 {
-		return nil, fmt.Errorf("no loaded package contain file %s", fh.URI())
-	}
-	current := metas[0] // pick one arbitrarily (they should all have the same package path)
 
 	// Parse the file's imports so we can compute which
 	// PackagePaths are imported by this specific file.

gopls/internal/lsp/source/references.go

@@ -171,7 +171,7 @@ func packageReferences(ctx context.Context, snapshot Snapshot, uri span.URI) ([]
 	// The widest package (possibly a test variant) has the
 	// greatest number of files and thus we choose it for the
 	// "internal" references.
-	widest := metas[len(metas)-1]
+	widest := metas[len(metas)-1] // may include _test.go files
 	for _, uri := range widest.CompiledGoFiles {
 		fh, err := snapshot.ReadFile(ctx, uri)
 		if err != nil {
@@ -204,7 +204,7 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
 	// declaration (e.g. because the _test.go files can change the
 	// meaning of a field or method selection), but the narrower
 	// package reports the more broadly referenced object.
-	pkg, pgf, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, uri)
 	if err != nil {
 		return nil, err
 	}
@@ -252,6 +252,7 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
 	if len(variants) == 0 {
 		return nil, fmt.Errorf("no packages for file %q", declURI) // can't happen
 	}
+	// (variants must include ITVs for reverse depedency computation below.)
 
 	// Is object exported?
 	// If so, compute scope and targets of the global search.
@@ -415,6 +416,7 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
 		for id := range globalScope {
 			globalIDs = append(globalIDs, id)
 		}
+		// TODO(adonovan): filter out ITVs?
 		indexes, err := snapshot.References(ctx, globalIDs...)
 		if err != nil {
 			return err
@@ -457,6 +459,7 @@ func expandMethodSearch(ctx context.Context, snapshot Snapshot, method *types.Fu
 		allIDs = append(allIDs, m.ID)
 	}
 	// Search the methodset index of each package in the workspace.
+	// TODO(adonovan): filter out ITVs?
 	indexes, err := snapshot.MethodSets(ctx, allIDs...)
 	if err != nil {
 		return err