From 84a35ef54dff3c5596983e180ec10919fc432242 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Wed, 1 Mar 2017 17:02:44 +0900 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Josh Bleecher Snyder --- go/ast/astutil/imports.go | 26 ++++++++-- go/ast/astutil/imports_test.go | 89 ++++++++++++++++++++++++++++++++++ imports/fix_test.go | 64 ++++++++++++++++++++++++ 3 files changed, 176 insertions(+), 3 deletions(-) diff --git a/go/ast/astutil/imports.go b/go/ast/astutil/imports.go index 4a77c782..83f196cd 100644 --- a/go/ast/astutil/imports.go +++ b/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) diff --git a/go/ast/astutil/imports_test.go b/go/ast/astutil/imports_test.go index 21682674..8bc34808 100644 --- a/go/ast/astutil/imports_test.go +++ b/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" +) `, }, { diff --git a/imports/fix_test.go b/imports/fix_test.go index ffab3907..89c7e7bc 100644 --- a/imports/fix_test.go +++ b/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 +} `, }, }