From a28dfb48e06b2296b66678872c2cb638f0304f20 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Wed, 7 Nov 2018 22:40:17 +0000 Subject: [PATCH] revert "imports: create named imports for name/path mismatches" This reverts CL 145699 (commit 864069cfd1b106326ae556b3ca5c1d6b94645987) Reason for revert: If the mismatch is bad enough that goimports can't find it again, then the import is just removed, even if the user adds it back again. Fixes #28645. Change-Id: I6c8fc5434c2b56c73b260bcec2c12d8746fac4ad Reviewed-on: https://go-review.googlesource.com/c/148237 Reviewed-by: Brad Fitzpatrick --- imports/fix.go | 118 +++++++++++++++++++++++++++++++++++--------- imports/fix_test.go | 69 ++++++++++++++++---------- 2 files changed, 139 insertions(+), 48 deletions(-) diff --git a/imports/fix.go b/imports/fix.go index ed702f458c..187bc7d3bf 100644 --- a/imports/fix.go +++ b/imports/fix.go @@ -224,7 +224,7 @@ func fixImports(fset *token.FileSet, f *ast.File, filename string) (added []stri if ipath == "C" { break } - local := path.Base(ipath) + local := importPathToName(ipath, srcDir) decls[local] = v case *ast.SelectorExpr: xident, ok := v.X.(*ast.Ident) @@ -363,6 +363,98 @@ 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() { @@ -680,34 +772,14 @@ 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, + // If the package name in the source doesn't match the import path's base, // return true so the rewriter adds a name (import foo "github.com/bar/go-foo") - // Module-style version suffixes are allowed. - lastSeg := path.Base(pkg.importPath) - if isVersionSuffix(lastSeg) { - lastSeg = path.Base(path.Dir(pkg.importPath)) - } - needsRename := lastSeg != pkgName + needsRename := path.Base(pkg.importPath) != 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 0acb47897c..7d71ecd72f 100644 --- a/imports/fix_test.go +++ b/imports/fix_test.go @@ -7,6 +7,7 @@ package imports import ( "fmt" "go/build" + "path/filepath" "runtime" "strings" "sync" @@ -1349,15 +1350,9 @@ var ( ` testConfig{ - 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"}, - }, + module: packagestest.Module{ + Name: "mypkg.com/outpkg", + Files: fm{"toformat.go": input}, }, }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, input) } @@ -1368,24 +1363,18 @@ 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 "mypkg.com/mypkg.v1" +var ( + _ = fmt.Print + _ = mypkg.Foo ) - -var _, _ = fmt.Print, mypkg.Foo ` - testConfig{ gopathOnly: true, module: packagestest.Module{ @@ -1395,7 +1384,7 @@ var _, _ = fmt.Print, mypkg.Foo "toformat.go": input, }, }, - }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, want) + }.processTest(t, "mypkg.com/outpkg", "toformat.go", nil, nil, input) } func TestInternal(t *testing.T) { @@ -1573,14 +1562,16 @@ 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. +// match its package name. For example, we want to generate: +// +// import cloudbilling "google.golang.org/api/cloudbilling/v1" func TestRenameWhenPackageNameMismatch(t *testing.T) { const input = `package main const Y = bar.X` const want = `package main -import bar "foo.com/foo/bar/baz" +import bar "foo.com/foo/bar/v1" const Y = bar.X ` @@ -1588,8 +1579,8 @@ const Y = bar.X module: packagestest.Module{ Name: "foo.com", Files: fm{ - "foo/bar/baz/x.go": "package bar \n const X = 1", - "test/t.go": input, + "foo/bar/v1/x.go": "package bar \n const X = 1", + "test/t.go": input, }, }, }.processTest(t, "foo.com", "test/t.go", nil, nil, want) @@ -1734,6 +1725,34 @@ 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