From b280e27dc741a4ab9d9520e2319f4fb1653e88e3 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 4 Oct 2022 09:49:34 -0400 Subject: [PATCH] gopls/internal/lsp/cache: make IsIntermediateTestVariant a method Metadata.IsIntermediateTestVariant can be derived from PkgPath and ForTest, so make it a method. Along the way, document the easily missed fact that intermediate test variants are not workspace packages. For golang/go#55293 Change-Id: Ie03011aef9c91ebce89e8aad01ef39b65bdde09a Reviewed-on: https://go-review.googlesource.com/c/tools/+/438497 Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot gopls-CI: kokoro Run-TryBot: Robert Findley --- gopls/internal/lsp/cache/load.go | 15 +++---- gopls/internal/lsp/cache/metadata.go | 62 +++++++++++++++------------- gopls/internal/lsp/cache/snapshot.go | 2 +- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 2bf4a337dfe..287c310a1f8 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -17,11 +17,11 @@ import ( "time" "golang.org/x/tools/go/packages" - "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/gocommand" - "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/internal/event" + "golang.org/x/tools/internal/event/tag" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/span" ) @@ -504,12 +504,6 @@ func buildMetadata(ctx context.Context, pkgPath PackagePath, pkg *packages.Packa } updates[id] = m - // 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 @@ -686,6 +680,9 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag case m.ForTest == m.PkgPath, m.ForTest+"_test" == m.PkgPath: // The test variant of some workspace package or its x_test. // To load it, we need to load the non-test variant with -test. + // + // Notably, this excludes intermediate test variants from workspace + // packages. workspacePackages[m.ID] = m.ForTest } } diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go index 711508c9a11..7778996061b 100644 --- a/gopls/internal/lsp/cache/metadata.go +++ b/gopls/internal/lsp/cache/metadata.go @@ -39,34 +39,6 @@ type Metadata struct { // Config is the *packages.Config associated with the loaded package. Config *packages.Config - - // IsIntermediateTestVariant reports whether the given package is an - // intermediate test variant, e.g. - // "golang.org/x/tools/gopls/internal/lsp/cache [golang.org/x/tools/gopls/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/gopls/internal/lsp/source" import. - // The intermediate test variant exists to hold the test variant import: - // - // golang.org/x/tools/gopls/internal/lsp/source_test [golang.org/x/tools/gopls/internal/lsp/source.test] - // | "golang.org/x/tools/gopls/internal/lsp/cache" -> golang.org/x/tools/gopls/internal/lsp/cache [golang.org/x/tools/gopls/internal/lsp/source.test] - // | "golang.org/x/tools/gopls/internal/lsp/source" -> golang.org/x/tools/gopls/internal/lsp/source [golang.org/x/tools/gopls/internal/lsp/source.test] - // | ... - // - // golang.org/x/tools/gopls/internal/lsp/cache [golang.org/x/tools/gopls/internal/lsp/source.test] - // | "golang.org/x/tools/gopls/internal/lsp/source" -> golang.org/x/tools/gopls/internal/lsp/source [golang.org/x/tools/gopls/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 } // Name implements the source.Metadata interface. @@ -79,6 +51,40 @@ func (m *Metadata) PackagePath() string { return string(m.PkgPath) } +// IsIntermediateTestVariant reports whether the given package is an +// intermediate test variant, e.g. "net/http [net/url.test]". +// +// 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: +// +// net/url_test [net/url.test] +// +// | "net/http" -> net/http [net/url.test] +// | "net/url" -> net/url [net/url.test] +// | ... +// +// net/http [net/url.test] +// +// | "net/url" -> net/url [net/url.test] +// | ... +// +// This restriction propagates throughout the import graph of net/http: for +// every package imported by net/http that imports net/url, there must be an +// intermediate test variant that instead imports "net/url [net/url.test]". +// +// As one can see from the example of net/url and net/http, intermediate test +// variants can result in many additional packages that are essentially (but +// not quite) identical. For this reason, we filter these variants wherever +// possible. +func (m *Metadata) IsIntermediateTestVariant() bool { + return m.ForTest != "" && m.ForTest != m.PkgPath && m.ForTest+"_test" != m.PkgPath +} + // ModuleInfo implements the source.Metadata interface. func (m *Metadata) ModuleInfo() *packages.Module { return m.Module diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index d13f56ca370..fb4e78bfde7 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -695,7 +695,7 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode for _, id := range knownIDs { // Filter out any intermediate test variants. We typically aren't // interested in these packages for file= style queries. - if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant && !withIntermediateTestVariants { + if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant() && !withIntermediateTestVariants { continue } var parseModes []source.ParseMode