diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 37d42db1df4..2550dfba4be 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -655,6 +655,7 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag if !m.Valid { continue } + if !containsPackageLocked(s, m.Metadata) { continue } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 8027b40a0ab..d7e39b31fb5 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -698,27 +698,16 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant() && !withIntermediateTestVariants { continue } - var parseModes []source.ParseMode - switch mode { - case source.TypecheckAll: - if s.workspaceParseMode(id) == source.ParseFull { - parseModes = []source.ParseMode{source.ParseFull} - } else { - parseModes = []source.ParseMode{source.ParseExported, source.ParseFull} - } - case source.TypecheckFull: - parseModes = []source.ParseMode{source.ParseFull} - case source.TypecheckWorkspace: - parseModes = []source.ParseMode{s.workspaceParseMode(id)} + parseMode := source.ParseFull + if mode == source.TypecheckWorkspace { + parseMode = s.workspaceParseMode(id) } - for _, parseMode := range parseModes { - ph, err := s.buildPackageHandle(ctx, id, parseMode) - if err != nil { - return nil, err - } - phs = append(phs, ph) + ph, err := s.buildPackageHandle(ctx, id, parseMode) + if err != nil { + return nil, err } + phs = append(phs, ph) } return phs, nil } diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go index 8d1362a50fb..b9f62f3028d 100644 --- a/gopls/internal/lsp/fake/editor.go +++ b/gopls/internal/lsp/fake/editor.go @@ -1137,7 +1137,8 @@ func (e *Editor) InlayHint(ctx context.Context, path string) ([]protocol.InlayHi return hints, nil } -// References executes a reference request on the server. +// References returns references to the object at (path, pos), as returned by +// the connected LSP server. If no server is connected, it returns (nil, nil). func (e *Editor) References(ctx context.Context, path string, pos Pos) ([]protocol.Location, error) { if e.Server == nil { return nil, nil @@ -1164,6 +1165,8 @@ func (e *Editor) References(ctx context.Context, path string, pos Pos) ([]protoc return locations, nil } +// Rename performs a rename of the object at (path, pos) to newName, using the +// connected LSP server. If no server is connected, it returns nil. func (e *Editor) Rename(ctx context.Context, path string, pos Pos, newName string) error { if e.Server == nil { return nil @@ -1185,6 +1188,28 @@ func (e *Editor) Rename(ctx context.Context, path string, pos Pos, newName strin return nil } +// Implementations returns implementations for the object at (path, pos), as +// returned by the connected LSP server. If no server is connected, it returns +// (nil, nil). +func (e *Editor) Implementations(ctx context.Context, path string, pos Pos) ([]protocol.Location, error) { + if e.Server == nil { + return nil, nil + } + e.mu.Lock() + _, ok := e.buffers[path] + e.mu.Unlock() + if !ok { + return nil, fmt.Errorf("buffer %q is not open", path) + } + params := &protocol.ImplementationParams{ + TextDocumentPositionParams: protocol.TextDocumentPositionParams{ + TextDocument: e.TextDocumentIdentifier(path), + Position: pos.ToProtocolPosition(), + }, + } + return e.Server.Implementation(ctx, params) +} + func (e *Editor) RenameFile(ctx context.Context, oldPath, newPath string) error { closed, opened, err := e.renameBuffers(ctx, oldPath, newPath) if err != nil { diff --git a/gopls/internal/lsp/regtest/wrappers.go b/gopls/internal/lsp/regtest/wrappers.go index 0f7cc9a1a66..63b59917c10 100644 --- a/gopls/internal/lsp/regtest/wrappers.go +++ b/gopls/internal/lsp/regtest/wrappers.go @@ -389,8 +389,7 @@ func (e *Env) WorkspaceSymbol(sym string) []protocol.SymbolInformation { return ans } -// References calls textDocument/references for the given path at the given -// position. +// References wraps Editor.References, calling t.Fatal on any error. func (e *Env) References(path string, pos fake.Pos) []protocol.Location { e.T.Helper() locations, err := e.Editor.References(e.Ctx, path, pos) @@ -408,6 +407,16 @@ func (e *Env) Rename(path string, pos fake.Pos, newName string) { } } +// Implementations wraps Editor.Implementations, calling t.Fatal on any error. +func (e *Env) Implementations(path string, pos fake.Pos) []protocol.Location { + e.T.Helper() + locations, err := e.Editor.Implementations(e.Ctx, path, pos) + if err != nil { + e.T.Fatal(err) + } + return locations +} + // RenameFile wraps Editor.RenameFile, calling t.Fatal on any error. func (e *Env) RenameFile(oldPath, newPath string) { e.T.Helper() diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go index e2c60bb55e7..2da488dd67a 100644 --- a/gopls/internal/lsp/source/implementation.go +++ b/gopls/internal/lsp/source/implementation.go @@ -208,36 +208,31 @@ var ( errNoObjectFound = errors.New("no object found") ) -// qualifiedObjsAtProtocolPos returns info for all the type.Objects -// referenced at the given position. An object will be returned for -// every package that the file belongs to, in every typechecking mode -// applicable. +// qualifiedObjsAtProtocolPos returns info for all the types.Objects referenced +// at the given position, for the following selection of packages: +// +// 1. all packages (including all test variants), in their workspace parse mode +// 2. if not included above, at least one package containing uri in full parse mode +// +// Finding objects in (1) ensures that we locate references within all +// workspace packages, including in x_test packages. Including (2) ensures that +// we find local references in the current package, for non-workspace packages +// that may be open. func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, uri span.URI, pp protocol.Position) ([]qualifiedObject, error) { - offset, err := protocolPositionToOffset(ctx, s, uri, pp) + fh, err := s.GetFile(ctx, uri) if err != nil { return nil, err } - return qualifiedObjsAtLocation(ctx, s, positionKey{uri, offset}, map[positionKey]bool{}) -} - -func protocolPositionToOffset(ctx context.Context, s Snapshot, uri span.URI, pp protocol.Position) (int, error) { - pkgs, err := s.PackagesForFile(ctx, uri, TypecheckAll, false) + content, err := fh.Read() if err != nil { - return 0, err - } - if len(pkgs) == 0 { - return 0, errNoObjectFound - } - pkg := pkgs[0] - pgf, err := pkg.File(uri) - if err != nil { - return 0, err + return nil, err } - pos, err := pgf.Mapper.Pos(pp) + m := protocol.NewColumnMapper(uri, content) + offset, err := m.Offset(pp) if err != nil { - return 0, err + return nil, err } - return safetoken.Offset(pgf.Tok, pos) + return qualifiedObjsAtLocation(ctx, s, positionKey{uri, offset}, map[positionKey]bool{}) } // A positionKey identifies a byte offset within a file (URI). @@ -251,8 +246,8 @@ type positionKey struct { offset int } -// qualifiedObjsAtLocation finds all objects referenced at offset in uri, across -// all packages in the snapshot. +// qualifiedObjsAtLocation finds all objects referenced at offset in uri, +// across all packages in the snapshot. func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key positionKey, seen map[positionKey]bool) ([]qualifiedObject, error) { if seen[key] { return nil, nil @@ -268,11 +263,31 @@ func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key positionKey, s // try to be comprehensive in case we ever support variations on build // constraints. - pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckAll, false) + pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckWorkspace, true) if err != nil { return nil, err } + // In order to allow basic references/rename/implementations to function when + // non-workspace packages are open, ensure that we have at least one fully + // parsed package for the current file. This allows us to find references + // inside the open package. Use WidestPackage to capture references in test + // files. + hasFullPackage := false + for _, pkg := range pkgs { + if pkg.ParseMode() == ParseFull { + hasFullPackage = true + break + } + } + if !hasFullPackage { + pkg, err := s.PackageForFile(ctx, key.uri, TypecheckFull, WidestPackage) + if err != nil { + return nil, err + } + pkgs = append(pkgs, pkg) + } + // report objects in the order we encounter them. This ensures that the first // result is at the cursor... var qualifiedObjs []qualifiedObject diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go index 9a10b38d161..de6687ad49f 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -62,7 +62,7 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit } if inPackageName { - renamingPkg, err := s.PackageForFile(ctx, f.URI(), TypecheckAll, NarrowestPackage) + renamingPkg, err := s.PackageForFile(ctx, f.URI(), TypecheckWorkspace, NarrowestPackage) if err != nil { return nil, err } diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go index c926dedcf6b..2f0ad56ec66 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -76,7 +76,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot err := errors.New("can't rename package: LSP client does not support file renaming") return nil, err, err } - renamingPkg, err := snapshot.PackageForFile(ctx, f.URI(), TypecheckAll, NarrowestPackage) + renamingPkg, err := snapshot.PackageForFile(ctx, f.URI(), TypecheckWorkspace, NarrowestPackage) if err != nil { return nil, err, err } @@ -169,7 +169,7 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, // TODO(rfindley): but is this correct? What about x_test packages that // import the renaming package? const includeTestVariants = false - pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckAll, includeTestVariants) + pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckWorkspace, includeTestVariants) if err != nil { return nil, true, err } diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index bf152805966..cd0c8417f47 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -437,16 +437,11 @@ var AllParseModes = []ParseMode{ParseHeader, ParseExported, ParseFull} type TypecheckMode int const ( - // Invalid default value. - TypecheckUnknown TypecheckMode = iota // TypecheckFull means to use ParseFull. - TypecheckFull + TypecheckFull TypecheckMode = iota // TypecheckWorkspace means to use ParseFull for workspace packages, and // ParseExported for others. TypecheckWorkspace - // TypecheckAll means ParseFull for workspace packages, and both Full and - // Exported for others. Only valid for some functions. - TypecheckAll ) type VersionedFileHandle interface { diff --git a/gopls/internal/regtest/misc/references_test.go b/gopls/internal/regtest/misc/references_test.go index 058aad3e60b..2cd4359d313 100644 --- a/gopls/internal/regtest/misc/references_test.go +++ b/gopls/internal/regtest/misc/references_test.go @@ -6,9 +6,12 @@ package misc import ( "fmt" + "sort" "strings" "testing" + "github.com/google/go-cmp/cmp" + "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" ) @@ -169,3 +172,119 @@ func main() { } }) } + +// Test for golang/go#43144. +// +// Verify that we search for references and implementations in intermediate +// test variants. +func TestReferencesInTestVariants(t *testing.T) { + const files = ` +-- go.mod -- +module foo.mod + +go 1.12 +-- foo/foo.go -- +package foo + +import "foo.mod/bar" + +const Foo = 42 + +type T int +type Interface interface{ M() } + +func _() { + _ = bar.Blah +} + +-- bar/bar.go -- +package bar + +var Blah = 123 + +-- bar/bar_test.go -- +package bar + +type Mer struct{} +func (Mer) M() {} + +func TestBar() { + _ = Blah +} +-- bar/bar_x_test.go -- +package bar_test + +import ( + "foo.mod/bar" + "foo.mod/foo" +) + +type Mer struct{} +func (Mer) M() {} + +func _() { + _ = bar.Blah + _ = foo.Foo +} +` + + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("foo/foo.go") + + // Helper to map locations relative file paths. + fileLocations := func(locs []protocol.Location) []string { + var got []string + for _, loc := range locs { + got = append(got, env.Sandbox.Workdir.URIToPath(loc.URI)) + } + sort.Strings(got) + return got + } + + refTests := []struct { + re string + wantRefs []string + }{ + // Blah is referenced: + // - inside the foo.mod/bar (ordinary) package + // - inside the foo.mod/bar [foo.mod/bar.test] test variant package + // - from the foo.mod/bar_test [foo.mod/bar.test] x_test package + // - from the foo.mod/foo package + {"Blah", []string{"bar/bar.go", "bar/bar_test.go", "bar/bar_x_test.go", "foo/foo.go"}}, + + // Foo is referenced in bar_x_test.go via the intermediate test variant + // foo.mod/foo [foo.mod/bar.test]. + {"Foo", []string{"bar/bar_x_test.go", "foo/foo.go"}}, + } + + for _, test := range refTests { + pos := env.RegexpSearch("foo/foo.go", test.re) + refs := env.References("foo/foo.go", pos) + + got := fileLocations(refs) + if diff := cmp.Diff(test.wantRefs, got); diff != "" { + t.Errorf("References(%q) returned unexpected diff (-want +got):\n%s", test.re, diff) + } + } + + implTests := []struct { + re string + wantImpls []string + }{ + // Interface is implemented both in foo.mod/bar [foo.mod/bar.test] (which + // doesn't import foo), and in foo.mod/bar_test [foo.mod/bar.test], which + // imports the test variant of foo. + {"Interface", []string{"bar/bar_test.go", "bar/bar_x_test.go"}}, + } + + for _, test := range implTests { + pos := env.RegexpSearch("foo/foo.go", test.re) + refs := env.Implementations("foo/foo.go", pos) + + got := fileLocations(refs) + if diff := cmp.Diff(test.wantImpls, got); diff != "" { + t.Errorf("Implementations(%q) returned unexpected diff (-want +got):\n%s", test.re, diff) + } + } + }) +}