go/packages: do not error out for patterns that match no packages

The documentation for Load says:
“Load returns an error if any of the patterns was invalid as defined
by the underlying build system. It may return an empty list of
packages without an error, for instance for an empty expansion of a
valid wildcard. Errors associated with a particular package are
recorded in the corresponding Package's Errors list, and do not cause
Load to return an error.”

Therefore, it should not be an error for a pattern to match no
packages. If the pattern is a literal package path that does not
exist, we should prefer to return a *Package for it with an error in
the Errors field.

Change-Id: Iaecfb920097e3b520e763bd52c0e326d2e7a4861
Reviewed-on: https://go-review.googlesource.com/137075
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/packages/external.go

@@ -16,7 +16,7 @@ import (
 	"strings"
 )
 
-// findExternalTool returns the file path of a tool that supplies supplies
+// findExternalTool returns the file path of a tool that supplies
 // the build system package structure, or "" if not found."
 // If GOPACKAGESDRIVER is set in the environment findExternalTool returns its
 // value, otherwise it searches for a binary named gopackagesdriver on the PATH.

go/packages/golist.go

@@ -136,6 +136,14 @@ type jsonPackage struct {
 	XTestImports    []string
 	ForTest         string // q in a "p [q.test]" package, else ""
 	DepOnly         bool
+
+	Error *jsonPackageError
+}
+
+type jsonPackageError struct {
+	ImportStack []string
+	Pos         string
+	Err         string
 }
 
 func otherFiles(p *jsonPackage) [][]string {
@@ -171,49 +179,48 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error)
 			return nil, fmt.Errorf("JSON decoding failed: %v", err)
 		}
 
