internal/imports,lsp: use callbacks for completion functions
We only need to return a relatively small number of completions to the
user. There's no point continuing once we have those, so switch the
completion functions to be callback-based, and cancel once we've got
what we want.
Change-Id: Ied199fb1f41346819c7237dfed8251fa3ac73ad7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212634
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index 7ae035f..07536d2 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -82,7 +82,7 @@
IdentName string
// FixType is the type of fix this is (AddImport, DeleteImport, SetImportName).
FixType ImportFixType
- relevance int // see pkg
+ Relevance int // see pkg
}
// An ImportInfo represents a single import statement.
@@ -585,6 +585,10 @@
return fixes, nil
}
+// Highest relevance, used for the standard library. Chosen arbitrarily to
+// match pre-existing gopls code.
+const MaxRelevance = 7
+
// getCandidatePkgs returns the list of pkgs that are accessible from filename,
// filtered to those that match pkgnameFilter.
func getCandidatePkgs(ctx context.Context, wrappedCallback *scanCallback, filename string, env *ProcessEnv) error {
@@ -596,7 +600,7 @@
dir: filepath.Join(env.GOROOT, "src", importPath),
importPathShort: importPath,
packageName: path.Base(importPath),
- relevance: 0,
+ relevance: MaxRelevance,
}
if wrappedCallback.packageNameLoaded(p) {
wrappedCallback.exportsLoaded(p, exports)
@@ -630,17 +634,6 @@
return env.GetResolver().scan(ctx, scanFilter, exclude)
}
-// Compare first by relevance, then by package name, with import path as a tiebreaker.
-func compareFix(fi, fj *ImportFix) bool {
- if fi.relevance != fj.relevance {
- return fi.relevance < fj.relevance
- }
- if fi.IdentName != fj.IdentName {
- return fi.IdentName < fj.IdentName
- }
- return fi.StmtInfo.ImportPath < fj.StmtInfo.ImportPath
-}
-
func candidateImportName(pkg *pkg) string {
if ImportPathToAssumedName(pkg.importPathShort) != pkg.packageName {
return pkg.packageName
@@ -649,39 +642,32 @@
}
// getAllCandidates gets all of the candidates to be imported, regardless of if they are needed.
-func getAllCandidates(ctx context.Context, prefix string, filename string, env *ProcessEnv) ([]ImportFix, error) {
- var mu sync.Mutex
- var results []ImportFix
- filter := &scanCallback{
+func getAllCandidates(ctx context.Context, wrapped func(ImportFix), prefix string, filename string, env *ProcessEnv) error {
+ callback := &scanCallback{
dirFound: func(pkg *pkg) bool {
// TODO(heschi): apply dir match heuristics like pkgIsCandidate
return true
},
packageNameLoaded: func(pkg *pkg) bool {
if strings.HasPrefix(pkg.packageName, prefix) {
- mu.Lock()
- defer mu.Unlock()
- results = append(results, ImportFix{
+ wrapped(ImportFix{
StmtInfo: ImportInfo{
ImportPath: pkg.importPathShort,
Name: candidateImportName(pkg),
},
IdentName: pkg.packageName,
FixType: AddImport,
- relevance: pkg.relevance,
+ Relevance: pkg.relevance,
})
}
return false
},
}
- err := getCandidatePkgs(ctx, filter, filename, env)
+ err := getCandidatePkgs(ctx, callback, filename, env)
if err != nil {
- return nil, err
+ return err
}
- sort.Slice(results, func(i, j int) bool {
- return compareFix(&results[i], &results[j])
- })
- return results, nil
+ return nil
}
// A PackageExport is a package and its exports.
@@ -690,9 +676,7 @@
Exports []string
}
-func getPackageExports(ctx context.Context, completePackage, filename string, env *ProcessEnv) ([]PackageExport, error) {
- var mu sync.Mutex
- var results []PackageExport
+func getPackageExports(ctx context.Context, wrapped func(PackageExport), completePackage, filename string, env *ProcessEnv) error {
callback := &scanCallback{
dirFound: func(pkg *pkg) bool {
// TODO(heschi): apply dir match heuristics like pkgIsCandidate
@@ -702,10 +686,8 @@
return pkg.packageName == completePackage
},
exportsLoaded: func(pkg *pkg, exports []string) {
- mu.Lock()
- defer mu.Unlock()
sort.Strings(exports)
- results = append(results, PackageExport{
+ wrapped(PackageExport{
Fix: &ImportFix{
StmtInfo: ImportInfo{
ImportPath: pkg.importPathShort,
@@ -713,7 +695,7 @@
},
IdentName: pkg.packageName,
FixType: AddImport,
- relevance: pkg.relevance,
+ Relevance: pkg.relevance,
},
Exports: exports,
})
@@ -721,12 +703,9 @@
}
err := getCandidatePkgs(ctx, callback, filename, env)
if err != nil {
- return nil, err
+ return err
}
- sort.Slice(results, func(i, j int) bool {
- return compareFix(results[i].Fix, results[j].Fix)
- })
- return results, nil
+ return nil
}
// ProcessEnv contains environment variables and settings that affect the use of
@@ -1175,10 +1154,10 @@
p := &pkg{
importPathShort: info.nonCanonicalImportPath,
dir: dir,
- relevance: 1,
+ relevance: MaxRelevance - 1,
}
if info.rootType == gopathwalk.RootGOROOT {
- p.relevance = 0
+ p.relevance = MaxRelevance
}
if callback.dirFound(p) {
diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go
index b48a69a..9e1dff8 100644
--- a/internal/imports/fix_test.go
+++ b/internal/imports/fix_test.go
@@ -13,6 +13,7 @@
"path/filepath"
"reflect"
"runtime"
+ "sort"
"strings"
"sync"
"testing"
@@ -2492,15 +2493,15 @@
// with correct priorities.
func TestGetCandidates(t *testing.T) {
type res struct {
+ relevance int
name, path string
}
want := []res{
- {"bytes", "bytes"},
- {"http", "net/http"},
- {"rand", "crypto/rand"},
- {"rand", "math/rand"},
- {"bar", "bar.com/bar"},
- {"foo", "foo.com/foo"},
+ {0, "bytes", "bytes"},
+ {0, "http", "net/http"},
+ {0, "rand", "crypto/rand"},
+ {0, "bar", "bar.com/bar"},
+ {0, "foo", "foo.com/foo"},
}
testConfig{
@@ -2515,18 +2516,31 @@
},
},
}.test(t, func(t *goimportTest) {
- candidates, err := getAllCandidates(context.Background(), "", "x.go", t.env)
- if err != nil {
- t.Fatalf("GetAllCandidates() = %v", err)
- }
+ var mu sync.Mutex
var got []res
- for _, c := range candidates {
+ add := func(c ImportFix) {
+ mu.Lock()
+ defer mu.Unlock()
for _, w := range want {
if c.StmtInfo.ImportPath == w.path {
- got = append(got, res{c.IdentName, c.StmtInfo.ImportPath})
+ got = append(got, res{c.Relevance, c.IdentName, c.StmtInfo.ImportPath})
}
}
}
+ if err := getAllCandidates(context.Background(), add, "", "x.go", t.env); err != nil {
+ t.Fatalf("GetAllCandidates() = %v", err)
+ }
+ // Sort, then clear out relevance so it doesn't mess up the DeepEqual.
+ sort.Slice(got, func(i, j int) bool {
+ ri, rj := got[i], got[j]
+ if ri.relevance != rj.relevance {
+ return ri.relevance > rj.relevance // Highest first.
+ }
+ return ri.name < rj.name
+ })
+ for i := range got {
+ got[i].relevance = 0
+ }
if !reflect.DeepEqual(want, got) {
t.Errorf("wanted stdlib results in order %v, got %v", want, got)
}
@@ -2535,12 +2549,12 @@
func TestGetPackageCompletions(t *testing.T) {
type res struct {
+ relevance int
name, path, symbol string
}
want := []res{
- {"rand", "crypto/rand", "Prime"},
- {"rand", "math/rand", "Seed"},
- {"rand", "bar.com/rand", "Bar"},
+ {0, "rand", "math/rand", "Seed"},
+ {0, "rand", "bar.com/rand", "Bar"},
}
testConfig{
@@ -2551,20 +2565,33 @@
},
},
}.test(t, func(t *goimportTest) {
- candidates, err := getPackageExports(context.Background(), "rand", "x.go", t.env)
- if err != nil {
- t.Fatalf("getPackageCompletions() = %v", err)
- }
+ var mu sync.Mutex
var got []res
- for _, c := range candidates {
+ add := func(c PackageExport) {
+ mu.Lock()
+ defer mu.Unlock()
for _, csym := range c.Exports {
for _, w := range want {
if c.Fix.StmtInfo.ImportPath == w.path && csym == w.symbol {
- got = append(got, res{c.Fix.IdentName, c.Fix.StmtInfo.ImportPath, csym})
+ got = append(got, res{c.Fix.Relevance, c.Fix.IdentName, c.Fix.StmtInfo.ImportPath, csym})
}
}
}
}
+ if err := getPackageExports(context.Background(), add, "rand", "x.go", t.env); err != nil {
+ t.Fatalf("getPackageCompletions() = %v", err)
+ }
+ // Sort, then clear out relevance so it doesn't mess up the DeepEqual.
+ sort.Slice(got, func(i, j int) bool {
+ ri, rj := got[i], got[j]
+ if ri.relevance != rj.relevance {
+ return ri.relevance > rj.relevance // Highest first.
+ }
+ return ri.name < rj.name
+ })
+ for i := range got {
+ got[i].relevance = 0
+ }
if !reflect.DeepEqual(want, got) {
t.Errorf("wanted stdlib results in order %v, got %v", want, got)
}
diff --git a/internal/imports/imports.go b/internal/imports/imports.go
index c857043..3855c8a 100644
--- a/internal/imports/imports.go
+++ b/internal/imports/imports.go
@@ -118,21 +118,21 @@
// GetAllCandidates gets all of the packages starting with prefix that can be
// imported by filename, sorted by import path.
-func GetAllCandidates(ctx context.Context, prefix string, filename string, opt *Options) (pkgs []ImportFix, err error) {
- _, opt, err = initialize(filename, nil, opt)
+func GetAllCandidates(ctx context.Context, callback func(ImportFix), prefix string, filename string, opt *Options) error {
+ _, opt, err := initialize(filename, nil, opt)
if err != nil {
- return nil, err
+ return err
}
- return getAllCandidates(ctx, prefix, filename, opt.Env)
+ return getAllCandidates(ctx, callback, prefix, filename, opt.Env)
}
// GetPackageExports returns all known packages with name pkg and their exports.
-func GetPackageExports(ctx context.Context, pkg, filename string, opt *Options) (exports []PackageExport, err error) {
- _, opt, err = initialize(filename, nil, opt)
+func GetPackageExports(ctx context.Context, callback func(PackageExport), pkg, filename string, opt *Options) error {
+ _, opt, err := initialize(filename, nil, opt)
if err != nil {
- return nil, err
+ return err
}
- return getPackageExports(ctx, pkg, filename, opt.Env)
+ return getPackageExports(ctx, callback, pkg, filename, opt.Env)
}
// initialize sets the values for opt and src.
diff --git a/internal/imports/mod.go b/internal/imports/mod.go
index 2632937..fb665a3 100644
--- a/internal/imports/mod.go
+++ b/internal/imports/mod.go
@@ -425,18 +425,18 @@
importPathShort: info.nonCanonicalImportPath,
dir: info.dir,
packageName: path.Base(info.nonCanonicalImportPath),
- relevance: 0,
+ relevance: MaxRelevance,
}, nil
}
importPath := info.nonCanonicalImportPath
- relevance := 3
+ relevance := MaxRelevance - 3
// Check if the directory is underneath a module that's in scope.
if mod := r.findModuleByDir(info.dir); mod != nil {
if mod.Indirect {
- relevance = 2
+ relevance = MaxRelevance - 2
} else {
- relevance = 1
+ relevance = MaxRelevance - 1
}
// It is. If dir is the target of a replace directive,
// our guessed import path is wrong. Use the real one.
diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go
index 61a6beb..e98f222 100644
--- a/internal/imports/mod_test.go
+++ b/internal/imports/mod_test.go
@@ -13,6 +13,7 @@
"path/filepath"
"reflect"
"regexp"
+ "sort"
"strings"
"sync"
"testing"
@@ -871,32 +872,42 @@
}
type res struct {
+ relevance int
name, path string
}
want := []res{
// Stdlib
- {"bytes", "bytes"},
- {"http", "net/http"},
+ {7, "bytes", "bytes"},
+ {7, "http", "net/http"},
// Direct module deps
- {"quote", "rsc.io/quote"},
+ {6, "quote", "rsc.io/quote"},
+ {6, "rpackage", "example.com/rpackage"},
// Indirect deps
- {"rpackage", "example.com/rpackage"},
- {"language", "golang.org/x/text/language"},
+ {5, "language", "golang.org/x/text/language"},
// Out of scope modules
- {"quote", "rsc.io/quote/v2"},
+ {4, "quote", "rsc.io/quote/v2"},
}
- candidates, err := getAllCandidates(context.Background(), "", "foo.go", mt.env)
- if err != nil {
- t.Fatalf("getAllCandidates() = %v", err)
- }
+ var mu sync.Mutex
var got []res
- for _, c := range candidates {
+ add := func(c ImportFix) {
+ mu.Lock()
+ defer mu.Unlock()
for _, w := range want {
if c.StmtInfo.ImportPath == w.path {
- got = append(got, res{c.IdentName, c.StmtInfo.ImportPath})
+ got = append(got, res{c.Relevance, c.IdentName, c.StmtInfo.ImportPath})
}
}
}
+ if err := getAllCandidates(context.Background(), add, "", "foo.go", mt.env); err != nil {
+ t.Fatalf("getAllCandidates() = %v", err)
+ }
+ sort.Slice(got, func(i, j int) bool {
+ ri, rj := got[i], got[j]
+ if ri.relevance != rj.relevance {
+ return ri.relevance > rj.relevance // Highest first.
+ }
+ return ri.name < rj.name
+ })
if !reflect.DeepEqual(want, got) {
t.Errorf("wanted candidates in order %v, got %v", want, got)
}
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index 767181d..ef2318e 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -14,6 +14,7 @@
"math"
"strconv"
"strings"
+ "sync"
"time"
"golang.org/x/tools/go/ast/astutil"
@@ -245,6 +246,13 @@
return p.content[p.cursor-p.spanRange.Start:]
}
+func (c *completer) deepCompletionContext() (context.Context, context.CancelFunc) {
+ if c.opts.Budget == 0 {
+ return context.WithCancel(c.ctx)
+ }
+ return context.WithDeadline(c.ctx, c.startTime.Add(c.opts.Budget))
+}
+
func (c *completer) setSurrounding(ident *ast.Ident) {
if c.surrounding != nil {
return
@@ -622,15 +630,11 @@
// Try unimported packages.
if id, ok := sel.X.(*ast.Ident); ok && c.opts.Unimported && len(c.items) < unimportedTarget {
- pkgExports, err := PackageExports(c.ctx, c.snapshot.View(), id.Name, c.filename)
- if err != nil {
- return err
- }
+ ctx, cancel := c.deepCompletionContext()
+ defer cancel()
+
known := c.snapshot.KnownImportPaths()
- for _, pkgExport := range pkgExports {
- if len(c.items) >= unimportedTarget {
- break
- }
+ add := func(pkgExport imports.PackageExport) {
// 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(), &importInfo{
@@ -638,22 +642,30 @@
name: pkgExport.Fix.StmtInfo.Name,
pkg: knownPkg,
})
- continue
+ } else {
+ // Otherwise, continue with untyped proposals.
+ pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName)
+ for _, export := range pkgExport.Exports {
+ score := 0.01 * float64(pkgExport.Fix.Relevance)
+ c.found(candidate{
+ obj: types.NewVar(0, pkg, export, nil),
+ score: score,
+ imp: &importInfo{
+ importPath: pkgExport.Fix.StmtInfo.ImportPath,
+ name: pkgExport.Fix.StmtInfo.Name,
+ },
+ })
+ }
}
-
- // Otherwise, continue with untyped proposals.
- pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName)
- for _, export := range pkgExport.Exports {
- c.found(candidate{
- obj: types.NewVar(0, pkg, export, nil),
- score: 0.07,
- imp: &importInfo{
- importPath: pkgExport.Fix.StmtInfo.ImportPath,
- name: pkgExport.Fix.StmtInfo.Name,
- },
- })
+ if len(c.items) >= unimportedTarget {
+ cancel()
}
}
+ if err := c.snapshot.View().RunProcessEnvFunc(ctx, func(opts *imports.Options) error {
+ return imports.GetPackageExports(ctx, add, id.Name, c.filename, opts)
+ }); err != nil && err != context.Canceled {
+ return err
+ }
}
return nil
}
@@ -830,39 +842,52 @@
}
if c.opts.Unimported && len(c.items) < unimportedTarget {
- ctx, cancel := context.WithDeadline(c.ctx, c.startTime.Add(c.opts.Budget))
+ ctx, cancel := c.deepCompletionContext()
defer cancel()
// Suggest packages that have not been imported yet.
prefix := ""
if c.surrounding != nil {
prefix = c.surrounding.Prefix()
}
- pkgs, err := CandidateImports(ctx, prefix, c.snapshot.View(), c.filename)
- if err != nil {
- return err
- }
- score := stdScore
- // Rank unimported packages significantly lower than other results.
- score *= 0.07
+ var mu sync.Mutex
+ add := func(pkg imports.ImportFix) {
+ mu.Lock()
+ defer mu.Unlock()
+ if _, ok := seen[pkg.IdentName]; ok {
+ return
+ }
+ // Rank unimported packages significantly lower than other results.
+ score := 0.01 * float64(pkg.Relevance)
- for _, pkg := range pkgs {
+ // Do not add the unimported packages to seen, since we can have
+ // multiple packages of the same name as completion suggestions, since
+ // only one will be chosen.
+ obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkg.StmtInfo.ImportPath, pkg.IdentName))
+ c.found(candidate{
+ obj: obj,
+ score: score,
+ imp: &importInfo{
+ importPath: pkg.StmtInfo.ImportPath,
+ name: pkg.StmtInfo.Name,
+ },
+ })
+
if len(c.items) >= unimportedTarget {
- break
+ cancel()
}
- if _, ok := seen[pkg.IdentName]; !ok {
- // Do not add the unimported packages to seen, since we can have
- // multiple packages of the same name as completion suggestions, since
- // only one will be chosen.
- obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkg.StmtInfo.ImportPath, pkg.IdentName))
- c.found(candidate{
- obj: obj,
- score: score,
- imp: &importInfo{
- importPath: pkg.StmtInfo.ImportPath,
- name: pkg.StmtInfo.Name,
- },
- })
- }
+ c.found(candidate{
+ obj: obj,
+ score: score,
+ imp: &importInfo{
+ importPath: pkg.StmtInfo.ImportPath,
+ name: pkg.StmtInfo.Name,
+ },
+ })
+ }
+ if err := c.snapshot.View().RunProcessEnvFunc(ctx, func(opts *imports.Options) error {
+ return imports.GetAllCandidates(ctx, add, prefix, c.filename, opts)
+ }); err != nil && err != context.Canceled {
+ return err
}
}
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index e11e2cd..406c795 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -303,37 +303,6 @@
return src[0:fset.Position(end).Offset], true
}
-// CandidateImports returns every import that could be added to filename.
-func CandidateImports(ctx context.Context, prefix string, view View, filename string) ([]imports.ImportFix, error) {
- ctx, done := trace.StartSpan(ctx, "source.CandidateImports")
- defer done()
-
- var imps []imports.ImportFix
- importFn := func(opts *imports.Options) error {
- var err error
- imps, err = imports.GetAllCandidates(ctx, prefix, filename, opts)
- return err
- }
- err := view.RunProcessEnvFunc(ctx, importFn)
- return imps, err
-}
-
-// PackageExports returns all the packages named pkg that could be imported by
-// filename, and their exports.
-func PackageExports(ctx context.Context, view View, pkg, filename string) ([]imports.PackageExport, error) {
- ctx, done := trace.StartSpan(ctx, "source.PackageExports")
- defer done()
-
- var pkgs []imports.PackageExport
- importFn := func(opts *imports.Options) error {
- var err error
- pkgs, err = imports.GetPackageExports(ctx, pkg, filename, opts)
- return err
- }
- err := view.RunProcessEnvFunc(ctx, importFn)
- return pkgs, err
-}
-
// hasParseErrors returns true if the given file has parse errors.
func hasParseErrors(pkg Package, uri span.URI) bool {
for _, e := range pkg.GetErrors() {
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index 734b201..3b9d129 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -20,6 +20,7 @@
"strings"
"sync"
"testing"
+ "time"
"golang.org/x/tools/go/expect"
"golang.org/x/tools/go/packages"
@@ -190,6 +191,7 @@
}
o.HoverKind = source.SynopsisDocumentation
o.InsertTextFormat = protocol.SnippetTextFormat
+ o.Completion.Budget = time.Minute
return o
}
