From 9c57063f678bcbddd0bd7d5c26fdcc3f8ffee056 Mon Sep 17 00:00:00 2001 From: Michael Fraenkel Date: Wed, 15 Feb 2017 08:46:03 -0500 Subject: [PATCH] imports: prioritize closer packages Prefer imports that are closer to the current package. Fixes golang/go#17557 Change-Id: Iec55a294d396feac6234be307e08608b8559f65c Reviewed-on: https://go-review.googlesource.com/37070 Reviewed-by: Brad Fitzpatrick Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- imports/fix.go | 58 +++++++++++++++++++++++---------- imports/fix_test.go | 79 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 17 deletions(-) diff --git a/imports/fix.go b/imports/fix.go index e2460849763..8a0d9834b7a 100644 --- a/imports/fix.go +++ b/imports/fix.go @@ -87,14 +87,6 @@ var dirPackageInfo = dirPackageInfoFile func dirPackageInfoFile(pkgName, srcDir, filename string) (*packageInfo, error) { considerTests := strings.HasSuffix(filename, "_test.go") - // Handle file from stdin - if _, err := os.Stat(filename); err != nil { - if os.IsNotExist(err) { - return &packageInfo{}, nil - } - return nil, err - } - fileBase := filepath.Base(filename) packageFileInfos, err := ioutil.ReadDir(srcDir) if err != nil { @@ -401,19 +393,44 @@ type pkg struct { dir string // absolute file path to pkg directory ("/usr/lib/go/src/net/http") importPath string // full pkg import path ("net/http", "foo/bar/vendor/a/b") importPathShort string // vendorless import path ("net/http", "a/b") + distance int // relative distance to target } -// byImportPathShortLength sorts by the short import path length, breaking ties on the -// import string itself. -type byImportPathShortLength []*pkg +// byDistanceOrImportPathShortLength sorts by relative distance breaking ties +// on the short import path length and then the import string itself. +type byDistanceOrImportPathShortLength []*pkg + +func (s byDistanceOrImportPathShortLength) Len() int { return len(s) } +func (s byDistanceOrImportPathShortLength) Less(i, j int) bool { + di, dj := s[i].distance, s[j].distance + if di == -1 { + return false + } + if dj == -1 { + return true + } + if di != dj { + return di < dj + } -func (s byImportPathShortLength) Len() int { return len(s) } -func (s byImportPathShortLength) Less(i, j int) bool { vi, vj := s[i].importPathShort, s[j].importPathShort - return len(vi) < len(vj) || (len(vi) == len(vj) && vi < vj) - + if len(vi) != len(vj) { + return len(vi) < len(vj) + } + return vi < vj +} +func (s byDistanceOrImportPathShortLength) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + +func distance(basepath, targetpath string) int { + p, err := filepath.Rel(basepath, targetpath) + if err != nil { + return -1 + } + if p == "." { + return 0 + } + return strings.Count(p, string(filepath.Separator)) + 1 } -func (s byImportPathShortLength) Swap(i, j int) { s[i], s[j] = s[j], s[i] } // guarded by populateIgnoreOnce; populates ignoredDirs. func populateIgnore() { @@ -724,6 +741,12 @@ func findImportGoPath(pkgName string, symbols map[string]bool, filename string) defer testMu.RUnlock() } + pkgDir, err := filepath.Abs(filename) + if err != nil { + return "", false, err + } + pkgDir = filepath.Dir(pkgDir) + // Fast path for the standard library. // In the common case we hopefully never have to scan the GOPATH, which can // be slow with moving disks. @@ -765,6 +788,7 @@ func findImportGoPath(pkgName string, symbols map[string]bool, filename string) var candidates []*pkg for _, pkg := range dirScan { if pkgIsCandidate(filename, pkgName, pkg) { + pkg.distance = distance(pkgDir, pkg.dir) candidates = append(candidates, pkg) } } @@ -773,7 +797,7 @@ func findImportGoPath(pkgName string, symbols map[string]bool, filename string) // assuming that shorter package names are better than long // ones. Note that this sorts by the de-vendored name, so // there's no "penalty" for vendoring. - sort.Sort(byImportPathShortLength(candidates)) + sort.Sort(byDistanceOrImportPathShortLength(candidates)) if Debug { for i, pkg := range candidates { log.Printf("%s candidate %d/%d: %v in %v", pkgName, i+1, len(candidates), pkg.importPathShort, pkg.dir) diff --git a/imports/fix_test.go b/imports/fix_test.go index 89c7e7bce69..2197bafd312 100644 --- a/imports/fix_test.go +++ b/imports/fix_test.go @@ -1878,3 +1878,82 @@ func TestProcessStdin(t *testing.T) { t.Errorf("expected fmt import; got: %s", got) } } + +// Tests LocalPackagePromotion when there is a local package that matches, it +// should be the closest match. +// https://golang.org/issues/17557 +func TestLocalPackagePromotion(t *testing.T) { + testConfig{ + gopathFiles: map[string]string{ + "config.net/config/config.go": "package config\n type SystemConfig struct {}", // Will match but should not be first choice + "mycompany.net/config/config.go": "package config\n type SystemConfig struct {}", // Will match but should not be first choice + "mycompany.net/tool/config/config.go": "package config\n type SystemConfig struct {}", // Local package should be promoted over shorter package + }, + }.test(t, func(t *goimportTest) { + const in = "package main\n var c = &config.SystemConfig{}" + const want = `package main + +import "mycompany.net/tool/config" + +var c = &config.SystemConfig{} +` + got, err := Process(filepath.Join(t.gopath, "src", "mycompany.net/tool/main.go"), []byte(in), nil) + if err != nil { + t.Fatal(err) + } + if string(got) != want { + t.Errorf("Process = %q; want %q", got, want) + } + }) +} + +// Tests FindImportInLocalGoFiles looks at the import lines for other Go files in the +// local directory, since the user is likely to import the same packages in the current +// Go file. If an import is found that satisfies the need, it should be used over the +// standard library. +// https://golang.org/issues/17557 +func TestFindImportInLocalGoFiles(t *testing.T) { + testConfig{ + gopathFiles: map[string]string{ + "bytes.net/bytes/bytes.go": "package bytes\n type Buffer struct {}", // Should be selected over standard library + "mycompany.net/tool/io.go": "package main\n import \"bytes.net/bytes\"\n var _ = &bytes.Buffer{}", // Contains package import that will cause stdlib to be ignored + "mycompany.net/tool/err.go": "package main\n import \"bogus.net/bytes\"\n var _ = &bytes.Buffer{}", // Contains import which is not resolved, so it is ignored + }, + }.test(t, func(t *goimportTest) { + const in = "package main\n var _ = &bytes.Buffer{}" + const want = `package main + +import "bytes.net/bytes" + +var _ = &bytes.Buffer{} +` + got, err := Process(filepath.Join(t.gopath, "src", "mycompany.net/tool/main.go"), []byte(in), nil) + if err != nil { + t.Fatal(err) + } + if string(got) != want { + t.Errorf("Process = got %q; want %q", got, want) + } + }) +} + +func TestImportNoGoFiles(t *testing.T) { + testConfig{ + gopathFiles: map[string]string{}, + }.test(t, func(t *goimportTest) { + const in = "package main\n var _ = &bytes.Buffer{}" + const want = `package main + +import "bytes" + +var _ = &bytes.Buffer{} +` + got, err := Process(filepath.Join(t.gopath, "src", "mycompany.net/tool/main.go"), []byte(in), nil) + if err != nil { + t.Fatal(err) + } + if string(got) != want { + t.Errorf("Process = got %q; want %q", got, want) + } + }) +}