gopls/internal/lsp/source: more ITV filtering

This change does additional filtering of intermediate
test variants for the cases where we invoke type
checking indirectly (through filecached derivatives)
not via Snapshot.TypeCheck.

Also:
- document the head-spinning subtlety of ITVs.
- change RemoveIntermediateTestVariants to use a pointer
  to a slice so that it is impossible to forget to use
  the result. D'oh!

Fixes golang/go#59458

Change-Id: I156385456cefeb9dc3a84f23db3b2fa5ef9f244a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487136
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>

gopls/internal/lsp/diagnostics.go

@@ -245,7 +245,7 @@ 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)
+		source.RemoveIntermediateTestVariants(&metas)
 		for _, m := range metas {
 			toDiagnose[m.ID] = m
 		}

gopls/internal/lsp/source/implementation.go

@@ -88,7 +88,7 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p
 	if err != nil {
 		return nil, err
 	}
-	RemoveIntermediateTestVariants(declMetas)
+	RemoveIntermediateTestVariants(&declMetas)
 	if len(declMetas) == 0 {
 		return nil, fmt.Errorf("no packages for file %s", declURI)
 	}
@@ -164,14 +164,14 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p
 	if err != nil {
 		return nil, err
 	}
+	RemoveIntermediateTestVariants(&globalMetas)
 	globalIDs := make([]PackageID, 0, len(globalMetas))
 	for _, m := range globalMetas {
 		if m.PkgPath == declPkg.Metadata().PkgPath {
 			continue // declaring package is handled by local implementation
 		}
 		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)

gopls/internal/lsp/source/linkname.go

