From 7f77468ce696ab54e2e1bc31b7eda9f65d1a34f0 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 3 Sep 2021 16:03:51 -0400 Subject: [PATCH] [gopls-release-branch.0.7] 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 (cherry picked from commit e5f719fbe6d517906eb38c02c72f038619c2b7c5) Reviewed-on: https://go-review.googlesource.com/c/tools/+/348372 --- .../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 e6181c70224..2b7d1a47d29 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 d5f230a204e..7744f9ebc8f 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 61c794b2850..36c319f1196 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 d931f512976..acf38202848 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 2ab6cfd6331..155f7c48587 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 3e35fa7607e..04aea37f9b0 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 4ff5d5740ed..00ab860fa4b 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 19fca6e8cca..2dd0dbc1d70 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.