imports: drop anything after a non identifier rune in package names

A package name cannot contain a '.' anyway, so this is a mostly likely
internal package name in the presence of a '.' in the import path.
This stops goimports from adding a local alias in places where it is not
needed, for instance in gopkg.in conventions.

Fixes golang/go#29556

Change-Id: I0ab11f2852d7f1dae14457995692760077201c8e
Reviewed-on: https://go-review.googlesource.com/c/157357
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

Author: ianthehat

imports/fix.go

@@ -23,6 +23,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+	"unicode"
+	"unicode/utf8"
 
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/go/packages"
@@ -289,7 +291,7 @@ func (p *pass) importIdentifier(imp *importInfo) string {
 	if known != nil && known.name != "" {
 		return known.name
 	}
-	return importPathToNameBasic(imp.importPath, p.srcDir)
+	return importPathToAssumedName(imp.importPath)
 }
 
 // load reads in everything necessary to run a pass, and reports whether the
@@ -389,7 +391,7 @@ func (p *pass) fix() bool {
 			}
 			path := strings.Trim(imp.Path.Value, `""`)
 			ident := p.importIdentifier(&importInfo{importPath: path})
-			if ident != importPathToNameBasic(path, p.srcDir) {
+			if ident != importPathToAssumedName(path) {
 				imp.Name = &ast.Ident{Name: ident, NamePos: imp.Pos()}
 			}
 		}
@@ -648,7 +650,7 @@ func (r *goPackagesResolver) loadPackageNames(importPaths []string, srcDir strin
 		if _, ok := names[path]; ok {
 			continue
 		}
-		names[path] = importPathToNameBasic(path, srcDir)
+		names[path] = importPathToAssumedName(path)
 	}
 	return names, nil
 
@@ -741,18 +743,36 @@ func addExternalCandidates(pass *pass, refs references, filename string) error {
 	return firstErr
 }
 
-// importPathToNameBasic assumes the package name is the base of import path,
-// except that if the path ends in foo/vN, it assumes the package name is foo.
-func importPathToNameBasic(importPath, srcDir string) (packageName string) {
+// notIdentifier reports whether ch is an invalid identifier character.
+func notIdentifier(ch rune) bool {
+	return !('a' <= ch && ch <= 'z' || 'A' <= ch && ch <= 'Z' ||
+		'0' <= ch && ch <= '9' ||
+		ch == '_' ||
+		ch >= utf8.RuneSelf && (unicode.IsLetter(ch) || unicode.IsDigit(ch)))
+}
+
+// importPathToAssumedName returns the assumed package name of an import path.
+// It does this using only string parsing of the import path.
+// It picks the last element of the path that does not look like a major
+// version, and then picks the valid identifier off the start of that element.
+// It is used to determine if a local rename should be added to an import for
+// clarity.
+// This function could be moved to a standard package and exported if we want
+// for use in other tools.
+func importPathToAssumedName(importPath string) string {
 	base := path.Base(importPath)
 	if strings.HasPrefix(base, "v") {
 		if _, err := strconv.Atoi(base[1:]); err == nil {
 			dir := path.Dir(importPath)
 			if dir != "." {
-				return path.Base(dir)
+				base = path.Base(dir)
 			}
 		}
 	}
+	base = strings.TrimPrefix(base, "go-")
+	if i := strings.IndexFunc(base, notIdentifier); i >= 0 {
+		base = base[:i]
+	}
 	return base
 }
 

imports/fix_test.go

@@ -1373,13 +1373,13 @@ var (
 
 // Test for correctly identifying the name of a vendored package when it
 // differs from its directory name. In this test, the import line
-// "mypkg.com/mypkg.v1" would be removed if goimports wasn't able to detect
+// "mypkg.com/mypkg_v1" would be removed if goimports wasn't able to detect
 // that the package name is "mypkg".
 func TestVendorPackage(t *testing.T) {
 	const input = `package p
 import (
 	"fmt"
-	"mypkg.com/mypkg.v1"
+	"mypkg.com/mypkg_v1"
 )
 var _, _ = fmt.Print, mypkg.Foo
 `
@@ -1389,7 +1389,7 @@ var _, _ = fmt.Print, mypkg.Foo
 import (
 	"fmt"
 
-	mypkg "mypkg.com/mypkg.v1"
+	mypkg "mypkg.com/mypkg_v1"
 )
 
 var _, _ = fmt.Print, mypkg.Foo
@@ -1400,7 +1400,7 @@ var _, _ = fmt.Print, mypkg.Foo
 		module: packagestest.Module{
 			Name: "mypkg.com/outpkg",
 			Files: fm{
-				"vendor/mypkg.com/mypkg.v1/f.go": "package mypkg\nvar Foo = 123\n",
+				"vendor/mypkg.com/mypkg_v1/f.go": "package mypkg\nvar Foo = 123\n",
 				"toformat.go":                    input,
 			},
 		},
@@ -1626,28 +1626,43 @@ func TestAddNameToMismatchedImport(t *testing.T) {
 	const input = `package main
 
 import (
+"foo.com/a.thing"
 "foo.com/surprise"
 "foo.com/v1"
+"foo.com/other/v2"
+"foo.com/other/v3"
+"foo.com/go-thing"
+"foo.com/go-wrong"
 )
 
-var _, _ = bar.X, v1.Y`
+var _ = []interface{}{bar.X, v1.Y, a.A, v2.V2, other.V3, thing.Thing, gow.Wrong}`
 
 	const want = `package main
 
 import (
+	"foo.com/a.thing"
+	"foo.com/go-thing"
+	gow "foo.com/go-wrong"
+	v2 "foo.com/other/v2"
+	"foo.com/other/v3"
 	bar "foo.com/surprise"
 	v1 "foo.com/v1"
 )
 
-var _, _ = bar.X, v1.Y
+var _ = []interface{}{bar.X, v1.Y, a.A, v2.V2, other.V3, thing.Thing, gow.Wrong}
 `
 
 	testConfig{
 		module: packagestest.Module{
 			Name: "foo.com",
 			Files: fm{
+				"a.thing/a.go":  "package a \n const A = 1",
 				"surprise/x.go": "package bar \n const X = 1",
 				"v1/x.go":       "package v1 \n const Y = 1",
+				"other/v2/y.go": "package v2 \n const V2 = 1",
+				"other/v3/z.go": "package other \n const V3 = 1",
+				"go-thing/b.go": "package thing \n const Thing = 1",
+				"go-wrong/b.go": "package gow \n const Wrong = 1",
 				"test/t.go":     input,
 			},
 		},