diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 3e6e56df1ef..72b43bd8848 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 6dafd40bacb..a29fc6e5912 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 2d394abb4f2..333dac4df89 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 dd7c39f3101..dcb8818aa08 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 f7203920023..ed0670c808c 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" )