go/ast/astutil: new third-party imports shouldn't go in the std group

Before this change, astutil would only do a prefix match of a new import
with all the existing ones, to try to place it in the correct group. If
none was found, the new import would be placed at the beginning of the
first import group.

This works well for new std imports, but it doesn't work well for new
third-party packages that don't share any prefix with any of the
existing imports.

Example:

	import (
		"time"

		"github.com/golang/snappy"
	)

When adding "golang.org/x/sys/unix" with astutil.AddImport, the import
is inserted as follows:

	import (
		"golang.org/x/sys/unix"
		"time"

		"github.com/golang/snappy"
	)

And goimports reorganizes the imports to separate std and third-party
packages:

	import (
		"time"

		"golang.org/x/sys/unix"

		"github.com/golang/snappy"
	)

We usually don't want to introduce a new import group; in most cases,
the desired behavior is separating std from third-party packages.

With this CL, new imports that don't share prefix with any existing ones
will be placed with the first group of third-party imports, if any
exist. If no third-party import group exists, a new one will be added.
In the case of our example above, this will be the new outcome:

	import (
		"time"

		"github.com/golang/snappy"
		"golang.org/x/sys/unix"
	)

Fixes golang/go#19190.

Change-Id: Id4630015c029bd815234a6c8726cb97f4af16f1c
Reviewed-on: https://go-review.googlesource.com/37552
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>

go/ast/astutil/imports.go

@@ -49,6 +49,8 @@ func AddNamedImport(fset *token.FileSet, f *ast.File, name, ipath string) (added
 		lastImport = -1         // index in f.Decls of the file's final import decl
 		impDecl    *ast.GenDecl // import decl containing the best match
 		impIndex   = -1         // spec index in impDecl containing the best match
+
+		isThirdPartyPath = isThirdParty(ipath)
 	)
 	for i, decl := range f.Decls {
 		gen, ok := decl.(*ast.GenDecl)
@@ -65,15 +67,27 @@ func AddNamedImport(fset *token.FileSet, f *ast.File, name, ipath string) (added
 				impDecl = gen
 			}
 
-			// Compute longest shared prefix with imports in this group.
+			// Compute longest shared prefix with imports in this group and find best
+			// matched import spec.
+			// 1. Always prefer import spec with longest shared prefix.
+			// 2. While match length is 0,
+			// - for stdlib package: prefer first import spec.
+			// - for third party package: prefer first third party import spec.
+			// We cannot use last import spec as best match for third party package
+			// because grouped imports are usually placed last by goimports -local
+			// flag.
+			// See issue #19190.
+			seenAnyThirdParty := false
 			for j, spec := range gen.Specs {
 				impspec := spec.(*ast.ImportSpec)
-				n := matchLen(importPath(impspec), ipath)
-				if n > bestMatch {
+				p := importPath(impspec)
+				n := matchLen(p, ipath)
+				if n > bestMatch || (bestMatch == 0 && !seenAnyThirdParty && isThirdPartyPath) {
 					bestMatch = n
 					impDecl = gen
 					impIndex = j
 				}
+				seenAnyThirdParty = seenAnyThirdParty || isThirdParty(p)
 			}
 		}
 	}
@@ -175,6 +189,12 @@ func AddNamedImport(fset *token.FileSet, f *ast.File, name, ipath string) (added
 	return true
 }
 
+func isThirdParty(importPath string) bool {
+	// Third party package import path usually contains "." (".com", ".org", ...)
+	// This logic is taken from golang.org/x/tools/imports package.
+	return strings.Contains(importPath, ".")
+}
+
 // DeleteImport deletes the import path from the file f, if present.
 func DeleteImport(fset *token.FileSet, f *ast.File, path string) (deleted bool) {
 	return DeleteNamedImport(fset, f, "", path)

go/ast/astutil/imports_test.go

@@ -140,6 +140,95 @@ import (
 
 	"d/f"
 )
+`,
+	},
+	{
+		name: "issue #19190",
+		pkg:  "x.org/y/z",
+		in: `package main
+
+// Comment
+import "C"
+
+import (
+	"bytes"
+	"os"
+
+	"d.com/f"
+)
+`,
+		out: `package main
+
+// Comment
+import "C"
+
+import (
+	"bytes"
+	"os"
+
+	"d.com/f"
+	"x.org/y/z"
+)
+`,
+	},
+	{
+		name: "issue #19190 with existing grouped import packages",
+		pkg:  "x.org/y/z",
+		in: `package main
+
+// Comment
+import "C"
+
+import (
+	"bytes"
+	"os"
+
+	"c.com/f"
+	"d.com/f"
+
+	"y.com/a"
+	"y.com/b"
+	"y.com/c"
+)
+`,
+		out: `package main
+
+// Comment
+import "C"
+
+import (
+	"bytes"
+	"os"
+
+	"c.com/f"
+	"d.com/f"
+	"x.org/y/z"
+
+	"y.com/a"
+	"y.com/b"
+	"y.com/c"
+)
+`,
+	},
+	{
+		name: "issue #19190 - match score is still respected",
+		pkg:  "y.org/c",
+		in: `package main
+
+import (
+	"x.org/a"
+
+	"y.org/b"
+)
+`,
+		out: `package main
+
+import (
+	"x.org/a"
+
+	"y.org/b"
+	"y.org/c"
+)
 `,
 	},
 	{

imports/fix_test.go

@@ -800,6 +800,70 @@ import (
 )
 
 var _ = fmt.Sprintf
+`,
+	},
+
+	{
+		name: "issue #19190 1",
+		in: `package main
+
+import (
+	"time"
+)
+
+func main() {
+	_ = snappy.Encode
+	_ = p.P
+	_ = time.Parse
+}
+`,
+		out: `package main
+
+import (
+	"time"
+
+	"code.google.com/p/snappy-go/snappy"
+	"rsc.io/p"
+)
+
+func main() {
+	_ = snappy.Encode
+	_ = p.P
+	_ = time.Parse
+}
+`,
+	},
+
+	{
+		name: "issue #19190 2",
+		in: `package main
+
+import (
+	"time"
+
+	"code.google.com/p/snappy-go/snappy"
+)
+
+func main() {
+	_ = snappy.Encode
+	_ = p.P
+	_ = time.Parse
+}
+`,
+		out: `package main
+
+import (
+	"time"
+
+	"code.google.com/p/snappy-go/snappy"
+	"rsc.io/p"
+)
+
+func main() {
+	_ = snappy.Encode
+	_ = p.P
+	_ = time.Parse
+}
 `,
 	},
 }