diff --git a/go/packages/external.go b/go/packages/external.go index 80aac505..53cc080d 100644 --- a/go/packages/external.go +++ b/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. diff --git a/go/packages/golist.go b/go/packages/golist.go index d3ead604..8f86b42c 100644 --- a/go/packages/golist.go +++ b/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) } diff --git a/go/packages/packages.go b/go/packages/packages.go index 62a80851..7e799801 100644 --- a/go/packages/packages.go +++ b/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 diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 4babf2c1..f23de18c 100644 --- a/go/packages/packages_test.go +++ b/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) }