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>
diff --git a/go/ast/astutil/imports.go b/go/ast/astutil/imports.go
index 4a77c78..83f196c 100644
--- a/go/ast/astutil/imports.go
+++ b/go/ast/astutil/imports.go
@@ -49,6 +49,8 @@
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 @@
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 @@
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)
diff --git a/go/ast/astutil/imports_test.go b/go/ast/astutil/imports_test.go
index 2168267..8bc3480 100644
--- a/go/ast/astutil/imports_test.go
+++ b/go/ast/astutil/imports_test.go
@@ -143,6 +143,95 @@
`,
},
{
+ 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"
+)
+`,
+ },
+ {
name: "import into singular group",
pkg: "bytes",
in: `package main
diff --git a/imports/fix_test.go b/imports/fix_test.go
index ffab390..89c7e7b 100644
--- a/imports/fix_test.go
+++ b/imports/fix_test.go
@@ -802,6 +802,70 @@
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
+}
+`,
+ },
}
func TestFixImports(t *testing.T) {
