From 1d19788894f308098139d81ac980580298f45d6b Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 6 Aug 2021 10:15:16 -0400 Subject: [PATCH] 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 gopls-CI: kokoro Run-TryBot: Robert Findley TryBot-Result: Gopher Robot --- internal/lsp/cache/load.go | 9 ++++++--- internal/lsp/cache/metadata.go | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 1d4921a8cd5..96c2a0733a5 100644 --- a/internal/lsp/cache/load.go +++ b/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 diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go index 618578dd896..c2a21969d88 100644 --- a/internal/lsp/cache/metadata.go +++ b/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 }