@@ -101,7 +101,7 @@ func findLinkname(ctx context.Context, snapshot Snapshot, fh FileHandle, pos pro
 	if err != nil {
 		return nil, err
 	}
-	metas = RemoveIntermediateTestVariants(metas)
+	RemoveIntermediateTestVariants(&metas)
 	for _, meta := range metas {
 		if meta.PkgPath == pkgPath {
 			pkgMeta = meta

gopls/internal/lsp/source/references.go

@@ -252,7 +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.)
+	// (variants must include ITVs for reverse dependency computation below.)
 
 	// Is object exported?
 	// If so, compute scope and targets of the global search.
@@ -413,10 +413,18 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
 	// Compute global references for selected reverse dependencies.
 	group.Go(func() error {
 		var globalIDs []PackageID
-		for id := range globalScope {
+		for id, m := range globalScope {
+			// Skip intermediate test variants.
+			//
+			// Strictly, an ITV's cross-reference index
+			// may have different objectpaths from the
+			// ordinary variant, but we ignore that. See
+			// explanation at IsIntermediateTestVariant.
+			if m.IsIntermediateTestVariant() {
+				continue
+			}
 			globalIDs = append(globalIDs, id)
 		}
-		// TODO(adonovan): filter out ITVs?
 		indexes, err := snapshot.References(ctx, globalIDs...)
 		if err != nil {
 			return err
@@ -454,12 +462,14 @@ func expandMethodSearch(ctx context.Context, snapshot Snapshot, method *types.Fu
 	if err != nil {
 		return err
 	}
+	// Discard ITVs to avoid redundant type-checking.
+	// (See explanation at IsIntermediateTestVariant.)
+	RemoveIntermediateTestVariants(&metas)
 	allIDs := make([]PackageID, 0, len(metas))
 	for _, m := range metas {
 		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

gopls/internal/lsp/source/rename.go

@@ -290,18 +290,19 @@ func Rename(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Po
 func renameOrdinary(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Position, newName string) (map[span.URI][]diff.Edit, error) {
 	// Type-check the referring package and locate the object(s).
 	//
-	// Unlike PackageForFile, we choose the widest variant as,
-	// for non-exported identifiers, it is the only package we need.
-	// (In case you're wondering why 'references' doesn't also want
-	// the widest variant: it computes the union across all variants.)
+	// Unlike NarrowestPackageForFile, this operation prefers the
+	// widest variant as, for non-exported identifiers, it is the
+	// only package we need. (In case you're wondering why
+	// 'references' doesn't also want the widest variant: it
+	// computes the union across all variants.)
 	var targets map[types.Object]ast.Node
 	var pkg Package
 	{
 		metas, err := snapshot.MetadataForFile(ctx, f.URI())
 		if err != nil {
 			return nil, err
 		}
-		RemoveIntermediateTestVariants(metas)
+		RemoveIntermediateTestVariants(&metas)
 		if len(metas) == 0 {
 			return nil, fmt.Errorf("no package metadata for file %s", f.URI())
 		}

gopls/internal/lsp/source/view.go

@@ -190,10 +190,11 @@ type Snapshot interface {
 	// The resulting packages' types may belong to different importers,
 	// so types from different packages are incommensurable.
 	//
-	// There should never be any need to type-check an
-	// intermediate test variant (ITV) package. Callers should
-	// apply RemoveIntermediateTestVariants (or equivalent) before
-	// this method, or any of the potentially type-checking methods below.
+	// In general, clients should never need to type-checked
+	// syntax for an intermediate test variant (ITV) package.
+	// Callers should apply RemoveIntermediateTestVariants (or
+	// equivalent) before this method, or any of the potentially
+	// type-checking methods below.
 	TypeCheck(ctx context.Context, ids ...PackageID) ([]Package, error)
 
 	// PackageDiagnostics returns diagnostics for files contained in specified
@@ -229,7 +230,7 @@ func NarrowestMetadataForFile(ctx context.Context, snapshot Snapshot, uri span.U
 	if err != nil {
 		return nil, err
 	}
-	RemoveIntermediateTestVariants(metas)
+	RemoveIntermediateTestVariants(&metas)
 	if len(metas) == 0 {
 		return nil, fmt.Errorf("no package metadata for file %s", uri)
 	}
@@ -268,7 +269,7 @@ func NarrowestPackageForFile(ctx context.Context, snapshot Snapshot, uri span.UR
 	if err != nil {
 		return nil, nil, err
 	}
-	RemoveIntermediateTestVariants(metas)
+	RemoveIntermediateTestVariants(&metas)
 	if len(metas) == 0 {
 		return nil, nil, fmt.Errorf("no package metadata for file %s", uri)
 	}
@@ -529,15 +530,18 @@ type Metadata struct {
 func (m *Metadata) String() string { return string(m.ID) }
 
 // IsIntermediateTestVariant reports whether the given package is an
-// intermediate test variant, e.g. "net/http [net/url.test]".
+// intermediate test variant (ITV), e.g. "net/http [net/url.test]".
+//
+// An ITV has identical syntax to the regular variant, but different
+// import metadata (DepsBy{Imp,Pkg}Path).
 //
 // Such test variants arise when an x_test package (in this case net/url_test)
 // imports a package (in this case net/http) that itself imports the the
 // non-x_test package (in this case net/url).
 //
 // This is done so that the forward transitive closure of net/url_test has
 // only one package for the "net/url" import.
-// The intermediate test variant exists to hold the test variant import:
+// The ITV exists to hold the test variant import:
 //
 // net/url_test [net/url.test]
 //
@@ -558,19 +562,86 @@ func (m *Metadata) String() string { return string(m.ID) }
 // variants can result in many additional packages that are essentially (but
 // not quite) identical. For this reason, we filter these variants wherever
 // possible.
+//
+// # Why we mostly ignore intermediate test variants
+//
+// In projects with complicated tests, there may be a very large
+// number of ITVs--asymptotically more than the number of ordinary
+// variants. Since they have identical syntax, it is fine in most
+// cases to ignore them since the results of analyzing the ordinary
+// variant suffice. However, this is not entirely sound.
+//
+// Consider this package:
+//
+//	// p/p.go -- in all variants of p
+//	package p
+//	type T struct { io.Closer }
+//
+//	// p/p_test.go -- in test variant of p
+//	package p
+//	func (T) Close() error { ... }
+//
+// The ordinary variant "p" defines T with a Close method promoted
+// from io.Closer. But its test variant "p [p.test]" defines a type T
+// with a Close method from p_test.go.
+//
+// Now consider a package q that imports p, perhaps indirectly. Within
+// it, T.Close will resolve to the first Close method:
+//
+//	// q/q.go -- in all variants of q
+//	package q
+//	import "p"
+//	var _ = new(p.T).Close
+//
+// Let's assume p also contains this file defining an external test (xtest):
+//
+//	// p/p_x_test.go -- external test of p
+//	package p_test
+//	import ( "q"; "testing" )
+//	func Test(t *testing.T) { ... }
+//
+// Note that q imports p, but p's xtest imports q. Now, in "q
+// [p.test]", the intermediate test variant of q built for p's
+// external test, T.Close resolves not to the io.Closer.Close
+// interface method, but to the concrete method of T.Close
+// declared in p_test.go.
+//
+// If we now request all references to the T.Close declaration in
+// p_test.go, the result should include the reference from q's ITV.
+// (It's not just methods that can be affected; fields can too, though
+// it requires bizarre code to achieve.)
+//
+// As a matter of policy, gopls mostly ignores this subtlety,
+// because to account for it would require that we type-check every
+// intermediate test variant of p, of which there could be many.
+// Good code doesn't rely on such trickery.
+//
+// Most callers of MetadataForFile call RemoveIntermediateTestVariants
+// to discard them before requesting type checking, or the products of
+// type-checking such as the cross-reference index or method set index.
+//
+// MetadataForFile doesn't do this filtering itself becaused in some
+// cases we need to make a reverse dependency query on the metadata
+// graph, and it's important to include the rdeps of ITVs in that
+// query. But the filtering of ITVs should be applied after that step,
+// before type checking.
+//
+// In general, we should never type check an ITV.
 func (m *Metadata) IsIntermediateTestVariant() bool {
 	return m.ForTest != "" && m.ForTest != m.PkgPath && m.ForTest+"_test" != m.PkgPath
 }
 
 // RemoveIntermediateTestVariants removes intermediate test variants, modifying the array.
-func RemoveIntermediateTestVariants(metas []*Metadata) []*Metadata {
+// We use a pointer to a slice make it impossible to forget to use the result.
+func RemoveIntermediateTestVariants(pmetas *[]*Metadata) {
+	metas := *pmetas
 	res := metas[:0]
 	for _, m := range metas {
 		if !m.IsIntermediateTestVariant() {
 			res = append(res, m)
 		}
 	}
-	return res
+	*pmetas = res
 }
 
 var ErrViewExists = errors.New("view already exists for session")