From 7871c2d76733878da8cc5459935c0e38cfa802b9 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Wed, 30 Oct 2019 16:59:29 -0400 Subject: [PATCH] internal/imports: sort import candidates by "relevance" When proposing packages to import, we can propose more relevant packages first. Introduce that concept to the pkg struct, and sort by it when returning candidates. In all cases we prefer stdlib packages first. Then, in module mode, we prefer packages that are in the module's dependencies over those that aren't. We could go further and prefer direct deps over indirect too, but I didn't have the code for that handy. I also changed the alphabetical sort from import path to package name, because that's what the user sees first in the UI. Updates golang/go#31906 Change-Id: Ia981ee9ffe3202e2a68eef3a36f65e81849a4ac2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/204203 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/imports/fix.go | 42 +++++++++++----- internal/imports/fix_test.go | 4 +- internal/imports/mod.go | 4 ++ internal/imports/mod_test.go | 49 +++++++++++++++++++ .../lsp/testdata/unimported/unimported.go | 6 +-- 5 files changed, 88 insertions(+), 17 deletions(-) diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 3e6e56df1e..72b43bd884 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -589,15 +589,6 @@ func getFixes(fset *token.FileSet, f *ast.File, filename string, env *ProcessEnv func getAllCandidates(filename string, env *ProcessEnv) ([]ImportFix, error) { // TODO(heschi): filter out current package. (Don't forget x_test can import x.) - // Exclude goroot results -- getting them is relatively expensive, not cached, - // and generally redundant with the in-memory version. - exclude := []gopathwalk.RootType{gopathwalk.RootGOROOT} - // Only the go/packages resolver uses the first argument, and nobody uses that resolver. - pkgs, err := env.GetResolver().scan(nil, true, exclude) - if err != nil { - return nil, err - } - // Start off with the standard library. var imports []ImportFix for importPath := range stdlib { @@ -609,6 +600,31 @@ func getAllCandidates(filename string, env *ProcessEnv) ([]ImportFix, error) { FixType: AddImport, }) } + // Sort the stdlib bits solely by name. + sort.Slice(imports, func(i int, j int) bool { + return imports[i].StmtInfo.ImportPath < imports[j].StmtInfo.ImportPath + }) + + // Exclude goroot results -- getting them is relatively expensive, not cached, + // and generally redundant with the in-memory version. + exclude := []gopathwalk.RootType{gopathwalk.RootGOROOT} + // Only the go/packages resolver uses the first argument, and nobody uses that resolver. + pkgs, err := env.GetResolver().scan(nil, true, exclude) + if err != nil { + return nil, err + } + // Sort first by relevance, then by name, so that when we add them they're + // still in order. + sort.Slice(pkgs, func(i, j int) bool { + pi, pj := pkgs[i], pkgs[j] + if pi.relevance < pj.relevance { + return true + } + if pi.relevance > pj.relevance { + return false + } + return pi.packageName < pj.packageName + }) dupCheck := map[string]struct{}{} for _, pkg := range pkgs { @@ -627,9 +643,6 @@ func getAllCandidates(filename string, env *ProcessEnv) ([]ImportFix, error) { FixType: AddImport, }) } - sort.Slice(imports, func(i int, j int) bool { - return imports[i].StmtInfo.ImportPath < imports[j].StmtInfo.ImportPath - }) return imports, nil } @@ -1036,6 +1049,7 @@ type pkg struct { dir string // absolute file path to pkg directory ("/usr/lib/go/src/net/http") importPathShort string // vendorless import path ("net/http", "a/b") packageName string // package name loaded from source if requested + relevance int // a weakly-defined score of how relevant a package is. 0 is most relevant. } type pkgDistance struct { @@ -1097,6 +1111,10 @@ func (r *gopathResolver) scan(_ references, loadNames bool, exclude []gopathwalk p := &pkg{ importPathShort: VendorlessPath(importpath), dir: dir, + relevance: 1, + } + if root.Type == gopathwalk.RootGOROOT { + p.relevance = 0 } if loadNames { var err error diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go index 6dafd40bac..a29fc6e591 100644 --- a/internal/imports/fix_test.go +++ b/internal/imports/fix_test.go @@ -2521,12 +2521,12 @@ func TestGetCandidates(t *testing.T) { name, path string } want := []res{ - {"bar", "bar.com/bar"}, {"bytes", "bytes"}, {"rand", "crypto/rand"}, - {"foo", "foo.com/foo"}, {"rand", "math/rand"}, {"http", "net/http"}, + {"bar", "bar.com/bar"}, + {"foo", "foo.com/foo"}, } testConfig{ diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 2d394abb4f..333dac4df8 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -430,12 +430,15 @@ func (r *ModuleResolver) canonicalize(info directoryPackageInfo) (*pkg, error) { importPathShort: info.nonCanonicalImportPath, dir: info.dir, packageName: path.Base(info.nonCanonicalImportPath), + relevance: 0, }, nil } importPath := info.nonCanonicalImportPath + relevance := 2 // Check if the directory is underneath a module that's in scope. if mod := r.findModuleByDir(info.dir); mod != nil { + relevance = 1 // It is. If dir is the target of a replace directive, // our guessed import path is wrong. Use the real one. if mod.Dir == info.dir { @@ -452,6 +455,7 @@ func (r *ModuleResolver) canonicalize(info directoryPackageInfo) (*pkg, error) { importPathShort: importPath, dir: info.dir, packageName: info.packageName, // may not be populated if the caller didn't ask for it + relevance: relevance, } // We may have discovered a package that has a different version // in scope already. Canonicalize to that one if possible. diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go index dd7c39f310..dcb8818aa0 100644 --- a/internal/imports/mod_test.go +++ b/internal/imports/mod_test.go @@ -10,6 +10,7 @@ import ( "log" "os" "path/filepath" + "reflect" "regexp" "strings" "sync" @@ -831,6 +832,54 @@ func TestInvalidModCache(t *testing.T) { resolver.scan(nil, true, nil) } +func TestGetCandidatesRanking(t *testing.T) { + mt := setup(t, ` +-- go.mod -- +module example.com + +require rsc.io/quote v1.5.1 + +-- rpackage/x.go -- +package rpackage +import _ "rsc.io/quote" +`, "") + defer mt.cleanup() + + if _, err := mt.env.invokeGo("mod", "download", "rsc.io/quote/v2@v2.0.1"); err != nil { + t.Fatal(err) + } + + type res struct { + name, path string + } + want := []res{ + // Stdlib + {"bytes", "bytes"}, + {"http", "net/http"}, + // In scope modules + {"language", "golang.org/x/text/language"}, + {"quote", "rsc.io/quote"}, + {"rpackage", "example.com/rpackage"}, + // Out of scope modules + {"quote", "rsc.io/quote/v2"}, + } + candidates, err := getAllCandidates("foo.go", mt.env) + if err != nil { + t.Fatalf("getAllCandidates() = %v", err) + } + var got []res + for _, c := range candidates { + for _, w := range want { + if c.StmtInfo.ImportPath == w.path { + got = append(got, res{c.IdentName, c.StmtInfo.ImportPath}) + } + } + } + if !reflect.DeepEqual(want, got) { + t.Errorf("wanted candidates in order %v, got %v", want, got) + } +} + func BenchmarkScanModCache(b *testing.B) { env := &ProcessEnv{ Debug: true, diff --git a/internal/lsp/testdata/unimported/unimported.go b/internal/lsp/testdata/unimported/unimported.go index f720392002..ed0670c808 100644 --- a/internal/lsp/testdata/unimported/unimported.go +++ b/internal/lsp/testdata/unimported/unimported.go @@ -1,13 +1,13 @@ package unimported func _() { - //@unimported("", bytes, context, cryptoslashrand, externalpackage, time, unsafe) + //@unimported("", bytes, context, cryptoslashrand, time, unsafe, externalpackage) } // Create markers for unimported std lib packages. Only for use by this test. /* bytes */ //@item(bytes, "bytes", "\"bytes\"", "package") /* context */ //@item(context, "context", "\"context\"", "package") /* rand */ //@item(cryptoslashrand, "rand", "\"crypto/rand\"", "package") -/* pkg */ //@item(externalpackage, "pkg", "\"example.com/extramodule/pkg\"", "package" ) -/* unsafe */ //@item(unsafe, "unsafe", "\"unsafe\"", "package") /* time */ //@item(time, "time", "\"time\"", "package") +/* unsafe */ //@item(unsafe, "unsafe", "\"unsafe\"", "package") +/* pkg */ //@item(externalpackage, "pkg", "\"example.com/extramodule/pkg\"", "package" )