Skip to content

Commit ff4ff8b

Browse files
findleyrfindleyr
committed
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>
1 parent 2d32e15 commit ff4ff8b

13 files changed

+87
-83
lines changed

‎gopls/internal/lsp/cache/snapshot.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,7 @@ func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source
671671
return ph.await(ctx, s)
672672
}
673673

674-
func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]*packageHandle, error) {
674+
func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, withIntermediateTestVariants bool) ([]*packageHandle, error) {
675675
// TODO(rfindley): why can't/shouldn't we awaitLoaded here? It seems that if
676676
// we ask for package handles for a file, we should wait for pending loads.
677677
// Else we will reload orphaned files before the initial load completes.
@@ -695,7 +695,7 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode
695695
for _, id := range knownIDs {
696696
// Filter out any intermediate test variants. We typically aren't
697697
// interested in these packages for file= style queries.
698-
if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant && !includeTestVariants {
698+
if m := s.getMetadata(id); m != nil && m.IsIntermediateTestVariant && !withIntermediateTestVariants {
699699
continue
700700
}
701701
var parseModes []source.ParseMode

‎gopls/internal/lsp/source/completion/format.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,12 @@ import (
1313
"go/types"
1414
"strings"
1515

16-
"golang.org/x/tools/internal/event"
17-
"golang.org/x/tools/internal/imports"
18-
"golang.org/x/tools/internal/event/tag"
1916
"golang.org/x/tools/gopls/internal/lsp/protocol"
2017
"golang.org/x/tools/gopls/internal/lsp/snippet"
2118
"golang.org/x/tools/gopls/internal/lsp/source"
19+
"golang.org/x/tools/internal/event"
20+
"golang.org/x/tools/internal/event/tag"
21+
"golang.org/x/tools/internal/imports"
2222
"golang.org/x/tools/internal/span"
2323
"golang.org/x/tools/internal/typeparams"
2424
)
@@ -80,7 +80,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e
8080
if _, ok := obj.Type().(*types.Struct); ok {
8181
detail = "struct{...}" // for anonymous structs
8282
} else if obj.IsField() {
83-
detail = source.FormatVarType(ctx, c.snapshot, c.pkg, obj, c.qf)
83+
detail = source.FormatVarType(c.snapshot.FileSet(), c.pkg, obj, c.qf)
8484
}
8585
if obj.IsField() {
8686
kind = protocol.FieldCompletion
@@ -237,7 +237,7 @@ Suffixes:
237237
uri := span.URIFromPath(pos.Filename)
238238

239239
// Find the source file of the candidate.
240-
pkg, err := source.FindPackageFromPos(ctx, c.snapshot, obj.Pos())
240+
pkg, err := source.FindPackageFromPos(c.snapshot.FileSet(), c.pkg, obj.Pos())
241241
if err != nil {
242242
return item, nil
243243
}

‎gopls/internal/lsp/source/completion/literal.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ import (
1111
"strings"
1212
"unicode"
1313

14-
"golang.org/x/tools/internal/event"
1514
"golang.org/x/tools/gopls/internal/lsp/protocol"
1615
"golang.org/x/tools/gopls/internal/lsp/snippet"
1716
"golang.org/x/tools/gopls/internal/lsp/source"
17+
"golang.org/x/tools/internal/event"
1818
"golang.org/x/tools/internal/typeparams"
1919
)
2020

@@ -162,7 +162,7 @@ func (c *completer) literal(ctx context.Context, literalType types.Type, imp *im
162162
if score := c.matcher.Score("func"); !cand.hasMod(reference) && score > 0 && !source.IsInterface(expType) {
163163
switch t := literalType.Underlying().(type) {
164164
case *types.Signature:
165-
c.functionLiteral(ctx, t, float64(score))
165+
c.functionLiteral(t, float64(score))
166166
}
167167
}
168168
}
@@ -175,7 +175,7 @@ const literalCandidateScore = highScore / 2
175175

176176
// functionLiteral adds a function literal completion item for the
177177
// given signature.
178-
func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, matchScore float64) {
178+
func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
179179
snip := &snippet.Builder{}
180180
snip.WriteText("func(")
181181

@@ -202,7 +202,7 @@ func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, m
202202
// If the param has no name in the signature, guess a name based
203203
// on the type. Use an empty qualifier to ignore the package.
204204
// For example, we want to name "http.Request" "r", not "hr".
205-
name = source.FormatVarType(ctx, c.snapshot, c.pkg, p, func(p *types.Package) string {
205+
name = source.FormatVarType(c.snapshot.FileSet(), c.pkg, p, func(p *types.Package) string {
206206
return ""
207207
})
208208
name = abbreviateTypeName(name)
@@ -264,7 +264,7 @@ func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, m
264264
// of "i int, j int".
265265
if i == sig.Params().Len()-1 || !types.Identical(p.Type(), sig.Params().At(i+1).Type()) {
266266
snip.WriteText(" ")
267-
typeStr := source.FormatVarType(ctx, c.snapshot, c.pkg, p, c.qf)
267+
typeStr := source.FormatVarType(c.snapshot.FileSet(), c.pkg, p, c.qf)
268268
if sig.Variadic() && i == sig.Params().Len()-1 {
269269
typeStr = strings.Replace(typeStr, "[]", "...", 1)
270270
}
@@ -314,7 +314,7 @@ func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, m
314314
snip.WriteText(name + " ")
315315
}
316316

317-
text := source.FormatVarType(ctx, c.snapshot, c.pkg, r, c.qf)
317+
text := source.FormatVarType(c.snapshot.FileSet(), c.pkg, r, c.qf)
318318
if tp, _ := r.Type().(*typeparams.TypeParam); tp != nil && !c.typeParamInScope(tp) {
319319
snip.WritePlaceholder(func(snip *snippet.Builder) {
320320
snip.WriteText(text)

‎gopls/internal/lsp/source/highlight.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import (
1313
"strings"
1414

1515
"golang.org/x/tools/go/ast/astutil"
16-
"golang.org/x/tools/internal/event"
1716
"golang.org/x/tools/gopls/internal/lsp/protocol"
17+
"golang.org/x/tools/internal/event"
1818
)
1919

2020
func Highlight(ctx context.Context, snapshot Snapshot, fh FileHandle, position protocol.Position) ([]protocol.Range, error) {
@@ -59,7 +59,7 @@ func Highlight(ctx context.Context, snapshot Snapshot, fh FileHandle, position p
5959
}
6060
var ranges []protocol.Range
6161
for rng := range result {
62-
mRng, err := posToMappedRange(snapshot, pkg, rng.start, rng.end)
62+
mRng, err := posToMappedRange(snapshot.FileSet(), pkg, rng.start, rng.end)
6363
if err != nil {
6464
return nil, err
6565
}

‎gopls/internal/lsp/source/hover.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ import (
2222

2323
"golang.org/x/text/unicode/runenames"
2424
"golang.org/x/tools/go/types/typeutil"
25-
"golang.org/x/tools/internal/event"
26-
"golang.org/x/tools/internal/bug"
2725
"golang.org/x/tools/gopls/internal/lsp/protocol"
2826
"golang.org/x/tools/gopls/internal/lsp/safetoken"
27+
"golang.org/x/tools/internal/bug"
28+
"golang.org/x/tools/internal/event"
2929
"golang.org/x/tools/internal/typeparams"
3030
)
3131

@@ -237,7 +237,7 @@ func findRune(ctx context.Context, snapshot Snapshot, fh FileHandle, position pr
237237
return 0, MappedRange{}, ErrNoRuneFound
238238
}
239239

240-
mappedRange, err := posToMappedRange(snapshot, pkg, start, end)
240+
mappedRange, err := posToMappedRange(snapshot.FileSet(), pkg, start, end)
241241
if err != nil {
242242
return 0, MappedRange{}, err
243243
}

‎gopls/internal/lsp/source/identifier.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@ import (
1616
"strconv"
1717

1818
"golang.org/x/tools/go/ast/astutil"
19-
"golang.org/x/tools/internal/event"
20-
"golang.org/x/tools/internal/bug"
2119
"golang.org/x/tools/gopls/internal/lsp/protocol"
2220
"golang.org/x/tools/gopls/internal/lsp/safetoken"
21+
"golang.org/x/tools/internal/bug"
22+
"golang.org/x/tools/internal/event"
2323
"golang.org/x/tools/internal/span"
2424
"golang.org/x/tools/internal/typeparams"
2525
)
@@ -81,6 +81,8 @@ func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, position
8181
ctx, done := event.Start(ctx, "source.Identifier")
8282
defer done()
8383

84+
// TODO(rfindley): Why isn't this PackageForFile? A single package should
85+
// suffice to find identifier info.
8486
pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), TypecheckAll, false)
8587
if err != nil {
8688
return nil, err
@@ -141,7 +143,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa
141143
// Special case for package declarations, since they have no
142144
// corresponding types.Object.
143145
if ident == file.Name {
144-
rng, err := posToMappedRange(snapshot, pkg, file.Name.Pos(), file.Name.End())
146+
rng, err := posToMappedRange(snapshot.FileSet(), pkg, file.Name.Pos(), file.Name.End())
145147
if err != nil {
146148
return nil, err
147149
}
@@ -155,7 +157,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa
155157
if declAST == nil {
156158
declAST = file
157159
}
158-
declRng, err := posToMappedRange(snapshot, pkg, declAST.Name.Pos(), declAST.Name.End())
160+
declRng, err := posToMappedRange(snapshot.FileSet(), pkg, declAST.Name.Pos(), declAST.Name.End())
159161
if err != nil {
160162
return nil, err
161163
}
@@ -183,7 +185,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa
183185

184186
result.Name = result.ident.Name
185187
var err error
186-
if result.MappedRange, err = posToMappedRange(snapshot, pkg, result.ident.Pos(), result.ident.End()); err != nil {
188+
if result.MappedRange, err = posToMappedRange(snapshot.FileSet(), pkg, result.ident.Pos(), result.ident.End()); err != nil {
187189
return nil, err
188190
}
189191

@@ -282,13 +284,13 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa
282284
}
283285
}
284286

285-
rng, err := objToMappedRange(snapshot, pkg, result.Declaration.obj)
287+
rng, err := objToMappedRange(snapshot.FileSet(), pkg, result.Declaration.obj)
286288
if err != nil {
287289
return nil, err
288290
}
289291
result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng)
290292

291-
declPkg, err := FindPackageFromPos(ctx, snapshot, result.Declaration.obj.Pos())
293+
declPkg, err := FindPackageFromPos(snapshot.FileSet(), pkg, result.Declaration.obj.Pos())
292294
if err != nil {
293295
return nil, err
294296
}
@@ -312,7 +314,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa
312314
if hasErrorType(result.Type.Object) {
313315
return result, nil
314316
}
315-
if result.Type.MappedRange, err = objToMappedRange(snapshot, pkg, result.Type.Object); err != nil {
317+
if result.Type.MappedRange, err = objToMappedRange(snapshot.FileSet(), pkg, result.Type.Object); err != nil {
316318
return nil, err
317319
}
318320
}
@@ -480,7 +482,7 @@ func importSpec(snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (
480482
Name: importPath,
481483
pkg: pkg,
482484
}
483-
if result.MappedRange, err = posToMappedRange(snapshot, pkg, imp.Path.Pos(), imp.Path.End()); err != nil {
485+
if result.MappedRange, err = posToMappedRange(snapshot.FileSet(), pkg, imp.Path.Pos(), imp.Path.End()); err != nil {
484486
return nil, err
485487
}
486488
// Consider the "declaration" of an import spec to be the imported package.
@@ -490,7 +492,7 @@ func importSpec(snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (
490492
}
491493
// Return all of the files in the package as the definition of the import spec.
492494
for _, dst := range importedPkg.GetSyntax() {
493-
rng, err := posToMappedRange(snapshot, pkg, dst.Pos(), dst.End())
495+
rng, err := posToMappedRange(snapshot.FileSet(), pkg, dst.Pos(), dst.End())
494496
if err != nil {
495497
return nil, err
496498
}

‎gopls/internal/lsp/source/implementation.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func Implementation(ctx context.Context, snapshot Snapshot, f FileHandle, pp pro
3232
if impl.pkg == nil || len(impl.pkg.CompiledGoFiles()) == 0 {
3333
continue
3434
}
35-
rng, err := objToMappedRange(snapshot, impl.pkg, impl.obj)
35+
rng, err := objToMappedRange(snapshot.FileSet(), impl.pkg, impl.obj)
3636
if err != nil {
3737
return nil, err
3838
}

‎gopls/internal/lsp/source/references.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i
206206
continue
207207
}
208208
seen[key] = true
209-
rng, err := posToMappedRange(snapshot, pkg, ident.Pos(), ident.End())
209+
rng, err := posToMappedRange(snapshot.FileSet(), pkg, ident.Pos(), ident.End())
210210
if err != nil {
211211
return nil, err
212212
}

‎gopls/internal/lsp/source/rename.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot
117117
}
118118

119119
func computePrepareRenameResp(snapshot Snapshot, pkg Package, node ast.Node, text string) (*PrepareItem, error) {
120-
mr, err := posToMappedRange(snapshot, pkg, node.Pos(), node.End())
120+
mr, err := posToMappedRange(snapshot.FileSet(), pkg, node.Pos(), node.End())
121121
if err != nil {
122122
return nil, err
123123
}
@@ -163,11 +163,16 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position,
163163
}
164164

165165
if inPackageName {
166-
pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckAll, true)
166+
// Since we only take one package below, no need to include test variants.
167+
//
168+
// TODO(rfindley): but is this correct? What about x_test packages that
169+
// import the renaming package?
170+
const includeTestVariants = false
171+
pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckAll, includeTestVariants)
167172
if err != nil {
168173
return nil, true, err
169174
}
170-
var pkg Package
175+
var pkg Package // TODO(rfindley): we should consider all packages, so that we get the full reverse transitive closure.
171176
for _, p := range pkgs {
172177
// pgf.File.Name must not be nil, else this will panic.
173178
if pgf.File.Name.Name == p.Name() {

‎gopls/internal/lsp/source/signature_help.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ import (
1212
"go/types"
1313

1414
"golang.org/x/tools/go/ast/astutil"
15-
"golang.org/x/tools/internal/event"
1615
"golang.org/x/tools/gopls/internal/lsp/protocol"
16+
"golang.org/x/tools/internal/event"
1717
)
1818

1919
func SignatureHelp(ctx context.Context, snapshot Snapshot, fh FileHandle, position protocol.Position) (*protocol.SignatureInformation, int, error) {
@@ -94,7 +94,7 @@ FindCall:
9494
comment *ast.CommentGroup
9595
)
9696
if obj != nil {
97-
declPkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos())
97+
declPkg, err := FindPackageFromPos(snapshot.FileSet(), pkg, obj.Pos())
9898
if err != nil {
9999
return nil, 0, err
100100
}

‎gopls/internal/lsp/source/types_format.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ import (
1515
"go/types"
1616
"strings"
1717

18+
"golang.org/x/tools/gopls/internal/lsp/protocol"
1819
"golang.org/x/tools/internal/event"
1920
"golang.org/x/tools/internal/event/tag"
20-
"golang.org/x/tools/gopls/internal/lsp/protocol"
2121
"golang.org/x/tools/internal/typeparams"
2222
)
2323

@@ -205,7 +205,7 @@ func NewSignature(ctx context.Context, s Snapshot, pkg Package, sig *types.Signa
205205
params := make([]string, 0, sig.Params().Len())
206206
for i := 0; i < sig.Params().Len(); i++ {
207207
el := sig.Params().At(i)
208-
typ := FormatVarType(ctx, s, pkg, el, qf)
208+
typ := FormatVarType(s.FileSet(), pkg, el, qf)
209209
p := typ
210210
if el.Name() != "" {
211211
p = el.Name() + " " + typ
@@ -220,7 +220,7 @@ func NewSignature(ctx context.Context, s Snapshot, pkg Package, sig *types.Signa
220220
needResultParens = true
221221
}
222222
el := sig.Results().At(i)
223-
typ := FormatVarType(ctx, s, pkg, el, qf)
223+
typ := FormatVarType(s.FileSet(), pkg, el, qf)
224224
if el.Name() == "" {
225225
results = append(results, typ)
226226
} else {
@@ -253,8 +253,8 @@ func NewSignature(ctx context.Context, s Snapshot, pkg Package, sig *types.Signa
253253
// FormatVarType formats a *types.Var, accounting for type aliases.
254254
// To do this, it looks in the AST of the file in which the object is declared.
255255
// On any errors, it always falls back to types.TypeString.
256-
func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *types.Var, qf types.Qualifier) string {
257-
pkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos())
256+
func FormatVarType(fset *token.FileSet, srcpkg Package, obj *types.Var, qf types.Qualifier) string {
257+
pkg, err := FindPackageFromPos(fset, srcpkg, obj.Pos())
258258
if err != nil {
259259
return types.TypeString(obj.Type(), qf)
260260
}
@@ -283,7 +283,7 @@ func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *
283283
// If the request came from a different package than the one in which the
284284
// types are defined, we may need to modify the qualifiers.
285285
qualified = qualifyExpr(qualified, srcpkg, pkg, clonedInfo, qf)
286-
fmted := FormatNode(snapshot.FileSet(), qualified)
286+
fmted := FormatNode(fset, qualified)
287287
return fmted
288288
}
289289

0 commit comments

Comments
 (0)