gopls/internal/lsp/source: find references in test packages
When finding references or implementations, we must include objects
defined in intermediate test variants. However, as we have seen we
should be careful to avoid accidentally type-checking intermediate test
variants in ParseFull, which is not their workspace parse mode.
Therefore eliminate the problematic TypecheckAll type-check mode in
favor of special handling in this one case where it is necessary.
Along the way:
- Simplify the mapping of protocol position->offset. This should not
require getting a package, or even parsing a file. For isolation,
just use the NewColumnMapper constructor, even though it may
technically result in building a token.File multiple times.
- Update package renaming logic to use TypecheckWorkspace, since we
only need to rename within the workspace.
- Add regtest support for Implementations requests.
Fixes golang/go#43144
Change-Id: I41f684ad766f5af805abbd7c5ee0a010ff9b9b8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/438755
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 37d42db..2550dfb 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -655,6 +655,7 @@
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 8027b40..d7e39b3 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -698,27 +698,16 @@
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 8d1362a..b9f62f3 100644
--- a/gopls/internal/lsp/fake/editor.go
+++ b/gopls/internal/lsp/fake/editor.go
@@ -1137,7 +1137,8 @@
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 @@
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 @@
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 0f7cc9a..63b5991 100644
--- a/gopls/internal/lsp/regtest/wrappers.go
+++ b/gopls/internal/lsp/regtest/wrappers.go
@@ -389,8 +389,7 @@
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 @@
}
}
+// 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 e2c60bb..2da488d 100644
--- a/gopls/internal/lsp/source/implementation.go
+++ b/gopls/internal/lsp/source/implementation.go
@@ -208,38 +208,33 @@
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
+ }
+ content, err := fh.Read()
+ if err != nil {
+ return nil, err
+ }
+ m := protocol.NewColumnMapper(uri, content)
+ offset, err := m.Offset(pp)
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)
- 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
- }
- pos, err := pgf.Mapper.Pos(pp)
- if err != nil {
- return 0, err
- }
- return safetoken.Offset(pgf.Tok, pos)
-}
-
// A positionKey identifies a byte offset within a file (URI).
//
// When a file has been parsed multiple times in the same FileSet,
@@ -251,8 +246,8 @@
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 @@
// 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 9a10b38..de6687a 100644
--- a/gopls/internal/lsp/source/references.go
+++ b/gopls/internal/lsp/source/references.go
@@ -62,7 +62,7 @@
}
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 c926ded..2f0ad56 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -76,7 +76,7 @@
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 @@
// 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 bf15280..cd0c841 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -437,16 +437,11 @@
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 {
