imports: create named imports for name/path mismatches
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.
goimports already had almost exactly the desired logic, but only when
adding a new import. So the bulk of this change is simply removing the
logic that allowed it to recognize that a mismatched import satisfied
some uses. With that gone, it will remove those imports as unused, then
add a new renamed import. Some comments may be destroyed.
Fixes golang/go#28428
Change-Id: I53846e6046affb420f41719f84c71086c5b9e5e6
Reviewed-on: https://go-review.googlesource.com/c/145699
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/imports/fix.go b/imports/fix.go
index 187bc7d..ed702f4 100644
--- a/imports/fix.go
+++ b/imports/fix.go
@@ -224,7 +224,7 @@
if ipath == "C" {
break
}
- local := importPathToName(ipath, srcDir)
+ local := path.Base(ipath)
decls[local] = v
case *ast.SelectorExpr:
xident, ok := v.X.(*ast.Ident)
@@ -363,98 +363,6 @@
return added, nil
}
-// importPathToName returns the package name for the given import path.
-var importPathToName func(importPath, srcDir string) (packageName string) = importPathToNameGoPath
-
-// 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) {
- 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)
- }
- }
- }
- return base
-}
-
-// importPathToNameGoPath finds out the actual package name, as declared in its .go files.
-// If there's a problem, it falls back to using importPathToNameBasic.
-func importPathToNameGoPath(importPath, srcDir string) (packageName string) {
- // Fast path for standard library without going to disk.
- if pkg, ok := stdImportPackage[importPath]; ok {
- return pkg
- }
-
- pkgName, err := importPathToNameGoPathParse(importPath, srcDir)
- if Debug {
- log.Printf("importPathToNameGoPathParse(%q, srcDir=%q) = %q, %v", importPath, srcDir, pkgName, err)
- }
- if err == nil {
- return pkgName
- }
- return importPathToNameBasic(importPath, srcDir)
-}
-
-// importPathToNameGoPathParse is a faster version of build.Import if
-// the only thing desired is the package name. It uses build.FindOnly
-// to find the directory and then only parses one file in the package,
-// trusting that the files in the directory are consistent.
-func importPathToNameGoPathParse(importPath, srcDir string) (packageName string, err error) {
- buildPkg, err := build.Import(importPath, srcDir, build.FindOnly)
- if err != nil {
- return "", err
- }
- d, err := os.Open(buildPkg.Dir)
- if err != nil {
- return "", err
- }
- names, err := d.Readdirnames(-1)
- d.Close()
- if err != nil {
- return "", err
- }
- sort.Strings(names) // to have predictable behavior
- var lastErr error
- var nfile int
- for _, name := range names {
- if !strings.HasSuffix(name, ".go") {
- continue
- }
- if strings.HasSuffix(name, "_test.go") {
- continue
- }
- nfile++
- fullFile := filepath.Join(buildPkg.Dir, name)
-
- fset := token.NewFileSet()
- f, err := parser.ParseFile(fset, fullFile, nil, parser.PackageClauseOnly)
- if err != nil {
- lastErr = err
- continue
- }
- pkgName := f.Name.Name
- if pkgName == "documentation" {
- // Special case from go/build.ImportDir, not
- // handled by ctx.MatchFile.
- continue
- }
- if pkgName == "main" {
- // Also skip package main, assuming it's a +build ignore generator or example.
- // Since you can't import a package main anyway, there's no harm here.
- continue
- }
- return pkgName, nil
- }
- if lastErr != nil {
- return "", lastErr
- }
- return "", fmt.Errorf("no importable package found in %d Go files", nfile)
-}
-
var stdImportPackage = map[string]string{} // "net/http" => "http"
func init() {
@@ -772,14 +680,34 @@
if pkg == nil {
continue
}
- // If the package name in the source doesn't match the import path's base,
+ // If the package name in the source doesn't match the import path,
// return true so the rewriter adds a name (import foo "github.com/bar/go-foo")
- needsRename := path.Base(pkg.importPath) != pkgName
+ // Module-style version suffixes are allowed.
+ lastSeg := path.Base(pkg.importPath)
+ if isVersionSuffix(lastSeg) {
+ lastSeg = path.Base(path.Dir(pkg.importPath))
+ }
+ needsRename := lastSeg != pkgName
return pkg.importPathShort, needsRename, nil
}
return "", false, nil
}
+// isVersionSuffix reports whether the path segment seg is a semantic import
+// versioning style major version suffix.
+func isVersionSuffix(seg string) bool {
+ if seg == "" {
+ return false
+ }
+ if seg[0] != 'v' {
+ return false
+ }
+ if _, err := strconv.Atoi(seg[1:]); err != nil {
+ return false
+ }
+ return true
+}
+
// pkgIsCandidate reports whether pkg is a candidate for satisfying the
// finding which package pkgIdent in the file named by filename is trying
// to refer to.
diff --git a/imports/fix_test.go b/imports/fix_test.go
index 7d71ecd..0acb478 100644
--- a/imports/fix_test.go
+++ b/imports/fix_test.go
@@ -7,7 +7,6 @@
import (
"fmt"
"go/build"
- "path/filepath"
"runtime"
"strings"
"sync"
@@ -1350,9 +1349,15 @@
`
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 +1368,24 @@
// 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
+`
+
+ const want = `package p
import (
"fmt"
- "mypkg.com/mypkg.v1"
+ mypkg "mypkg.com/mypkg.v1"
)
-var (
- _ = fmt.Print
- _ = mypkg.Foo
-)
+var _, _ = fmt.Print, mypkg.Foo
`
+
testConfig{
gopathOnly: true,
module: packagestest.Module{
@@ -1384,7 +1395,7 @@
"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,16 +1573,14 @@
}
// 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
`
@@ -1579,8 +1588,8 @@
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)
@@ -1725,34 +1734,6 @@
}.processTest(t, "foo.com", "x/x.go", nil, nil, want)
}
-// Tests importPathToNameGoPathParse and in particular that it stops
-// after finding the first non-documentation package name, not
-// reporting an error on inconsistent package names (since it should
-// never make it that far).
-func TestImportPathToNameGoPathParse(t *testing.T) {
- testConfig{
- gopathOnly: true,
- module: packagestest.Module{
- Name: "example.net/pkg",
- Files: fm{
- "doc.go": "package documentation\n", // ignored
- "gen.go": "package main\n", // also ignored
- "pkg.go": "package the_pkg_name_to_find\n and this syntax error is ignored because of parser.PackageClauseOnly",
- "z.go": "package inconsistent\n", // inconsistent but ignored
- },
- },
- }.test(t, func(t *goimportTest) {
- got, err := importPathToNameGoPathParse("example.net/pkg", filepath.Join(t.gopath, "src", "other.net"))
- if err != nil {
- t.Fatal(err)
- }
- const want = "the_pkg_name_to_find"
- if got != want {
- t.Errorf("importPathToNameGoPathParse(..) = %q; want %q", got, want)
- }
- })
-}
-
func TestIgnoreConfiguration(t *testing.T) {
const input = `package x
