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 <rstambler@golang.org>
Change-Id: I4693ec69b50ed4acd569cff87883769c1edf332b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/347563
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/misc/definition_test.go b/gopls/internal/regtest/misc/definition_test.go
index e6181c7..2b7d1a4 100644
--- a/gopls/internal/regtest/misc/definition_test.go
+++ b/gopls/internal/regtest/misc/definition_test.go
@@ -10,6 +10,7 @@
"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 @@
})
}
}
+
+// 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 d5f230a..7744f9e 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -453,10 +453,10 @@
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) 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 @@
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 @@
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 61c794b..36c319f 100644
--- a/internal/lsp/command.go
+++ b/internal/lsp/command.go
@@ -358,7 +358,7 @@
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 d931f51..acf3820 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -153,7 +153,7 @@
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 @@
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 2ab6cfd..155f7c4 100644
--- a/internal/lsp/source/identifier.go
+++ b/internal/lsp/source/identifier.go
@@ -78,7 +78,7 @@
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 3e35fa7..04aea37 100644
--- a/internal/lsp/source/implementation.go
+++ b/internal/lsp/source/implementation.go
@@ -215,7 +215,7 @@
// 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 @@
// 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 4ff5d57..00ab860 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -282,9 +282,7 @@
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 19fca6e..2dd0dbc 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -137,7 +137,7 @@
// 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.
