go/buildutil: use chan (not func) in the ForEachPackage API

The callbacks are intentionally concurrent, making this function very
easy to misuse (most clients so far have got it wrong, even my own).
Using a channel in the API makes the concurrency obvious, the
correct usage easy, and the client control flow simpler.

Change-Id: Ied38c3ed5c98b40eb1b322a984ed9dc092ac0918
Reviewed-on: https://go-review.googlesource.com/3250
Reviewed-by: Sameer Ajmani <sameer@golang.org>

go/buildutil/allpackages.go

@@ -30,11 +30,8 @@ import (
 //
 func AllPackages(ctxt *build.Context) []string {
 	var list []string
-	var mu sync.Mutex
 	ForEachPackage(ctxt, func(pkg string, _ error) {
-		mu.Lock()
 		list = append(list, pkg)
-		mu.Unlock()
 	})
 	sort.Strings(list)
 	return list
@@ -47,27 +44,42 @@ func AllPackages(ctxt *build.Context) []string {
 // If the package directory exists but could not be read, the second
 // argument to the found function provides the error.
 //
-// The found function and the build.Context file system interface
-// accessors must be concurrency safe.
+// All I/O is done via the build.Context file system interface,
+// which must be concurrency-safe.
 //
 func ForEachPackage(ctxt *build.Context, found func(importPath string, err error)) {
 	// We use a counting semaphore to limit
 	// the number of parallel calls to ReadDir.
 	sema := make(chan bool, 20)
 
+	ch := make(chan item)
+
 	var wg sync.WaitGroup
 	for _, root := range ctxt.SrcDirs() {
 		root := root
 		wg.Add(1)
 		go func() {
-			allPackages(ctxt, sema, root, found)
+			allPackages(ctxt, sema, root, ch)
 			wg.Done()
 		}()
 	}
+	go func() {
 		wg.Wait()
+		close(ch)
+	}()
+
+	// All calls to found occur in the caller's goroutine.
+	for i := range ch {
+		found(i.importPath, i.err)
+	}
 }
 
-func allPackages(ctxt *build.Context, sema chan bool, root string, found func(string, error)) {
+type item struct {
+	importPath string
+	err        error // (optional)
+}
+
+func allPackages(ctxt *build.Context, sema chan bool, root string, ch chan<- item) {
 	root = filepath.Clean(root) + string(os.PathSeparator)
 
 	var wg sync.WaitGroup
@@ -92,7 +104,7 @@ func allPackages(ctxt *build.Context, sema chan bool, root string, found func(st
 		files, err := ReadDir(ctxt, dir)
 		<-sema
 		if pkg != "" || err != nil {
-			found(pkg, err)
+			ch <- item{pkg, err}
 		}
 		for _, fi := range files {
 			fi := fi

refactor/rename/mvpkg.go

@@ -26,7 +26,6 @@ import (
 	"runtime"
 	"strconv"
 	"strings"
-	"sync"
 	"text/template"
 
 	"golang.org/x/tools/go/buildutil"
@@ -133,13 +132,12 @@ func srcDir(ctxt *build.Context, pkg string) (string, error) {
 // subpackages returns the set of packages in the given srcDir whose
 // import paths start with dir.
 func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool {
-	var mu sync.Mutex
 	subs := map[string]bool{dir: true}
 
 	// Find all packages under srcDir whose import paths start with dir.
 	buildutil.ForEachPackage(ctxt, func(pkg string, err error) {
 		if err != nil {
-			log.Fatalf("unexpected error in ForEackPackage: %v", err)
+			log.Fatalf("unexpected error in ForEachPackage: %v", err)
 		}
 
 		if !strings.HasPrefix(pkg, path.Join(dir, "")) {
@@ -157,9 +155,7 @@ func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool
 			return
 		}
 
-		mu.Lock()
 		subs[pkg] = true
-		mu.Unlock()
 	})
 
 	return subs

refactor/rename/mvpkg_test.go

@@ -15,7 +15,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-	"sync"
 	"testing"
 
 	"golang.org/x/tools/go/buildutil"
@@ -212,7 +211,6 @@ var _ a.T
 	for _, test := range tests {
 		ctxt := test.ctxt
 
-		var mu sync.Mutex
 		got := make(map[string]string)
 		// Populate got with starting file set. rewriteFile and moveDirectory
 		// will mutate got to produce resulting file set.
@@ -225,19 +223,17 @@ var _ a.T
 				return
 			}
 			f, err := ctxt.OpenFile(path)
-			defer f.Close()
 			if err != nil {
 				t.Errorf("unexpected error opening file: %s", err)
 				return
 			}
 			bytes, err := ioutil.ReadAll(f)
+			f.Close()
 			if err != nil {
 				t.Errorf("unexpected error reading file: %s", err)
 				return
 			}
-			mu.Lock()
 			got[path] = string(bytes)
-			defer mu.Unlock()
 		})
 		rewriteFile = func(fset *token.FileSet, f *ast.File, orig string) error {
 			var out bytes.Buffer