-		// Bad package?
-		if p.Name == "" {
-			// This could be due to:
-			// - no such package
-			// - package directory contains no Go source files
-			// - all package declarations are mangled
-			// - and possibly other things.
-			//
-			// For now, we throw it away and let later
-			// stages rediscover the problem, but this
-			// discards the error message computed by go list
-			// and computes a new one---by different logic:
-			// if only one of the package declarations is
-			// bad, for example, should we report an error
-			// in Metadata mode?
-			// Unless we parse and typecheck, we might not
-			// notice there's a problem.
-			//
-			// Perhaps we should save a map of PackageID to
-			// errors for such cases.
-			continue
+		if p.ImportPath == "" {
+			// The documentation for go list says that “[e]rroneous packages will have
+			// a non-empty ImportPath”. If for some reason it comes back empty, we
+			// prefer to error out rather than silently discarding data or handing
+			// back a package without any way to refer to it.
+			if p.Error != nil {
+				return nil, Error{
+					Pos: p.Error.Pos,
+					Msg: p.Error.Err,
+				}
+			}
+			return nil, fmt.Errorf("package missing import path: %+v", p)
 		}
 
-		id := p.ImportPath
+		pkg := &Package{
+			Name:            p.Name,
+			ID:              p.ImportPath,
+			GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
+			CompiledGoFiles: absJoin(p.Dir, p.CompiledGoFiles),
+			OtherFiles:      absJoin(p.Dir, otherFiles(p)...),
+		}
 
 		// Extract the PkgPath from the package's ID.
-		pkgpath := id
-		if i := strings.IndexByte(id, ' '); i >= 0 {
-			pkgpath = id[:i]
+		if i := strings.IndexByte(pkg.ID, ' '); i >= 0 {
+			pkg.PkgPath = pkg.ID[:i]
+		} else {
+			pkg.PkgPath = pkg.ID
 		}
 
-		if pkgpath == "unsafe" {
-			p.GoFiles = nil // ignore fake unsafe.go file
+		if pkg.PkgPath == "unsafe" {
+			pkg.GoFiles = nil // ignore fake unsafe.go file
 		}
 
 		// Assume go list emits only absolute paths for Dir.
-		if !filepath.IsAbs(p.Dir) {
+		if p.Dir != "" && !filepath.IsAbs(p.Dir) {
 			log.Fatalf("internal error: go list returned non-absolute Package.Dir: %s", p.Dir)
 		}
 
-		export := p.Export
-		if export != "" && !filepath.IsAbs(export) {
-			export = filepath.Join(p.Dir, export)
+		if p.Export != "" && !filepath.IsAbs(p.Export) {
+			pkg.ExportFile = filepath.Join(p.Dir, p.Export)
+		} else {
+			pkg.ExportFile = p.Export
 		}
 
 		// imports
@@ -224,9 +231,9 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error)
 		for _, id := range p.Imports {
 			ids[id] = true
 		}
-		imports := make(map[string]*Package)
+		pkg.Imports = make(map[string]*Package)
 		for path, id := range p.ImportMap {
-			imports[path] = &Package{ID: id} // non-identity import
+			pkg.Imports[path] = &Package{ID: id} // non-identity import
 			delete(ids, id)
 		}
 		for id := range ids {
@@ -234,25 +241,24 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error)
 				continue
 			}
 
-			imports[id] = &Package{ID: id} // identity import
+			pkg.Imports[id] = &Package{ID: id} // identity import
 		}
 		if !p.DepOnly {
-			response.Roots = append(response.Roots, id)
-		}
-		pkg := &Package{
-			ID:              id,
-			Name:            p.Name,
-			PkgPath:         pkgpath,
-			GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
-			CompiledGoFiles: absJoin(p.Dir, p.CompiledGoFiles),
-			OtherFiles:      absJoin(p.Dir, otherFiles(p)...),
-			Imports:         imports,
-			ExportFile:      export,
+			response.Roots = append(response.Roots, pkg.ID)
 		}
+
 		// TODO(matloob): Temporary hack since CompiledGoFiles isn't always set.
 		if len(pkg.CompiledGoFiles) == 0 {
 			pkg.CompiledGoFiles = pkg.GoFiles
 		}
+
+		if p.Error != nil {
+			pkg.Errors = append(pkg.Errors, Error{
+				Pos: p.Error.Pos,
+				Msg: p.Error.Err,
+			})
+		}
+
 		response.Packages = append(response.Packages, pkg)
 	}
 

go/packages/packages.go

@@ -379,9 +379,6 @@ func newLoader(cfg *Config) *loader {
 // refine connects the supplied packages into a graph and then adds type and
 // and syntax information as requested by the LoadMode.
 func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
-	if len(list) == 0 {
-		return nil, fmt.Errorf("packages not found")
-	}
 	isRoot := make(map[string]bool, len(roots))
 	for _, root := range roots {
 		isRoot[root] = true

go/packages/packages_test.go

@@ -402,22 +402,23 @@ func TestConfigDir(t *testing.T) {
 	defer cleanup()
 
 	for _, test := range []struct {
-		dir     string
-		pattern string
-		want    string // value of Name constant, or error
+		dir         string
+		pattern     string
+		want        string // value of Name constant
+		errContains string
 	}{
-		{"", "a", `"a"`},
-		{"", "b", `"b"`},
-		{"", "./a", "packages not found"},
-		{"", "./b", "packages not found"},
-		{filepath.Join(tmp, "/src"), "a", `"a"`},
-		{filepath.Join(tmp, "/src"), "b", `"b"`},
-		{filepath.Join(tmp, "/src"), "./a", `"a"`},
-		{filepath.Join(tmp, "/src"), "./b", `"b"`},
-		{filepath.Join(tmp, "/src/a"), "a", `"a"`},
-		{filepath.Join(tmp, "/src/a"), "b", `"b"`},
-		{filepath.Join(tmp, "/src/a"), "./a", "packages not found"},
-		{filepath.Join(tmp, "/src/a"), "./b", `"a/b"`},
+		{pattern: "a", want: `"a"`},
+		{pattern: "b", want: `"b"`},
+		{pattern: "./a", errContains: "cannot find"},
+		{pattern: "./b", errContains: "cannot find"},
+		{dir: filepath.Join(tmp, "/src"), pattern: "a", want: `"a"`},
+		{dir: filepath.Join(tmp, "/src"), pattern: "b", want: `"b"`},
+		{dir: filepath.Join(tmp, "/src"), pattern: "./a", want: `"a"`},
+		{dir: filepath.Join(tmp, "/src"), pattern: "./b", want: `"b"`},
+		{dir: filepath.Join(tmp, "/src/a"), pattern: "a", want: `"a"`},
+		{dir: filepath.Join(tmp, "/src/a"), pattern: "b", want: `"b"`},
+		{dir: filepath.Join(tmp, "/src/a"), pattern: "./a", errContains: "cannot find"},
+		{dir: filepath.Join(tmp, "/src/a"), pattern: "./b", want: `"a/b"`},
 	} {
 		cfg := &packages.Config{
 			Mode: packages.LoadSyntax, // Use LoadSyntax to ensure that files can be opened.
@@ -426,16 +427,24 @@ func TestConfigDir(t *testing.T) {
 		}
 
 		initial, err := packages.Load(cfg, test.pattern)
-		var got string
+		var got, errMsg string
 		if err != nil {
-			got = err.Error()
-		} else {
-			got = constant(initial[0], "Name").Val().String()
+			errMsg = err.Error()
+		} else if len(initial) > 0 {
+			if len(initial[0].Errors) > 0 {
+				errMsg = initial[0].Errors[0].Error()
+			} else if c := constant(initial[0], "Name"); c != nil {
+				got = c.Val().String()
+			}
 		}
 		if got != test.want {
 			t.Errorf("dir %q, pattern %q: got %s, want %s",
 				test.dir, test.pattern, got, test.want)
 		}
+		if !strings.Contains(errMsg, test.errContains) {
+			t.Errorf("dir %q, pattern %q: error %s, want %s",
+				test.dir, test.pattern, errMsg, test.errContains)
+		}
 	}
 
 }
@@ -1427,5 +1436,9 @@ func makeTree(t *testing.T, tree map[string]string) (dir string, cleanup func())
 }
 
 func constant(p *packages.Package, name string) *types.Const {
-	return p.Types.Scope().Lookup(name).(*types.Const)
+	c := p.Types.Scope().Lookup(name)
+	if c == nil {
+		return nil
+	}
+	return c.(*types.Const)
 }