cmd/doc: don't stop after first package if the symbol is not found

The test case is
	go doc rand.Float64
The first package it finds is crypto/rand, which does not have a Float64.
Before this change, cmd/doc would stop there even though math/rand
has the symbol. After this change, we get:

	% go doc rand.Float64
	package rand // import "math/rand"

	func Float64() float64

	    Float64 returns, as a float64, a pseudo-random number in [0.0,1.0) from the
	    default Source.
	%

Another nice consequence is that if a symbol is not found, we might get
a longer list of packages that were examined:

	% go doc rand.Int64
	doc: no symbol Int64 in packages crypto/rand, math/rand
	exit status 1
	%

This change introduces a coroutine to scan the file system so that if
the symbol is not found, the coroutine can deliver another path to try.
(This is darned close to the original motivation for coroutines.)
Paths are delivered on an unbuffered channel so the scanner does
not proceed until candidate paths are needed.

The scanner is attached to a new type, called Dirs, that caches the results
so if we need to scan a second time, we don't walk the file system
again. This is significantly more efficient than the existing code, which
could scan the tree multiple times looking for a package with
the symbol.

Change-Id: I2789505b9992cf04c19376c51ae09af3bc305f7f
Reviewed-on: https://go-review.googlesource.com/14921Reviewed-by: Andrew Gerrand <adg@golang.org>

src/cmd/doc/dirs.go

+// Copyright 2015 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+	"go/build"
+	"os"
+	"path"
+	"path/filepath"
+	"strings"
+)
+
+// Dirs is a structure for scanning the directory tree.
+// Its Next method returns the next Go source directory it finds.
+// Although it can be used to scan the tree multiple times, it
+// only walks the tree once, caching the data it finds.
+type Dirs struct {
+	scan   chan string // directories generated by walk.
+	paths  []string    // Cache of known paths.
+	offset int         // Counter for Next.
+}
+
+var dirs Dirs
+
+func init() {
+	dirs.paths = make([]string, 0, 1000)
+	dirs.scan = make(chan string)
+	go dirs.walk()
+}
+
+// Reset puts the scan back at the beginning.
+func (d *Dirs) Reset() {
+	d.offset = 0
+}
+
+// Next returns the next directory in the scan. The boolean
+// is false when the scan is done.
+func (d *Dirs) Next() (string, bool) {
+	if d.offset < len(d.paths) {
+		path := d.paths[d.offset]
+		d.offset++
+		return path, true
+	}
+	path, ok := <-d.scan
+	if !ok {
+		return "", false
+	}
+	d.paths = append(d.paths, path)
+	d.offset++
+	return path, ok
+}
+
+// walk walks the trees in GOROOT and GOPATH.
+func (d *Dirs) walk() {
+	d.walkRoot(build.Default.GOROOT)
+	for _, root := range splitGopath() {
+		d.walkRoot(root)
+	}
+	close(d.scan)
+}
+
+// walkRoot walks a single directory. Each Go source directory it finds is
+// delivered on d.scan.
+func (d *Dirs) walkRoot(root string) {
+	root = path.Join(root, "src")
+	slashDot := string(filepath.Separator) + "."
+	// We put a slash on the pkg so can use simple string comparison below
+	// yet avoid inadvertent matches, like /foobar matching bar.
+
+	visit := func(pathName string, f os.FileInfo, err error) error {
+		if err != nil {
+			return nil
+		}
+		// One package per directory. Ignore the files themselves.
+		if !f.IsDir() {
+			return nil
+		}
+		// No .git or other dot nonsense please.
+		if strings.Contains(pathName, slashDot) {
+			return filepath.SkipDir
+		}
+		// Does the directory contain any Go files? If so, it's a candidate.
+		if hasGoFiles(pathName) {
+			d.scan <- pathName
+			return nil
+		}
+		return nil
+	}
+
+	filepath.Walk(root, visit)
+}
+
+// hasGoFiles tests whether the directory contains at least one file with ".go"
+// extension
+func hasGoFiles(path string) bool {
+	dir, err := os.Open(path)
+	if err != nil {
+		// ignore unreadable directories
+		return false
+	}
+	defer dir.Close()
+
+	names, err := dir.Readdirnames(0)
+	if err != nil {
+		// ignore unreadable directories
+		return false
+	}
+
+	for _, name := range names {
+		if strings.HasSuffix(name, ".go") {
+			return true
+		}
+	}
+
+	return false
+}

