From 8a6051197512ff9cbf65108d80613b631fe0b4f3 Mon Sep 17 00:00:00 2001
From: Heschi Kreinick <heschi@google.com>
Date: Thu, 3 Jan 2019 13:53:54 -0500
Subject: [PATCH] imports: don't look for, or find, empty packages

Fix a pair of bugs that combined to cause golang/go#29520. First, don't go
looking for a package if we've only seen unexported identifiers selected
from it. It's probably a typo. Second, don't find packages with no files
in them, e.g. because they're all build tagged out. We can't know what
package they form, so we have no business considering them.

Test only for the first, since without the first bug the second has no
observable effect on behavior, and I don't want to test the private API.

Fixes golang/go#29520

Change-Id: I5b797940bec051be5945b9c5cb4e7bf28527a939
Reviewed-on: https://go-review.googlesource.com/c/156178
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
---
 imports/fix.go      | 72 ++++++++++++++++++---------------------------
 imports/fix_test.go | 10 +++++++
 2 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/imports/fix.go b/imports/fix.go
index 691240926cb..1565f92945d 100644
--- a/imports/fix.go
+++ b/imports/fix.go
@@ -157,7 +157,11 @@ func collectReferences(f *ast.File) map[string]map[string]bool {
 				break
 			}
 			if xident.Obj != nil {
-				// if the parser can resolve it, it's not a package ref
+				// If the parser can resolve it, it's not a package ref.
+				break
+			}
+			if !ast.IsExported(v.Sel.Name) {
+				// Whatever this is, it's not exported from a package.
 				break
 			}
 			pkgName := xident.Name
@@ -166,9 +170,7 @@ func collectReferences(f *ast.File) map[string]map[string]bool {
 				r = make(map[string]bool)
 				refs[pkgName] = r
 			}
-			if ast.IsExported(v.Sel.Name) {
-				r[v.Sel.Name] = true
-			}
+			r[v.Sel.Name] = true
 		}
 		return visitor
 	}
@@ -855,10 +857,7 @@ func loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[strin
 		for _, fname := range pkg.goPackage.CompiledGoFiles {
 			f, err := parser.ParseFile(fset, fname, nil, 0)
 			if err != nil {
-				if Debug {
-					log.Printf("Parsing %s: %v", fname, err)
-				}
-				return nil, err
+				return nil, fmt.Errorf("parsing %s: %v", fname, err)
 			}
 			for name := range f.Scope.Objects {
 				if ast.IsExported(name) {
@@ -871,34 +870,29 @@ func loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[strin
 
 	exports := make(map[string]bool)
 
-	buildCtx := build.Default
-
-	// ReadDir is like ioutil.ReadDir, but only returns *.go files
-	// and filters out _test.go files since they're not relevant
-	// and only slow things down.
-	buildCtx.ReadDir = func(dir string) (notTests []os.FileInfo, err error) {
-		all, err := ioutil.ReadDir(dir)
-		if err != nil {
-			return nil, err
+	// Look for non-test, buildable .go files which could provide exports.
+	all, err := ioutil.ReadDir(pkg.dir)
+	if err != nil {
+		return nil, err
+	}
+	var files []os.FileInfo
+	for _, fi := range all {
+		name := fi.Name()
+		if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
+			continue
 		}
-		notTests = all[:0]
-		for _, fi := range all {
-			name := fi.Name()
-			if strings.HasSuffix(name, ".go") && !strings.HasSuffix(name, "_test.go") {
-				notTests = append(notTests, fi)
-			}
+		match, err := build.Default.MatchFile(pkg.dir, fi.Name())
+		if err != nil || !match {
+			continue
 		}
-		return notTests, nil
+		files = append(files, fi)
 	}
 
-	files, err := buildCtx.ReadDir(pkg.dir)
-	if err != nil {
-		log.Print(err)
-		return nil, err
+	if len(files) == 0 {
+		return nil, fmt.Errorf("dir %v contains no buildable, non-test .go files", pkg.dir)
 	}
 
 	fset := token.NewFileSet()
-
 	for _, fi := range files {
 		select {
 		case <-ctx.Done():
@@ -906,30 +900,19 @@ func loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[strin
 		default:
 		}
 
-		match, err := buildCtx.MatchFile(pkg.dir, fi.Name())
-		if err != nil || !match {
-			continue
-		}
 		fullFile := filepath.Join(pkg.dir, fi.Name())
 		f, err := parser.ParseFile(fset, fullFile, nil, 0)
 		if err != nil {
-			if Debug {
-				log.Printf("Parsing %s: %v", fullFile, err)
-			}
-			return nil, err
+			return nil, fmt.Errorf("parsing %s: %v", fullFile, err)
 		}
 		pkgName := f.Name.Name
 		if pkgName == "documentation" {
 			// Special case from go/build.ImportDir, not
-			// handled by buildCtx.MatchFile.
+			// handled by MatchFile above.
 			continue
 		}
 		if pkgName != expectPackage {
-			err := fmt.Errorf("scan of dir %v is not expected package %v (actually %v)", pkg.dir, expectPackage, pkgName)
-			if Debug {
-				log.Print(err)
-			}
-			return nil, err
+			return nil, fmt.Errorf("scan of dir %v is not expected package %v (actually %v)", pkg.dir, expectPackage, pkgName)
 		}
 		for name := range f.Scope.Objects {
 			if ast.IsExported(name) {
@@ -1015,6 +998,9 @@ func findImport(ctx context.Context, dirScan []*pkg, pkgName string, symbols map
 
 				exports, err := loadExports(ctx, pkgName, c.pkg)
 				if err != nil {
+					if Debug {
+						log.Printf("loading exports in dir %s (seeking package %s): %v", c.pkg.dir, pkgName, err)
+					}
 					resc <- nil
 					return
 				}
diff --git a/imports/fix_test.go b/imports/fix_test.go
index ef80384c063..1006e62d2a2 100644
--- a/imports/fix_test.go
+++ b/imports/fix_test.go
@@ -752,6 +752,16 @@ func main() { fmt.Println() }
 `,
 	},
 
+	{
+		name: "ignore_unexported_identifier",
+		in: `package main
+var _ = fmt.unexported`,
+		out: `package main
+
+var _ = fmt.unexported
+`,
+	},
+
 	// FormatOnly
 	{
 		name:       "formatonly_works",