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 <[email protected]>

Author: heschi

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
 			}
 

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
+`
 
-var (
-	_ = fmt.Print
-	_ = mypkg.Foo
+	const want = `package p
+
+import (
+	"fmt"
+
+	mypkg "mypkg.com/mypkg.v1"
 )
+
+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,25 +1574,57 @@ 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
 `
 	testConfig{
 		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)