src/cmd/doc/doc_test.go

@@ -7,13 +7,21 @@ package main
 import (
 	"bytes"
 	"flag"
-	"os"
-	"os/exec"
 	"regexp"
 	"runtime"
+	"strings"
 	"testing"
 )
 
+func maybeSkip(t *testing.T) {
+	if strings.HasPrefix(runtime.GOOS, "nacl") {
+		t.Skip("nacl does not have a full file tree")
+	}
+	if runtime.GOOS == "darwin" && strings.HasPrefix(runtime.GOARCH, "arm") {
+		t.Skip("darwin/arm does not have a full file tree")
+	}
+}
+
 const (
 	dataDir = "testdata"
 	binary  = "testdoc"
@@ -301,9 +309,7 @@ var tests = []test{
 }
 
 func TestDoc(t *testing.T) {
-	if runtime.GOOS == "darwin" && (runtime.GOARCH == "arm" || runtime.GOARCH == "arm64") {
-		t.Skip("TODO: on darwin/arm, test fails: no such package cmd/doc/testdata")
-	}
+	maybeSkip(t)
 	for _, test := range tests {
 		var b bytes.Buffer
 		var flagSet flag.FlagSet
@@ -339,12 +345,59 @@ func TestDoc(t *testing.T) {
 	}
 }
 
-// run runs the command, but calls t.Fatal if there is an error.
-func run(c *exec.Cmd, t *testing.T) []byte {
-	output, err := c.CombinedOutput()
-	if err != nil {
-		os.Stdout.Write(output)
-		t.Fatal(err)
+// Test the code to try multiple packages. Our test case is
+//	go doc rand.Float64
+// This needs to find math/rand.Float64; however crypto/rand, which doesn't
+// have the symbol, usually appears first in the directory listing.
+func TestMultiplePackages(t *testing.T) {
+	if testing.Short() {
+		t.Skip("scanning file system takes too long")
+	}
+	maybeSkip(t)
+	var b bytes.Buffer // We don't care about the output.
+	// Make sure crypto/rand does not have the symbol.
+	{
+		var flagSet flag.FlagSet
+		err := do(&b, &flagSet, []string{"crypto/rand.float64"})
+		if err == nil {
+			t.Errorf("expected error from crypto/rand.float64")
+		} else if !strings.Contains(err.Error(), "no symbol float64") {
+			t.Errorf("unexpected error %q from crypto/rand.float64", err)
+		}
+	}
+	// Make sure math/rand does have the symbol.
+	{
+		var flagSet flag.FlagSet
+		err := do(&b, &flagSet, []string{"math/rand.float64"})
+		if err != nil {
+			t.Errorf("unexpected error %q from math/rand.float64", err)
+		}
+	}
+	// Try the shorthand.
+	{
+		var flagSet flag.FlagSet
+		err := do(&b, &flagSet, []string{"rand.float64"})
+		if err != nil {
+			t.Errorf("unexpected error %q from rand.float64", err)
+		}
+	}
+	// Now try a missing symbol. We should see both packages in the error.
+	{
+		var flagSet flag.FlagSet
+		err := do(&b, &flagSet, []string{"rand.doesnotexit"})
+		if err == nil {
+			t.Errorf("expected error from rand.doesnotexit")
+		} else {
+			errStr := err.Error()
+			if !strings.Contains(errStr, "no symbol") {
+				t.Errorf("error %q should contain 'no symbol", errStr)
+			}
+			if !strings.Contains(errStr, "crypto/rand") {
+				t.Errorf("error %q should contain crypto/rand", errStr)
+			}
+			if !strings.Contains(errStr, "math/rand") {
+				t.Errorf("error %q should contain math/rand", errStr)
+			}
+		}
 	}
-	return output
 }

src/cmd/doc/main.go

@@ -32,13 +32,13 @@
 package main
 
 import (
+	"bytes"
 	"flag"
 	"fmt"
 	"go/build"
 	"io"
 	"log"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 	"unicode"
@@ -84,40 +84,72 @@ func do(writer io.Writer, flagSet *flag.FlagSet, args []string) (err error) {
 	flagSet.BoolVar(&matchCase, "c", false, "symbol matching honors case (paths not affected)")
 	flagSet.BoolVar(&showCmd, "cmd", false, "show symbols with package docs even if package is a command")
 	flagSet.Parse(args)
-	buildPackage, userPath, symbol := parseArgs(flagSet.Args())
-	symbol, method := parseSymbol(symbol)
-	pkg := parsePackage(writer, buildPackage, userPath)
+	var paths []string
+	var symbol, method string
+	// Loop until something is printed.
+	dirs.Reset()
+	for i := 0; ; i++ {
+		buildPackage, userPath, sym, more := parseArgs(flagSet.Args())
+		if i > 0 && !more { // Ignore the "more" bit on the first iteration.
+			return failMessage(paths, symbol, method)
+		}
+		symbol, method = parseSymbol(sym)
+		pkg := parsePackage(writer, buildPackage, userPath)
+		paths = append(paths, pkg.prettyPath())
 
-	defer func() {
-		pkg.flush()
-		e := recover()
-		if e == nil {
-			return
+		defer func() {
+			pkg.flush()
+			e := recover()
+			if e == nil {
+				return
+			}
+			pkgError, ok := e.(PackageError)
+			if ok {
+				err = pkgError
+				return
+			}
+			panic(e)
+		}()
+
+		// The builtin package needs special treatment: its symbols are lower
+		// case but we want to see them, always.
+		if pkg.build.ImportPath == "builtin" {
+			unexported = true
 		}
-		pkgError, ok := e.(PackageError)
-		if ok {
-			err = pkgError
+
+		switch {
+		case symbol == "":
+			pkg.packageDoc() // The package exists, so we got some output.
 			return
+		case method == "":
+			if pkg.symbolDoc(symbol) {
+				return
+			}
+		default:
+			if pkg.methodDoc(symbol, method) {
+				return
+			}
 		}
-		panic(e)
-	}()
-
-	// The builtin package needs special treatment: its symbols are lower
-	// case but we want to see them, always.
-	if pkg.build.ImportPath == "builtin" {
-		unexported = true
 	}
+}
 
-	switch {
-	case symbol == "":
-		pkg.packageDoc()
-		return
-	case method == "":
-		pkg.symbolDoc(symbol)
-	default:
-		pkg.methodDoc(symbol, method)
+// failMessage creates a nicely formatted error message when there is no result to show.
+func failMessage(paths []string, symbol, method string) error {
+	var b bytes.Buffer
+	if len(paths) > 1 {
+		b.WriteString("s")
+	}
+	b.WriteString(" ")
+	for i, path := range paths {
+		if i > 0 {
+			b.WriteString(", ")
+		}
+		b.WriteString(path)
 	}
-	return nil
+	if method == "" {
+		return fmt.Errorf("no symbol %s in package%s", symbol, &b)
+	}
+	return fmt.Errorf("no method %s.%s in package%s", symbol, method, &b)
 }
 
 // parseArgs analyzes the arguments (if any) and returns the package
@@ -125,13 +157,19 @@ func do(writer io.Writer, flagSet *flag.FlagSet, args []string) (err error) {
 // the path (or "" if it's the current package) and the symbol
 // (possibly with a .method) within that package.
 // parseSymbol is used to analyze the symbol itself.
-func parseArgs(args []string) (*build.Package, string, string) {
+// The boolean final argument reports whether it is possible that
+// there may be more directories worth looking at. It will only
+// be true if the package path is a partial match for some directory
+// and there may be more matches. For example, if the argument
+// is rand.Float64, we must scan both crypto/rand and math/rand
+// to find the symbol, and the first call will return crypto/rand, true.
+func parseArgs(args []string) (pkg *build.Package, path, symbol string, more bool) {
 	switch len(args) {
 	default:
 		usage()
 	case 0:
 		// Easy: current directory.
-		return importDir(pwd()), "", ""
+		return importDir(pwd()), "", "", false
 	case 1:
 		// Done below.
 	case 2:
@@ -140,7 +178,7 @@ func parseArgs(args []string) (*build.Package, string, string) {
 		if err != nil {
 			log.Fatalf("%s", err)
 		}
-		return pkg, args[0], args[1]
+		return pkg, args[0], args[1], false
 	}
 	// Usual case: one argument.
 	arg := args[0]
@@ -150,7 +188,7 @@ func parseArgs(args []string) (*build.Package, string, string) {
 	// package paths as their prefix.
 	pkg, err := build.Import(arg, "", build.ImportComment)
 	if err == nil {
-		return pkg, arg, ""
+		return pkg, arg, "", false
 	}
 	// Another disambiguator: If the symbol starts with an upper
 	// case letter, it can only be a symbol in the current directory.
@@ -159,7 +197,7 @@ func parseArgs(args []string) (*build.Package, string, string) {
 	if isUpper(arg) {
 		pkg, err := build.ImportDir(".", build.ImportComment)
 		if err == nil {
-			return pkg, "", arg
+			return pkg, "", arg, false
 		}
 	}
 	// If it has a slash, it must be a package path but there is a symbol.
@@ -185,21 +223,23 @@ func parseArgs(args []string) (*build.Package, string, string) {
 		// Have we identified a package already?
 		pkg, err := build.Import(arg[0:period], "", build.ImportComment)
 		if err == nil {
-			return pkg, arg[0:period], symbol
+			return pkg, arg[0:period], symbol, false
 		}
 		// See if we have the basename or tail of a package, as in json for encoding/json
 		// or ivy/value for robpike.io/ivy/value.
-		path := findPackage(arg[0:period])
-		if path != "" {
-			return importDir(path), arg[0:period], symbol
+		// Launch findPackage as a goroutine so it can return multiple paths if required.
+		path, ok := findPackage(arg[0:period])
+		if ok {
+			return importDir(path), arg[0:period], symbol, true
 		}
+		dirs.Reset() // Next iteration of for loop must scan all the directories again.
 	}
 	// If it has a slash, we've failed.
 	if slash >= 0 {
 		log.Fatalf("no such package %s", arg[0:period])
 	}
 	// Guess it's a symbol in the current directory.
-	return importDir(pwd()), "", arg
+	return importDir(pwd()), "", arg, false
 }
 
 // importDir is just an error-catching wrapper for build.ImportDir.
@@ -260,26 +300,23 @@ func isUpper(name string) bool {
 	return unicode.IsUpper(ch)
 }
 
-// findPackage returns the full file name path specified by the
-// (perhaps partial) package path pkg.
-func findPackage(pkg string) string {
-	if pkg == "" {
-		return ""
-	}
-	if isUpper(pkg) {
-		return "" // Upper case symbol cannot be a package name.
+// findPackage returns the full file name path that first matches the
+// (perhaps partial) package path pkg. The boolean reports if any match was found.
+func findPackage(pkg string) (string, bool) {
+	if pkg == "" || isUpper(pkg) { // Upper case symbol cannot be a package name.
+		return "", false
 	}
-	path := pathFor(build.Default.GOROOT, pkg)
-	if path != "" {
-		return path
-	}
-	for _, root := range splitGopath() {
-		path = pathFor(root, pkg)
-		if path != "" {
-			return path
+	pkgString := filepath.Clean(string(filepath.Separator) + pkg)
+	for {
+		path, ok := dirs.Next()
+		if !ok {
+			return "", false
+		}
+		if strings.HasSuffix(path, pkgString) {
+			return path, true
 		}
 	}
-	return ""
+	return "", false
 }
 
 // splitGopath splits $GOPATH into a list of roots.
@@ -287,73 +324,6 @@ func splitGopath() []string {
 	return filepath.SplitList(build.Default.GOPATH)
 }
 
-// pathsFor recursively walks the tree at root looking for possible directories for the package:
-// those whose package path is pkg or which have a proper suffix pkg.
-func pathFor(root, pkg string) (result string) {
-	root = path.Join(root, "src")
-	slashDot := string(filepath.Separator) + "."
-	// We put a slash on the pkg so can use simple string comparison below
-	// yet avoid inadvertent matches, like /foobar matching bar.
-	pkgString := filepath.Clean(string(filepath.Separator) + pkg)
-
-	// We use panic/defer to short-circuit processing at the first match.
-	// A nil panic reports that the path has been found.
-	defer func() {
-		err := recover()
-		if err != nil {
-			panic(err)
-		}
-	}()
-
-	visit := func(pathName string, f os.FileInfo, err error) error {
-		if err != nil {
-			return nil
-		}
-		// One package per directory. Ignore the files themselves.
-		if !f.IsDir() {
-			return nil
-		}
-		// No .git or other dot nonsense please.
-		if strings.Contains(pathName, slashDot) {
-			return filepath.SkipDir
-		}
-		// Is the tail of the path correct?
-		if strings.HasSuffix(pathName, pkgString) && hasGoFiles(pathName) {
-			result = pathName
-			panic(nil)
-		}
-		return nil
-	}
-
-	filepath.Walk(root, visit)
-	return "" // Call to panic above sets the real value.
-}
-
-// hasGoFiles tests whether the directory contains at least one file with ".go"
-// extension
-func hasGoFiles(path string) bool {
-	dir, err := os.Open(path)
-	if err != nil {
-		// ignore unreadable directories
-		return false
-	}
-	defer dir.Close()
-
-	names, err := dir.Readdirnames(0)
-	if err != nil {
-		// ignore unreadable directories
-		return false
-	}
-
-	for _, name := range names {
-		if strings.HasSuffix(name, ".go") {
-			return true
-		}
-	}
-
-	return false
-}
-
 // pwd returns the current directory.
 func pwd() string {
 	wd, err := os.Getwd()

src/cmd/doc/pkg.go

@@ -16,6 +16,8 @@ import (
 	"io"
 	"log"
 	"os"
+	"path/filepath"
+	"strings"
 	"unicode"
 	"unicode/utf8"
 )
@@ -46,6 +48,33 @@ func (p PackageError) Error() string {
 	return string(p)
 }
 
+// prettyPath returns a version of the package path that is suitable for an
+// error message. It obeys the import comment if present. Also, since
+// pkg.build.ImportPath is sometimes the unhelpful "" or ".", it looks for a
+// directory name in GOROOT or GOPATH if that happens.
+func (pkg *Package) prettyPath() string {
+	path := pkg.build.ImportComment
+	if path == "" {
+		path = pkg.build.ImportPath
+	}
+	if path != "." && path != "" {
+		return path
+	}
+	// Conver the source directory into a more useful path.
+	path = filepath.Clean(pkg.build.Dir)
+	// Can we find a decent prefix?
+	goroot := filepath.Join(build.Default.GOROOT, "src")
+	if strings.HasPrefix(path, goroot) {
+		return path[len(goroot)+1:]
+	}
+	for _, gopath := range splitGopath() {
+		if strings.HasPrefix(path, gopath) {
+			return path[len(gopath)+1:]
+		}
+	}
+	return path
+}
+
 // pkg.Fatalf is like log.Fatalf, but panics so it can be recovered in the
 // main do function, so it doesn't cause an exit. Allows testing to work
 // without running a subprocess. The log prefix will be added when
@@ -344,7 +373,7 @@ func (pkg *Package) findTypeSpec(decl *ast.GenDecl, symbol string) *ast.TypeSpec
 // symbolDoc prints the docs for symbol. There may be multiple matches.
 // If symbol matches a type, output includes its methods factories and associated constants.
 // If there is no top-level symbol, symbolDoc looks for methods that match.
-func (pkg *Package) symbolDoc(symbol string) {
+func (pkg *Package) symbolDoc(symbol string) bool {
 	defer pkg.flush()
 	found := false
 	// Functions.
@@ -413,9 +442,10 @@ func (pkg *Package) symbolDoc(symbol string) {
 	if !found {
 		// See if there are methods.
 		if !pkg.printMethodDoc("", symbol) {
-			log.Printf("symbol %s not present in package %s installed in %q", symbol, pkg.name, pkg.build.ImportPath)
+			return false
 		}
 	}
+	return true
 }
 
 // trimUnexportedElems modifies spec in place to elide unexported fields from
@@ -493,11 +523,9 @@ func (pkg *Package) printMethodDoc(symbol, method string) bool {
 }
 
 // methodDoc prints the docs for matches of symbol.method.
-func (pkg *Package) methodDoc(symbol, method string) {
+func (pkg *Package) methodDoc(symbol, method string) bool {
 	defer pkg.flush()
-	if !pkg.printMethodDoc(symbol, method) {
-		pkg.Fatalf("no method %s.%s in package %s installed in %q", symbol, method, pkg.name, pkg.build.ImportPath)
-	}
+	return pkg.printMethodDoc(symbol, method)
 }
 
 // match reports whether the user's symbol matches the program's.