gopls/internal/lsp/source: don't type-check in FindPackageFromPos
In all cases where we call FindPackageFromPos, we know that the given
position must be in the forward transitive closure of an originating
package. Refactor to use this information, potentially saving
significant type-checking when searching for a package.
As a result of this change, we never need to search intermediate test
variants when querying PackagesForFile.
Also replace snapshot arguments with token.FileSet arguments, when the
snapshot is only needed for its FileSet.
For golang/go#55293
Change-Id: Icf6236bea76ab5105a6bab24ce3afc574147882b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/438495
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index d1d2b1f..d13f56c 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -671,7 +671,7 @@
return ph.await(ctx, s)
}
-func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]*packageHandle, error) {
+func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, withIntermediateTestVariants bool) ([]*packageHandle, error) {
// TODO(rfindley): why can't/shouldn't we awaitLoaded here? It seems that if
// we ask for package handles for a file, we should wait for pending loads.
// Else we will reload orphaned files before the initial load completes.
@@ -695,7 +695,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 && !includeTestVariants {
+ if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant && !withIntermediateTestVariants {
continue
}
var parseModes []source.ParseMode
diff --git a/gopls/internal/lsp/source/completion/format.go b/gopls/internal/lsp/source/completion/format.go
index bcf523f..4a76eef 100644
--- a/gopls/internal/lsp/source/completion/format.go
+++ b/gopls/internal/lsp/source/completion/format.go
@@ -13,12 +13,12 @@
"go/types"
"strings"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/imports"
- "golang.org/x/tools/internal/event/tag"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/snippet"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
+ "golang.org/x/tools/internal/event/tag"
+ "golang.org/x/tools/internal/imports"
"golang.org/x/tools/internal/span"
"golang.org/x/tools/internal/typeparams"
)
@@ -80,7 +80,7 @@
if _, ok := obj.Type().(*types.Struct); ok {
detail = "struct{...}" // for anonymous structs
} else if obj.IsField() {
- detail = source.FormatVarType(ctx, c.snapshot, c.pkg, obj, c.qf)
+ detail = source.FormatVarType(c.snapshot.FileSet(), c.pkg, obj, c.qf)
}
if obj.IsField() {
kind = protocol.FieldCompletion
@@ -237,7 +237,7 @@
uri := span.URIFromPath(pos.Filename)
// Find the source file of the candidate.
- pkg, err := source.FindPackageFromPos(ctx, c.snapshot, obj.Pos())
+ pkg, err := source.FindPackageFromPos(c.snapshot.FileSet(), c.pkg, obj.Pos())
if err != nil {
return item, nil
}
diff --git a/gopls/internal/lsp/source/completion/literal.go b/gopls/internal/lsp/source/completion/literal.go
index 8f0e43c..0a9fc83 100644
--- a/gopls/internal/lsp/source/completion/literal.go
+++ b/gopls/internal/lsp/source/completion/literal.go
@@ -11,10 +11,10 @@
"strings"
"unicode"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/snippet"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/typeparams"
)
@@ -162,7 +162,7 @@
if score := c.matcher.Score("func"); !cand.hasMod(reference) && score > 0 && !source.IsInterface(expType) {
switch t := literalType.Underlying().(type) {
case *types.Signature:
- c.functionLiteral(ctx, t, float64(score))
+ c.functionLiteral(t, float64(score))
}
}
}
@@ -175,7 +175,7 @@
// functionLiteral adds a function literal completion item for the
// given signature.
-func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, matchScore float64) {
+func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
snip := &snippet.Builder{}
snip.WriteText("func(")
@@ -202,7 +202,7 @@
// If the param has no name in the signature, guess a name based
// on the type. Use an empty qualifier to ignore the package.
// For example, we want to name "http.Request" "r", not "hr".
- name = source.FormatVarType(ctx, c.snapshot, c.pkg, p, func(p *types.Package) string {
+ name = source.FormatVarType(c.snapshot.FileSet(), c.pkg, p, func(p *types.Package) string {
return ""
})
name = abbreviateTypeName(name)
@@ -264,7 +264,7 @@
// of "i int, j int".
if i == sig.Params().Len()-1 || !types.Identical(p.Type(), sig.Params().At(i+1).Type()) {
snip.WriteText(" ")
- typeStr := source.FormatVarType(ctx, c.snapshot, c.pkg, p, c.qf)
+ typeStr := source.FormatVarType(c.snapshot.FileSet(), c.pkg, p, c.qf)
if sig.Variadic() && i == sig.Params().Len()-1 {
typeStr = strings.Replace(typeStr, "[]", "...", 1)
}
@@ -314,7 +314,7 @@
snip.WriteText(name + " ")
}
- text := source.FormatVarType(ctx, c.snapshot, c.pkg, r, c.qf)
+ text := source.FormatVarType(c.snapshot.FileSet(), c.pkg, r, c.qf)
if tp, _ := r.Type().(*typeparams.TypeParam); tp != nil && !c.typeParamInScope(tp) {
snip.WritePlaceholder(func(snip *snippet.Builder) {
snip.WriteText(text)
diff --git a/gopls/internal/lsp/source/highlight.go b/gopls/internal/lsp/source/highlight.go
index 0cde628..5ec71d0 100644
--- a/gopls/internal/lsp/source/highlight.go
+++ b/gopls/internal/lsp/source/highlight.go
@@ -13,8 +13,8 @@
"strings"
"golang.org/x/tools/go/ast/astutil"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
+ "golang.org/x/tools/internal/event"
)
func Highlight(ctx context.Context, snapshot Snapshot, fh FileHandle, position protocol.Position) ([]protocol.Range, error) {
@@ -59,7 +59,7 @@
}
var ranges []protocol.Range
for rng := range result {
- mRng, err := posToMappedRange(snapshot, pkg, rng.start, rng.end)
+ mRng, err := posToMappedRange(snapshot.FileSet(), pkg, rng.start, rng.end)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go
index 74b7e57..0dc8d8a 100644
--- a/gopls/internal/lsp/source/hover.go
+++ b/gopls/internal/lsp/source/hover.go
@@ -22,10 +22,10 @@
"golang.org/x/text/unicode/runenames"
"golang.org/x/tools/go/types/typeutil"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/bug"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/safetoken"
+ "golang.org/x/tools/internal/bug"
+ "golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/typeparams"
)
@@ -237,7 +237,7 @@
return 0, MappedRange{}, ErrNoRuneFound
}
- mappedRange, err := posToMappedRange(snapshot, pkg, start, end)
+ mappedRange, err := posToMappedRange(snapshot.FileSet(), pkg, start, end)
if err != nil {
return 0, MappedRange{}, err
}
diff --git a/gopls/internal/lsp/source/identifier.go b/gopls/internal/lsp/source/identifier.go
index 9ab3fe7..7e856b5 100644
--- a/gopls/internal/lsp/source/identifier.go
+++ b/gopls/internal/lsp/source/identifier.go
@@ -16,10 +16,10 @@
"strconv"
"golang.org/x/tools/go/ast/astutil"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/bug"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/safetoken"
+ "golang.org/x/tools/internal/bug"
+ "golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/span"
"golang.org/x/tools/internal/typeparams"
)
@@ -81,6 +81,8 @@
ctx, done := event.Start(ctx, "source.Identifier")
defer done()
+ // TODO(rfindley): Why isn't this PackageForFile? A single package should
+ // suffice to find identifier info.
pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), TypecheckAll, false)
if err != nil {
return nil, err
@@ -141,7 +143,7 @@
// Special case for package declarations, since they have no
// corresponding types.Object.
if ident == file.Name {
- rng, err := posToMappedRange(snapshot, pkg, file.Name.Pos(), file.Name.End())
+ rng, err := posToMappedRange(snapshot.FileSet(), pkg, file.Name.Pos(), file.Name.End())
if err != nil {
return nil, err
}
@@ -155,7 +157,7 @@
if declAST == nil {
declAST = file
}
- declRng, err := posToMappedRange(snapshot, pkg, declAST.Name.Pos(), declAST.Name.End())
+ declRng, err := posToMappedRange(snapshot.FileSet(), pkg, declAST.Name.Pos(), declAST.Name.End())
if err != nil {
return nil, err
}
@@ -183,7 +185,7 @@
result.Name = result.ident.Name
var err error
- if result.MappedRange, err = posToMappedRange(snapshot, pkg, result.ident.Pos(), result.ident.End()); err != nil {
+ if result.MappedRange, err = posToMappedRange(snapshot.FileSet(), pkg, result.ident.Pos(), result.ident.End()); err != nil {
return nil, err
}
@@ -282,13 +284,13 @@
}
}
- rng, err := objToMappedRange(snapshot, pkg, result.Declaration.obj)
+ rng, err := objToMappedRange(snapshot.FileSet(), pkg, result.Declaration.obj)
if err != nil {
return nil, err
}
result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng)
- declPkg, err := FindPackageFromPos(ctx, snapshot, result.Declaration.obj.Pos())
+ declPkg, err := FindPackageFromPos(snapshot.FileSet(), pkg, result.Declaration.obj.Pos())
if err != nil {
return nil, err
}
@@ -312,7 +314,7 @@
if hasErrorType(result.Type.Object) {
return result, nil
}
- if result.Type.MappedRange, err = objToMappedRange(snapshot, pkg, result.Type.Object); err != nil {
+ if result.Type.MappedRange, err = objToMappedRange(snapshot.FileSet(), pkg, result.Type.Object); err != nil {
return nil, err
}
}
@@ -480,7 +482,7 @@
Name: importPath,
pkg: pkg,
}
- if result.MappedRange, err = posToMappedRange(snapshot, pkg, imp.Path.Pos(), imp.Path.End()); err != nil {
+ if result.MappedRange, err = posToMappedRange(snapshot.FileSet(), pkg, imp.Path.Pos(), imp.Path.End()); err != nil {
return nil, err
}
// Consider the "declaration" of an import spec to be the imported package.
@@ -490,7 +492,7 @@
}
// Return all of the files in the package as the definition of the import spec.
for _, dst := range importedPkg.GetSyntax() {
- rng, err := posToMappedRange(snapshot, pkg, dst.Pos(), dst.End())
+ rng, err := posToMappedRange(snapshot.FileSet(), pkg, dst.Pos(), dst.End())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go
index 4626bae..3533c92 100644
--- a/gopls/internal/lsp/source/implementation.go
+++ b/gopls/internal/lsp/source/implementation.go
@@ -32,7 +32,7 @@
if impl.pkg == nil || len(impl.pkg.CompiledGoFiles()) == 0 {
continue
}
- rng, err := objToMappedRange(snapshot, impl.pkg, impl.obj)
+ rng, err := objToMappedRange(snapshot.FileSet(), impl.pkg, impl.obj)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go
index d1cf1ea..714a3cb 100644
--- a/gopls/internal/lsp/source/references.go
+++ b/gopls/internal/lsp/source/references.go
@@ -206,7 +206,7 @@
continue
}
seen[key] = true
- rng, err := posToMappedRange(snapshot, pkg, ident.Pos(), ident.End())
+ rng, err := posToMappedRange(snapshot.FileSet(), pkg, ident.Pos(), ident.End())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go
index d0225b5..93ded0f 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -117,7 +117,7 @@
}
func computePrepareRenameResp(snapshot Snapshot, pkg Package, node ast.Node, text string) (*PrepareItem, error) {
- mr, err := posToMappedRange(snapshot, pkg, node.Pos(), node.End())
+ mr, err := posToMappedRange(snapshot.FileSet(), pkg, node.Pos(), node.End())
if err != nil {
return nil, err
}
@@ -163,11 +163,16 @@
}
if inPackageName {
- pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckAll, true)
+ // Since we only take one package below, no need to include test variants.
+ //
+ // 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)
if err != nil {
return nil, true, err
}
- var pkg Package
+ var pkg Package // TODO(rfindley): we should consider all packages, so that we get the full reverse transitive closure.
for _, p := range pkgs {
// pgf.File.Name must not be nil, else this will panic.
if pgf.File.Name.Name == p.Name() {
diff --git a/gopls/internal/lsp/source/signature_help.go b/gopls/internal/lsp/source/signature_help.go
index f1a4bac..68ac1be 100644
--- a/gopls/internal/lsp/source/signature_help.go
+++ b/gopls/internal/lsp/source/signature_help.go
@@ -12,8 +12,8 @@
"go/types"
"golang.org/x/tools/go/ast/astutil"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
+ "golang.org/x/tools/internal/event"
)
func SignatureHelp(ctx context.Context, snapshot Snapshot, fh FileHandle, position protocol.Position) (*protocol.SignatureInformation, int, error) {
@@ -94,7 +94,7 @@
comment *ast.CommentGroup
)
if obj != nil {
- declPkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos())
+ declPkg, err := FindPackageFromPos(snapshot.FileSet(), pkg, obj.Pos())
if err != nil {
return nil, 0, err
}
diff --git a/gopls/internal/lsp/source/types_format.go b/gopls/internal/lsp/source/types_format.go
index 036c8c7..f1c03ce 100644
--- a/gopls/internal/lsp/source/types_format.go
+++ b/gopls/internal/lsp/source/types_format.go
@@ -15,9 +15,9 @@
"go/types"
"strings"
+ "golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/event/tag"
- "golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/internal/typeparams"
)
@@ -205,7 +205,7 @@
params := make([]string, 0, sig.Params().Len())
for i := 0; i < sig.Params().Len(); i++ {
el := sig.Params().At(i)
- typ := FormatVarType(ctx, s, pkg, el, qf)
+ typ := FormatVarType(s.FileSet(), pkg, el, qf)
p := typ
if el.Name() != "" {
p = el.Name() + " " + typ
@@ -220,7 +220,7 @@
needResultParens = true
}
el := sig.Results().At(i)
- typ := FormatVarType(ctx, s, pkg, el, qf)
+ typ := FormatVarType(s.FileSet(), pkg, el, qf)
if el.Name() == "" {
results = append(results, typ)
} else {
@@ -253,8 +253,8 @@
// FormatVarType formats a *types.Var, accounting for type aliases.
// To do this, it looks in the AST of the file in which the object is declared.
// On any errors, it always falls back to types.TypeString.
-func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *types.Var, qf types.Qualifier) string {
- pkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos())
+func FormatVarType(fset *token.FileSet, srcpkg Package, obj *types.Var, qf types.Qualifier) string {
+ pkg, err := FindPackageFromPos(fset, srcpkg, obj.Pos())
if err != nil {
return types.TypeString(obj.Type(), qf)
}
@@ -283,7 +283,7 @@
// If the request came from a different package than the one in which the
// types are defined, we may need to modify the qualifiers.
qualified = qualifyExpr(qualified, srcpkg, pkg, clonedInfo, qf)
- fmted := FormatNode(snapshot.FileSet(), qualified)
+ fmted := FormatNode(fset, qualified)
return fmted
}
diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go
index d1c90b6..9939ca0 100644
--- a/gopls/internal/lsp/source/util.go
+++ b/gopls/internal/lsp/source/util.go
@@ -123,7 +123,7 @@
return false
}
-func objToMappedRange(snapshot Snapshot, pkg Package, obj types.Object) (MappedRange, error) {
+func objToMappedRange(fset *token.FileSet, pkg Package, obj types.Object) (MappedRange, error) {
nameLen := len(obj.Name())
if pkgName, ok := obj.(*types.PkgName); ok {
// An imported Go package has a package-local, unqualified name.
@@ -140,13 +140,20 @@
nameLen = len(pkgName.Imported().Path()) + len(`""`)
}
}
- return posToMappedRange(snapshot, pkg, obj.Pos(), obj.Pos()+token.Pos(nameLen))
+ return posToMappedRange(fset, pkg, obj.Pos(), obj.Pos()+token.Pos(nameLen))
}
// posToMappedRange returns the MappedRange for the given [start, end) span,
// which must be among the transitive dependencies of pkg.
-func posToMappedRange(snapshot Snapshot, pkg Package, pos, end token.Pos) (MappedRange, error) {
- tokFile := snapshot.FileSet().File(pos)
+func posToMappedRange(fset *token.FileSet, pkg Package, pos, end token.Pos) (MappedRange, error) {
+ if !pos.IsValid() {
+ return MappedRange{}, fmt.Errorf("invalid start position")
+ }
+ if !end.IsValid() {
+ return MappedRange{}, fmt.Errorf("invalid end position")
+ }
+
+ tokFile := fset.File(pos)
// Subtle: it is not safe to simplify this to tokFile.Name
// because, due to //line directives, a Position within a
// token.File may have a different filename than the File itself.
@@ -155,19 +162,35 @@
if err != nil {
return MappedRange{}, err
}
- if !pos.IsValid() {
- return MappedRange{}, fmt.Errorf("invalid start position")
- }
- if !end.IsValid() {
- return MappedRange{}, fmt.Errorf("invalid end position")
- }
- // It is fishy that pgf.Mapper (from the parsed Go file) is
+ // It is problematic that pgf.Mapper (from the parsed Go file) is
// accompanied here not by pgf.Tok but by tokFile from the global
// FileSet, which is a distinct token.File that doesn't
- // contain [pos,end). TODO(adonovan): clean this up.
+ // contain [pos,end).
+ //
+ // This is done because tokFile is the *token.File for the compiled go file
+ // containing pos, whereas Mapper is the UTF16 mapper for the go file pointed
+ // to by line directives.
+ //
+ // TODO(golang/go#55043): clean this up.
return NewMappedRange(tokFile, pgf.Mapper, pos, end), nil
}
+// FindPackageFromPos returns the Package for the given position, which must be
+// among the transitive dependencies of pkg.
+//
+// TODO(rfindley): is this the best factoring of this API? This function is
+// really a trivial wrapper around findFileInDeps, which may be a more useful
+// function to expose.
+func FindPackageFromPos(fset *token.FileSet, pkg Package, pos token.Pos) (Package, error) {
+ if !pos.IsValid() {
+ return nil, fmt.Errorf("invalid position")
+ }
+ fileName := fset.File(pos).Name()
+ uri := span.URIFromPath(fileName)
+ _, pkg, err := findFileInDeps(pkg, uri)
+ return pkg, err
+}
+
// Matches cgo generated comment as well as the proposed standard:
//
// https://golang.org/s/generatedcode
@@ -286,35 +309,6 @@
return 0
}
-// FindPackageFromPos finds the first package containing pos in its
-// type-checked AST.
-func FindPackageFromPos(ctx context.Context, snapshot Snapshot, pos token.Pos) (Package, error) {
- tok := snapshot.FileSet().File(pos)
- if tok == nil {
- return nil, fmt.Errorf("no file for pos %v", pos)
- }
- uri := span.URIFromPath(tok.Name())
- pkgs, err := snapshot.PackagesForFile(ctx, uri, TypecheckAll, true)
- if err != nil {
- return nil, err
- }
- // Only return the package if it actually type-checked the given position.
- for _, pkg := range pkgs {
- parsed, err := pkg.File(uri)
- if err != nil {
- // TODO(adonovan): should this be a bug.Report or log.Fatal?
- // The logic in Identifier seems to think so.
- // Should it be a postcondition of PackagesForFile?
- // And perhaps PackagesForFile should return the PGFs too.
- return nil, err
- }
- if parsed != nil && parsed.Tok.Base() == tok.Base() {
- return pkg, nil
- }
- }
- return nil, fmt.Errorf("no package for given file position")
-}
-
// findFileInDeps finds uri in pkg or its dependencies.
func findFileInDeps(pkg Package, uri span.URI) (*ParsedGoFile, Package, error) {
queue := []Package{pkg}
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 8b1dbdb..fa9d6a9 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -138,7 +138,10 @@
// PackagesForFile returns an unordered list of packages that contain
// the file denoted by uri, type checked in the specified mode.
- PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode, includeTestVariants bool) ([]Package, error)
+ //
+ // If withIntermediateTestVariants is set, the resulting package set includes
+ // intermediate test variants.
+ PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode, withIntermediateTestVariants bool) ([]Package, error)
// PackageForFile returns a single package that this file belongs to,
// checked in mode and filtered by the package policy.
