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>
This commit is contained in:
Heschi Kreinick 2018-10-29 17:38:22 -04:00
parent 15ad1aa0cb
commit 864069cfd1
2 changed files with 48 additions and 139 deletions

View File

@ -224,7 +224,7 @@ func fixImports(fset *token.FileSet, f *ast.File, filename string) (added []stri
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 @@ func fixImports(fset *token.FileSet, f *ast.File, filename string) (added []stri
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 @@ func findImportGoPath(ctx context.Context, pkgName string, symbols map[string]bo
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.

View File

@ -7,7 +7,6 @@ package imports
import (
"fmt"
"go/build"
"path/filepath"
"runtime"
"strings"
"sync"
@ -1350,9 +1349,15 @@ var (
`
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 @@ var (
// 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 @@ var (
"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 @@ func (t *goimportTest) process(module, file string, contents []byte, opts *Optio
}
// 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 @@ const Y = bar.X
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 @@ const Y = foo.X
}.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