internal/lsp/cache: always compute IsIntermediateTestVariant

It is inconsistent to only compute IsIntermediateTestVariant for
workspace packages, and could be a source of bugs. Always compute it.

Change-Id: I16f40fe44a1145a74ef77fee4b7fd813abe603cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340732
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

Author: findleyr

internal/lsp/cache/load.go

@@ -441,6 +441,12 @@ func (s *snapshot) setMetadataLocked(ctx context.Context, pkgPath PackagePath, p
 		depsErrors: packagesinternal.GetDepsErrors(pkg),
 	}
 
+	// Identify intermediate test variants for later filtering. See the
+	// documentation of IsIntermediateTestVariant for more information.
+	if m.ForTest != "" && m.ForTest != m.PkgPath && m.ForTest+"_test" != m.PkgPath {
+		m.IsIntermediateTestVariant = true
+	}
+
 	for _, err := range pkg.Errors {
 		// Filter out parse errors from go list. We'll get them when we
 		// actually parse, and buggy overlay support may generate spurious
@@ -532,9 +538,6 @@ func (s *snapshot) setMetadataLocked(ctx context.Context, pkgPath PackagePath, p
 			// The test variant of some workspace package or its x_test.
 			// To load it, we need to load the non-test variant with -test.
 			s.workspacePackages[m.ID] = m.ForTest
-		default:
-			// A test variant of some intermediate package. We don't care about it.
-			m.IsIntermediateTestVariant = true
 		}
 	}
 	return m, nil

internal/lsp/cache/metadata.go

@@ -43,6 +43,29 @@ type Metadata struct {
 	// IsIntermediateTestVariant reports whether the given package is an
 	// intermediate test variant, e.g.
 	// "golang.org/x/tools/internal/lsp/cache [golang.org/x/tools/internal/lsp/source.test]".
+	//
+	// Such test variants arise when an x_test package (in this case source_test)
+	// imports a package (in this case cache) that itself imports the the
+	// non-x_test package (in this case source).
+	//
+	// This is done so that the forward transitive closure of source_test has
+	// only one package for the "golang.org/x/tools/internal/lsp/source" import.
+	// The intermediate test variant exists to hold the test variant import:
+	//
+	// golang.org/x/tools/internal/lsp/source_test [golang.org/x/tools/internal/lsp/source.test]
+	//  | "golang.org/x/tools/internal/lsp/cache" -> golang.org/x/tools/internal/lsp/cache [golang.org/x/tools/internal/lsp/source.test]
+	//  | "golang.org/x/tools/internal/lsp/source" -> golang.org/x/tools/internal/lsp/source [golang.org/x/tools/internal/lsp/source.test]
+	//  | ...
+	//
+	// golang.org/x/tools/internal/lsp/cache [golang.org/x/tools/internal/lsp/source.test]
+	//  | "golang.org/x/tools/internal/lsp/source" -> golang.org/x/tools/internal/lsp/source [golang.org/x/tools/internal/lsp/source.test]
+	//  | ...
+	//
+	// We filter these variants out in certain places. For example, there is
+	// generally no reason to run diagnostics or analysis on them.
+	//
+	// TODO(rfindley): this can probably just be a method, since it is derived
+	// from other fields.
 	IsIntermediateTestVariant bool
 }