From f505e54dbd80b113163938b13fe5e5f6e952dac2 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 1 Nov 2019 17:59:28 -0400 Subject: [PATCH] internal/lsp: use available type info for unimported completions Packages that aren't imported in the current file will often have been used elsewhere, which means that gopls will have their type information available. Expose loaded packages in the Snapshot, and try to use that information when possible for unimported packages. Change-Id: Icb672618a9f9ec31b9796f0c5da56ed3d2b38aa7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/204824 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/snapshot.go | 24 +++++++++++++++++++ internal/lsp/source/completion.go | 8 +++++++ internal/lsp/source/view.go | 4 ++++ internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/unimported/unimported.go.in | 10 +++++--- .../unimported/unimported_cand_type.go | 5 +++- 6 files changed, 48 insertions(+), 5 deletions(-) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index b02faac9124..f409d73e1fc 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -98,6 +98,30 @@ func (s *snapshot) getPackages(uri span.URI, m source.ParseMode) (cphs []source. return cphs } +func (s *snapshot) KnownImportPaths() map[string]source.Package { + s.mu.Lock() + defer s.mu.Unlock() + + results := map[string]source.Package{} + for _, cph := range s.packages { + cachedPkg, err := cph.cached() + if err != nil { + continue + } + for importPath, newPkg := range cachedPkg.imports { + if oldPkg, ok := results[string(importPath)]; ok { + // Using the same trick as NarrowestPackageHandle, prefer non-variants. + if len(newPkg.files) < len(oldPkg.(*pkg).files) { + results[string(importPath)] = newPkg + } + } else { + results[string(importPath)] = newPkg + } + } + } + return results +} + func (s *snapshot) getPackage(id packageID, m source.ParseMode) *checkPackageHandle { s.mu.Lock() defer s.mu.Unlock() diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 905043d814a..70be745229f 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -587,7 +587,15 @@ func (c *completer) selector(sel *ast.SelectorExpr) error { if err != nil { return err } + known := c.snapshot.KnownImportPaths() for _, pkgExport := range pkgExports { + // If we've seen this import path, use the fully-typed version. + if knownPkg, ok := known[pkgExport.Fix.StmtInfo.ImportPath]; ok { + c.packageMembers(knownPkg.GetTypes(), &pkgExport.Fix.StmtInfo) + continue + } + + // Otherwise, continue with untyped proposals. pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName) for _, export := range pkgExport.Exports { c.found(types.NewVar(0, pkg, export, nil), 0.07, &pkgExport.Fix.StmtInfo) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 8935f334b06..747c9232ebe 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -275,6 +275,10 @@ type Snapshot interface { // CheckPackageHandles returns the CheckPackageHandles for the packages // that this file belongs to. CheckPackageHandles(ctx context.Context, f File) ([]CheckPackageHandle, error) + + // KnownImportPaths returns all the packages loaded in this snapshot, + // indexed by their import path. + KnownImportPaths() map[string]Package } // File represents a source file of any type. diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 1b64a5da4d5..51af997c103 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -1,7 +1,7 @@ -- summary -- CompletionsCount = 213 CompletionSnippetCount = 40 -UnimportedCompletionsCount = 2 +UnimportedCompletionsCount = 3 DeepCompletionsCount = 5 FuzzyCompletionsCount = 7 RankedCompletionsCount = 8 diff --git a/internal/lsp/testdata/unimported/unimported.go.in b/internal/lsp/testdata/unimported/unimported.go.in index c721b314ea5..3b78f7274a3 100644 --- a/internal/lsp/testdata/unimported/unimported.go.in +++ b/internal/lsp/testdata/unimported/unimported.go.in @@ -2,7 +2,9 @@ package unimported func _() { //@unimported("", bytes, context, cryptoslashrand, time, unsafe, externalpackage) - bytes. //@unimported(" /", bytesbuffer) + // container/ring is extremely unlikely to be imported by anything, so shouldn't have type information. + ring.Ring //@unimported("Ring", ringring) + signature.Foo //@unimported("Foo", signaturefoo) } // Create markers for unimported std lib packages. Only for use by this test. @@ -11,6 +13,8 @@ func _() { /* rand */ //@item(cryptoslashrand, "rand", "\"crypto/rand\"", "package") /* time */ //@item(time, "time", "\"time\"", "package") /* unsafe */ //@item(unsafe, "unsafe", "\"unsafe\"", "package") -/* pkg */ //@item(externalpackage, "pkg", "\"example.com/extramodule/pkg\"", "package" ) +/* pkg */ //@item(externalpackage, "pkg", "\"example.com/extramodule/pkg\"", "package") -/* bytes.Buffer */ //@item(bytesbuffer, "Buffer", "(from \"bytes\")", "var" ) \ No newline at end of file +/* ring.Ring */ //@item(ringring, "Ring", "(from \"container/ring\")", "var") + +/* signature.Foo */ //@item(signaturefoo, "Foo", "func(a string, b int) (c bool) (from \"golang.org/x/tools/internal/lsp/signature\")", "func") \ No newline at end of file diff --git a/internal/lsp/testdata/unimported/unimported_cand_type.go b/internal/lsp/testdata/unimported/unimported_cand_type.go index 765ab19075e..2690c1a4a29 100644 --- a/internal/lsp/testdata/unimported/unimported_cand_type.go +++ b/internal/lsp/testdata/unimported/unimported_cand_type.go @@ -1,6 +1,9 @@ package unimported -import "golang.org/x/tools/internal/lsp/baz" +import ( + "golang.org/x/tools/internal/lsp/baz" + "golang.org/x/tools/internal/lsp/signature" // provide type information for unimported completions in the other file +) func _() { foo.StructFoo{} //@item(litFooStructFoo, "foo.StructFoo{}", "struct{...}", "struct")