Skip to content

Commit

Permalink
gopls/internal/lsp/cache: make IsIntermediateTestVariant a method
Browse files Browse the repository at this point in the history 
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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
  • Loading branch information
@findleyr
findleyr committed Oct 5, 2022
1 parent c5514b7 commit b280e27
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 38 deletions.
15 changes: 6 additions & 9 deletions gopls/internal/lsp/cache/load.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
Expand Down
62 changes: 34 additions & 28 deletions gopls/internal/lsp/cache/metadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/lsp/cache/snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b280e27

Please sign in to comment.