From ee45598d2ff288037f53f9e13ae0b1a6e2165ad5 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 20 Nov 2018 15:24:09 -0500 Subject: [PATCH] imports: create named imports for name/path mismatches (again) For clarity and performance reasons, we want the fast path of goimports to be purely syntactic. Packages whose import paths don't match their package names make this harder. Before this CL, we parsed each imported package to get its real package name. Now, we make named imports for such packages, and on subsequent runs we don't have to do the extra work. A package name matches its import path if the name is the last segment of the path, or the next-to-last before a version suffix vNN. gopkg.in style .vNN suffixes are considered mismatching. The previous attempt (CL 145699) failed because I assumed we'd be able to find all packages from scratch. That's not true if the import path is completely unrelated to the package name. To avoid that problem, rather than removing and re-adding mismatched imports, we just literally add names to them. Fixes golang/go#28428 Change-Id: I93bd820f5956162ae9c99afa73bdcfc570a2e7b4 Reviewed-on: https://go-review.googlesource.com/c/152000 Reviewed-by: Brad Fitzpatrick --- imports/fix.go | 27 ++++++++++++++--- imports/fix_test.go | 74 ++++++++++++++++++++++++++++++++++++--------- 2 files changed, 82 insertions(+), 19 deletions(-) diff --git a/imports/fix.go b/imports/fix.go index 198094b25d..6e969d03b3 100644 --- a/imports/fix.go +++ b/imports/fix.go @@ -238,9 +238,10 @@ type pass struct { used map[*importInfo]bool // Inputs to fix. These can be augmented between successive fix calls. - lastTry bool // indicates that this is the last call and fix should clean up as best it can. - candidates []*importInfo // candidate imports in priority order. - knownPackages map[string]*packageInfo // information about all known packages. + lastTry bool // indicates that this is the last call and fix should clean up as best it can. + addImportNames bool // add names to mismatched imports. + candidates []*importInfo // candidate imports in priority order. + knownPackages map[string]*packageInfo // information about all known packages. } // loadPackageNames saves the package names for everything referenced by imports. @@ -334,6 +335,23 @@ func (p *pass) fix() bool { astutil.DeleteNamedImport(p.fset, p.f, imp.name, imp.importPath) } } + + if p.addImportNames { + for _, imp := range p.f.Imports { + if imp.Name != nil { + continue + } + path := strings.Trim(imp.Path.Value, `""`) + pkg, ok := p.knownPackages[path] + if !ok { + continue + } + if pkg.name != importPathToNameBasic(path, p.srcDir) { + imp.Name = &ast.Ident{Name: pkg.name, NamePos: imp.Pos()} + } + } + } + for _, imp := range selected { astutil.AddNamedImport(p.fset, p.f, imp.name, imp.importPath) } @@ -421,6 +439,7 @@ func fixImports(fset *token.FileSet, f *ast.File, filename string) error { // the naive algorithm. p = &pass{fset: fset, f: f, srcDir: srcDir} p.pathToName = importPathToName + p.addImportNames = true p.otherFiles = otherFiles if p.load() { return nil @@ -506,7 +525,7 @@ func addGoPathCandidates(pass *pass, refs map[string]map[string]bool, filename s } // If the package name isn't what you'd expect looking // at the import path, add an explicit name. - if path.Base(ipath) != pkgName { + if importPathToNameBasic(ipath, pass.srcDir) != pkgName { imp.name = pkgName } diff --git a/imports/fix_test.go b/imports/fix_test.go index 7d71ecd72f..20bc219f03 100644 --- a/imports/fix_test.go +++ b/imports/fix_test.go @@ -1350,9 +1350,15 @@ var ( ` testConfig{ - module: packagestest.Module{ - Name: "mypkg.com/outpkg", - Files: fm{"toformat.go": input}, + modules: []packagestest.Module{ + { + Name: "mypkg.com/outpkg", + Files: fm{"toformat.go": input}, + }, + { + Name: "github.com/foo/v2", + Files: fm{"x.go": "package foo\n func Foo(){}\n"}, + }, }, }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, input) } @@ -1363,18 +1369,24 @@ var ( // that the package name is "mypkg". func TestVendorPackage(t *testing.T) { const input = `package p +import ( + "fmt" + "mypkg.com/mypkg.v1" +) +var _, _ = fmt.Print, mypkg.Foo +` + + const want = `package p import ( "fmt" - "mypkg.com/mypkg.v1" + mypkg "mypkg.com/mypkg.v1" ) -var ( - _ = fmt.Print - _ = mypkg.Foo -) +var _, _ = fmt.Print, mypkg.Foo ` + testConfig{ gopathOnly: true, module: packagestest.Module{ @@ -1384,7 +1396,7 @@ var ( "toformat.go": input, }, }, - }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, input) + }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, want) } func TestInternal(t *testing.T) { @@ -1562,16 +1574,14 @@ func (t *goimportTest) process(module, file string, contents []byte, opts *Optio } // Tests that added imports are renamed when the import path's base doesn't -// match its package name. For example, we want to generate: -// -// import cloudbilling "google.golang.org/api/cloudbilling/v1" +// match its package name. func TestRenameWhenPackageNameMismatch(t *testing.T) { const input = `package main const Y = bar.X` const want = `package main -import bar "foo.com/foo/bar/v1" +import bar "foo.com/foo/bar/baz" const Y = bar.X ` @@ -1579,8 +1589,42 @@ const Y = bar.X module: packagestest.Module{ Name: "foo.com", Files: fm{ - "foo/bar/v1/x.go": "package bar \n const X = 1", - "test/t.go": input, + "foo/bar/baz/x.go": "package bar \n const X = 1", + "test/t.go": input, + }, + }, + }.processTest(t, "foo.com", "test/t.go", nil, nil, want) +} + +// Tests that an existing import with badly mismatched path/name has its name +// correctly added. See #28645 and #29041. +func TestAddNameToMismatchedImport(t *testing.T) { + const input = `package main + +import ( +"foo.com/surprise" +"foo.com/v1" +) + +var _, _ = bar.X, v1.Y` + + const want = `package main + +import ( + bar "foo.com/surprise" + v1 "foo.com/v1" +) + +var _, _ = bar.X, v1.Y +` + + testConfig{ + module: packagestest.Module{ + Name: "foo.com", + Files: fm{ + "surprise/x.go": "package bar \n const X = 1", + "v1/x.go": "package v1 \n const Y = 1", + "test/t.go": input, }, }, }.processTest(t, "foo.com", "test/t.go", nil, nil, want)