gopls/internal/lsp/source: reduce usage of TypecheckWorkspace
Fix calls to TypeCheck using TypecheckWorkspace where we really want
TypecheckFull. Also, use NarrowestPackage where it suffices.
In approximate order of appearance:
- Code actions, semantic tokens, code lens, and document highlights are
all scoped to a file; the narrowest package for that file should
suffice.
- When completing at a position, we need the full package to find
enclosing context. Furthermore, that file is open, and so will be
fully type-checked by other operations.
- Ditto for suggested fixes, inlay hints, and signature help.
The current behavior leads to incorrect or missing functionality when
outside the workspace. I did not add comprehensive tests demonstrating
this in all cases, but added one for signature help.
For golang/go#57987
Change-Id: I8270d0f0a0787e36bd4103378176d150426d37f2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/463375
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/code_action.go b/gopls/internal/lsp/code_action.go
index c91def6..58bb59e 100644
--- a/gopls/internal/lsp/code_action.go
+++ b/gopls/internal/lsp/code_action.go
@@ -158,7 +158,7 @@
// Type-check the package and also run analysis,
// then combine their diagnostics.
- pkg, _, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.WidestPackage)
+ pkg, _, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.NarrowestPackage)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/regtest/wrappers.go b/gopls/internal/lsp/regtest/wrappers.go
index 03f3551..c206296 100644
--- a/gopls/internal/lsp/regtest/wrappers.go
+++ b/gopls/internal/lsp/regtest/wrappers.go
@@ -131,6 +131,9 @@
// RegexpSearch returns the starting position of the first match for re in the
// buffer specified by name, calling t.Fatal on any error. It first searches
// for the position in open buffers, then in workspace files.
+//
+// TODO(rfindley): RegexpSearch should return a protocol.Location (but that is
+// a large change).
func (e *Env) RegexpSearch(name, re string) protocol.Position {
e.T.Helper()
pos, err := e.Editor.RegexpSearch(name, re)
diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go
index 9332877..0406311 100644
--- a/gopls/internal/lsp/semantic.go
+++ b/gopls/internal/lsp/semantic.go
@@ -93,7 +93,7 @@
if kind != source.Go {
return nil, nil
}
- pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.WidestPackage)
+ pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.NarrowestPackage)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/code_lens.go b/gopls/internal/lsp/source/code_lens.go
index a864981..dd66fe5 100644
--- a/gopls/internal/lsp/source/code_lens.go
+++ b/gopls/internal/lsp/source/code_lens.go
@@ -100,7 +100,7 @@
if !strings.HasSuffix(fh.URI().Filename(), "_test.go") {
return out, nil
}
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, WidestPackage)
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return out, err
}
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index 3343a1a..f27190c 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -430,7 +430,7 @@
startTime := time.Now()
- pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckWorkspace, source.NarrowestPackage)
+ pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.TypecheckFull, source.NarrowestPackage)
if err != nil || pgf.File.Package == token.NoPos {
// If we can't parse this file or find position for the package
// keyword, it may be missing a package declaration. Try offering
diff --git a/gopls/internal/lsp/source/fix.go b/gopls/internal/lsp/source/fix.go
index 68e5f8e..49bce04 100644
--- a/gopls/internal/lsp/source/fix.go
+++ b/gopls/internal/lsp/source/fix.go
@@ -53,12 +53,16 @@
// singleFile calls analyzers that expect inputs for a single file
func singleFile(sf singleFileFixFunc) SuggestedFixFunc {
return func(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
- fset, rng, src, file, pkg, info, err := getAllSuggestedFixInputs(ctx, snapshot, fh, pRng)
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return nil, nil, err
}
- fix, err := sf(fset, rng, src, file, pkg, info)
- return fset, fix, err
+ rng, err := pgf.RangeToTokenRange(pRng)
+ if err != nil {
+ return nil, nil, err
+ }
+ fix, err := sf(pkg.FileSet(), rng, pgf.Src, pgf.File, pkg.GetTypes(), pkg.GetTypesInfo())
+ return pkg.FileSet(), fix, err
}
}
@@ -130,17 +134,3 @@
}
return edits, nil
}
-
-// getAllSuggestedFixInputs is a helper function to collect all possible needed
-// inputs for an AppliesFunc or SuggestedFixFunc.
-func getAllSuggestedFixInputs(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng protocol.Range) (*token.FileSet, safetoken.Range, []byte, *ast.File, *types.Package, *types.Info, error) {
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, NarrowestPackage)
- if err != nil {
- return nil, safetoken.Range{}, nil, nil, nil, nil, fmt.Errorf("getting file for Identifier: %w", err)
- }
- rng, err := pgf.RangeToTokenRange(pRng)
- if err != nil {
- return nil, safetoken.Range{}, nil, nil, nil, nil, err
- }
- return pkg.FileSet(), rng, pgf.Src, pgf.File, pkg.GetTypes(), pkg.GetTypesInfo(), nil
-}
diff --git a/gopls/internal/lsp/source/highlight.go b/gopls/internal/lsp/source/highlight.go
index d0f77e6..78718e5 100644
--- a/gopls/internal/lsp/source/highlight.go
+++ b/gopls/internal/lsp/source/highlight.go
@@ -23,7 +23,7 @@
// We always want fully parsed files for highlight, regardless
// of whether the file belongs to a workspace package.
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, WidestPackage)
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return nil, fmt.Errorf("getting package for Highlight: %w", err)
}
diff --git a/gopls/internal/lsp/source/inlay_hint.go b/gopls/internal/lsp/source/inlay_hint.go
index 08fefa2..da2e31a 100644
--- a/gopls/internal/lsp/source/inlay_hint.go
+++ b/gopls/internal/lsp/source/inlay_hint.go
@@ -82,7 +82,7 @@
ctx, done := event.Start(ctx, "source.InlayHint")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, NarrowestPackage)
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return nil, fmt.Errorf("getting file for InlayHint: %w", err)
}
diff --git a/gopls/internal/lsp/source/signature_help.go b/gopls/internal/lsp/source/signature_help.go
index e655481..1d43dc5 100644
--- a/gopls/internal/lsp/source/signature_help.go
+++ b/gopls/internal/lsp/source/signature_help.go
@@ -20,7 +20,9 @@
ctx, done := event.Start(ctx, "source.SignatureHelp")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, NarrowestPackage)
+ // We need full type-checking here, as we must type-check function bodies in
+ // order to provide signature help at the requested position.
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return nil, 0, fmt.Errorf("getting file for SignatureHelp: %w", err)
}
diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go
index 7b6f4f4..c3f2661 100644
--- a/gopls/internal/lsp/source/stub.go
+++ b/gopls/internal/lsp/source/stub.go
@@ -26,7 +26,7 @@
)
func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle, rng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckWorkspace, NarrowestPackage)
+ pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
if err != nil {
return nil, nil, fmt.Errorf("GetTypedFile: %w", err)
}
diff --git a/gopls/internal/regtest/misc/signature_help_test.go b/gopls/internal/regtest/misc/signature_help_test.go
new file mode 100644
index 0000000..05f5830
--- /dev/null
+++ b/gopls/internal/regtest/misc/signature_help_test.go
@@ -0,0 +1,69 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package misc
+
+import (
+ "testing"
+
+ "github.com/google/go-cmp/cmp"
+ "golang.org/x/tools/gopls/internal/lsp/protocol"
+ . "golang.org/x/tools/gopls/internal/lsp/regtest"
+)
+
+func TestSignatureHelpInNonWorkspacePackage(t *testing.T) {
+ const files = `
+-- a/go.mod --
+module a.com
+
+go 1.18
+-- a/a/a.go --
+package a
+
+func DoSomething(int) {}
+
+func _() {
+ DoSomething()
+}
+-- b/go.mod --
+module b.com
+go 1.18
+
+require a.com v1.0.0
+
+replace a.com => ../a
+-- b/b/b.go --
+package b
+
+import "a.com/a"
+
+func _() {
+ a.DoSomething()
+}
+`
+
+ WithOptions(
+ WorkspaceFolders("a"),
+ ).Run(t, files, func(t *testing.T, env *Env) {
+ env.OpenFile("a/a/a.go")
+ env.OpenFile("b/b/b.go")
+ signatureHelp := func(filename string) *protocol.SignatureHelp {
+ pos := env.RegexpSearch(filename, `DoSomething\(()\)`)
+ var params protocol.SignatureHelpParams
+ params.Position = pos
+ params.TextDocument.URI = env.Sandbox.Workdir.URI(filename)
+ help, err := env.Editor.Server.SignatureHelp(env.Ctx, ¶ms)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return help
+ }
+ ahelp := signatureHelp("a/a/a.go")
+ bhelp := signatureHelp("b/b/b.go")
+
+ if diff := cmp.Diff(ahelp, bhelp); diff != "" {
+ t.Fatal(diff)
+ }
+ })
+}
