From e5f719fbe6d517906eb38c02c72f038619c2b7c5 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 3 Sep 2021 16:03:51 -0400 Subject: [PATCH] internal/lsp/source: consider test variants when finding pkg from pos When resolving a position to a package we must consider all packages, including intermediate test variants. This manifests, for example, when jumping to definition in a package that is imported as a test variant (see golang/go#47825). For now, fix this by threading through an 'includeTestVariants' flag to PackagesForFile. This isn't pretty, but should be a trivially safe change: the only effect will be to increase the number of packages considered in FindPackageFromPos. Since we are discussing future changes to the API for querying packages from the snapshot, now did not seem like a good time to undertake significant refactoring. A regtest based on the original issue is included. This CL is joint with rstambler@golang.org. Fixes golang/go#47825 Co-authored-by: Rebecca Stambler Change-Id: I4693ec69b50ed4acd569cff87883769c1edf332b Reviewed-on: https://go-review.googlesource.com/c/tools/+/347563 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- .../internal/regtest/misc/definition_test.go | 43 +++++++++++++++++++ internal/lsp/cache/snapshot.go | 10 ++--- internal/lsp/command.go | 2 +- internal/lsp/diagnostics.go | 4 +- internal/lsp/source/identifier.go | 2 +- internal/lsp/source/implementation.go | 4 +- internal/lsp/source/util.go | 4 +- internal/lsp/source/view.go | 2 +- 8 files changed, 56 insertions(+), 15 deletions(-) diff --git a/gopls/internal/regtest/misc/definition_test.go b/gopls/internal/regtest/misc/definition_test.go index e6181c7022..2b7d1a47d2 100644 --- a/gopls/internal/regtest/misc/definition_test.go +++ b/gopls/internal/regtest/misc/definition_test.go @@ -10,6 +10,7 @@ import ( "testing" . "golang.org/x/tools/internal/lsp/regtest" + "golang.org/x/tools/internal/testenv" "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/tests" @@ -234,3 +235,45 @@ func main() {} }) } } + +// Test for golang/go#47825. +func TestImportTestVariant(t *testing.T) { + testenv.NeedsGo1Point(t, 13) + + const mod = ` +-- go.mod -- +module mod.com + +go 1.12 +-- client/test/role.go -- +package test + +import _ "mod.com/client" + +type RoleSetup struct{} +-- client/client_role_test.go -- +package client_test + +import ( + "testing" + _ "mod.com/client" + ctest "mod.com/client/test" +) + +func TestClient(t *testing.T) { + _ = ctest.RoleSetup{} +} +-- client/client_test.go -- +package client + +import "testing" + +func TestClient(t *testing.T) {} +-- client.go -- +package client +` + Run(t, mod, func(t *testing.T, env *Env) { + env.OpenFile("client/client_role_test.go") + env.GoToDefinition("client/client_role_test.go", env.RegexpSearch("client/client_role_test.go", "RoleSetup")) + }) +} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index d5f230a204..7744f9ebc8 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -453,10 +453,10 @@ func hashUnsavedOverlays(files map[span.URI]source.VersionedFileHandle) string { return hashContents([]byte(strings.Join(unsaved, ""))) } -func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode) ([]source.Package, error) { +func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]source.Package, error) { ctx = event.Label(ctx, tag.URI.Of(uri)) - phs, err := s.packageHandlesForFile(ctx, uri, mode) + phs, err := s.packageHandlesForFile(ctx, uri, mode, includeTestVariants) if err != nil { return nil, err } @@ -474,7 +474,7 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI, mode sourc func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, pkgPolicy source.PackageFilter) (source.Package, error) { ctx = event.Label(ctx, tag.URI.Of(uri)) - phs, err := s.packageHandlesForFile(ctx, uri, mode) + phs, err := s.packageHandlesForFile(ctx, uri, mode, false) if err != nil { return nil, err } @@ -503,7 +503,7 @@ func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source return ph.check(ctx, s) } -func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode) ([]*packageHandle, error) { +func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]*packageHandle, error) { // Check if we should reload metadata for the file. We don't invalidate IDs // (though we should), so the IDs will be a better source of truth than the // metadata. If there are no IDs for the file, then we should also reload. @@ -523,7 +523,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 { + if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant && !includeTestVariants { continue } var parseModes []source.ParseMode diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 61c794b285..36c319f119 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -358,7 +358,7 @@ func (c *commandHandler) RunTests(ctx context.Context, args command.RunTestsArgs func (c *commandHandler) runTests(ctx context.Context, snapshot source.Snapshot, work *progress.WorkDone, uri protocol.DocumentURI, tests, benchmarks []string) error { // TODO: fix the error reporting when this runs async. - pkgs, err := snapshot.PackagesForFile(ctx, uri.SpanURI(), source.TypecheckWorkspace) + pkgs, err := snapshot.PackagesForFile(ctx, uri.SpanURI(), source.TypecheckWorkspace, false) if err != nil { return err } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index d931f51297..acf3820284 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -153,7 +153,7 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps if snapshot.IsBuiltin(ctx, uri) { continue } - pkgs, err := snapshot.PackagesForFile(ctx, uri, source.TypecheckFull) + pkgs, err := snapshot.PackagesForFile(ctx, uri, source.TypecheckFull, false) if err != nil { // TODO (findleyr): we should probably do something with the error here, // but as of now this can fail repeatedly if load fails, so can be too @@ -423,7 +423,7 @@ func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snaps if snapshot.IsBuiltin(ctx, fh.URI()) { return nil } - pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), source.TypecheckWorkspace) + pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), source.TypecheckWorkspace, false) if len(pkgs) > 0 || err == nil { return nil } diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 2ab6cfd633..155f7c4858 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -78,7 +78,7 @@ func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, pos proto ctx, done := event.Start(ctx, "source.Identifier") defer done() - pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), TypecheckAll) + pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), TypecheckAll, false) if err != nil { return nil, err } diff --git a/internal/lsp/source/implementation.go b/internal/lsp/source/implementation.go index 3e35fa7607..04aea37f9b 100644 --- a/internal/lsp/source/implementation.go +++ b/internal/lsp/source/implementation.go @@ -215,7 +215,7 @@ var ( // every package that the file belongs to, in every typechecking mode // applicable. func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, uri span.URI, pp protocol.Position) ([]qualifiedObject, error) { - pkgs, err := s.PackagesForFile(ctx, uri, TypecheckAll) + pkgs, err := s.PackagesForFile(ctx, uri, TypecheckAll, false) if err != nil { return nil, err } @@ -262,7 +262,7 @@ func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key objSearchKey, // try to be comprehensive in case we ever support variations on build // constraints. - pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckAll) + pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckAll, false) if err != nil { return nil, err } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 4ff5d5740e..00ab860fa4 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -282,9 +282,7 @@ func FindPackageFromPos(ctx context.Context, snapshot Snapshot, pos token.Pos) ( return nil, errors.Errorf("no file for pos %v", pos) } uri := span.URIFromPath(tok.Name()) - // Search all packages: some callers may be working with packages not - // type-checked in workspace mode. - pkgs, err := snapshot.PackagesForFile(ctx, uri, TypecheckAll) + pkgs, err := snapshot.PackagesForFile(ctx, uri, TypecheckAll, true) if err != nil { return nil, err } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 19fca6e8cc..2dd0dbc1d7 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -137,7 +137,7 @@ type Snapshot interface { // PackagesForFile returns the packages that this file belongs to, checked // in mode. - PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode) ([]Package, error) + PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode, includeTestVariants bool) ([]Package, error) // PackageForFile returns a single package that this file belongs to, // checked in mode and filtered by the package policy.