diff --git a/go/loader/doc.go b/go/loader/doc.go index 1ff4b15de94..600cd1ab70a 100644 --- a/go/loader/doc.go +++ b/go/loader/doc.go @@ -50,6 +50,11 @@ // // CONCEPTS AND TERMINOLOGY // +// The WORKSPACE is the set of packages accessible to the loader. The +// workspace is defined by Config.Build, a *build.Context. The +// default context treats subdirectories of $GOROOT and $GOPATH as +// packages, but this behavior may be overridden. +// // An AD HOC package is one specified as a set of source files on the // command line. In the simplest case, it may consist of a single file // such as $GOROOT/src/net/http/triv.go. @@ -59,14 +64,25 @@ // same directory. (go/build.Package calls these files XTestFiles.) // // An IMPORTABLE package is one that can be referred to by some import -// spec. The Path() of each importable package is unique within a -// Program. +// spec. Every importable package is uniquely identified by its +// PACKAGE PATH or just PATH, a string such as "fmt", "encoding/json", +// or "cmd/vendor/golang.org/x/arch/x86/x86asm". A package path +// typically denotes a subdirectory of the workspace. +// +// An import declaration uses an IMPORT PATH to refer to a package. +// Most import declarations use the package path as the import path. +// +// Due to VENDORING (https://golang.org/s/go15vendor), the +// interpretation of an import path may depend on the directory in which +// it appears. To resolve an import path to a package path, go/build +// must search the enclosing directories for a subdirectory named +// "vendor". // // ad hoc packages and external test packages are NON-IMPORTABLE. The -// Path() of an ad hoc package is inferred from the package +// path of an ad hoc package is inferred from the package // declarations of its files and is therefore not a unique package key. // For example, Config.CreatePkgs may specify two initial ad hoc -// packages both called "main". +// packages, both with path "main". // // An AUGMENTED package is an importable package P plus all the // *_test.go files with same 'package foo' declaration as P. @@ -125,7 +141,7 @@ package loader // Let us define the import dependency graph as follows. Each node is a // list of files passed to (Checker).Files at once. Many of these lists // are the production code of an importable Go package, so those nodes -// are labelled by the package's import path. The remaining nodes are +// are labelled by the package's path. The remaining nodes are // ad hoc packages and lists of in-package *_test.go files that augment // an importable package; those nodes have no label. // diff --git a/go/loader/loader.go b/go/loader/loader.go index 983052767e8..f7bbd3a7e01 100644 --- a/go/loader/loader.go +++ b/go/loader/loader.go @@ -20,6 +20,7 @@ import ( "time" "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/go/buildutil" "golang.org/x/tools/go/types" ) @@ -45,13 +46,13 @@ type Config struct { // The supplied Import function is not used either. TypeChecker types.Config - // TypeCheckFuncBodies is a predicate over package import - // paths. A package for which the predicate is false will + // TypeCheckFuncBodies is a predicate over package paths. + // A package for which the predicate is false will // have its package-level declarations type checked, but not // its function bodies; this can be used to quickly load // dependencies from source. If nil, all func bodies are type // checked. - TypeCheckFuncBodies func(string) bool + TypeCheckFuncBodies func(path string) bool // If Build is non-nil, it is used to locate source packages. // Otherwise &build.Default is used. @@ -84,9 +85,8 @@ type Config struct { // the corresponding elements of the Program.Created slice. CreatePkgs []PkgSpec - // ImportPkgs specifies a set of initial packages to load from - // source. The map keys are package import paths, used to - // locate the package relative to $GOROOT. + // ImportPkgs specifies a set of initial packages to load. + // The map keys are package paths. // // The map value indicates whether to load tests. If true, Load // will add and type-check two lists of files to the package: @@ -96,13 +96,14 @@ type Config struct { ImportPkgs map[string]bool // FindPackage is called during Load to create the build.Package - // for a given import path. If nil, a default implementation + // for a given import path from a given directory. + // If FindPackage is nil, a default implementation // based on ctxt.Import is used. A client may use this hook to // adapt to a proprietary build system that does not follow the // "go build" layout conventions, for example. // // It must be safe to call concurrently from multiple goroutines. - FindPackage func(ctxt *build.Context, importPath string) (*build.Package, error) + FindPackage func(ctxt *build.Context, fromDir, importPath string) (*build.Package, error) } // A PkgSpec specifies a non-importable package to be created by Load. @@ -110,7 +111,7 @@ type Config struct { // Filenames is provided. The path needn't be globally unique. // type PkgSpec struct { - Path string // import path ("" => use package declaration) + Path string // package path ("" => use package declaration) Files []*ast.File // ASTs of already-parsed files Filenames []string // names of files to be parsed } @@ -143,7 +144,7 @@ type Program struct { // dependencies, including incomplete ones. AllPackages map[*types.Package]*PackageInfo - // importMap is the canonical mapping of import paths to + // importMap is the canonical mapping of package paths to // packages. It contains all Imported initial packages, but not // Created ones, and all imported dependencies. importMap map[string]*types.Package @@ -161,6 +162,7 @@ type PackageInfo struct { Files []*ast.File // syntax trees for the package's files Errors []error // non-nil if the package had errors types.Info // type-checker deductions. + dir string // package directory checker *types.Checker // transient type-checker state errorFunc func(error) @@ -375,6 +377,10 @@ type importer struct { progMu sync.Mutex // guards prog prog *Program // the resulting program + // findpkg is a memoization of FindPackage. + findpkgMu sync.Mutex // guards findpkg + findpkg map[[2]string]findpkgValue // key is (fromDir, importPath) + importedMu sync.Mutex // guards imported imported map[string]*importInfo // all imported packages (incl. failures) by import path @@ -387,6 +393,11 @@ type importer struct { graph map[string]map[string]bool } +type findpkgValue struct { + bp *build.Package + err error +} + // importInfo tracks the success or failure of a single import. // // Upon completion, exactly one of info and err is non-nil: @@ -394,35 +405,30 @@ type importer struct { // A successful package may still contain type errors. // type importInfo struct { - path string // import path - mu sync.Mutex // guards the following fields prior to completion - info *PackageInfo // results of typechecking (including errors) - err error // reason for failure to create a package - complete sync.Cond // complete condition is that one of info, err is non-nil. + path string // import path + info *PackageInfo // results of typechecking (including errors) + complete chan struct{} // closed to broadcast that info is set. } // awaitCompletion blocks until ii is complete, -// i.e. the info and err fields are safe to inspect without a lock. -// It is concurrency-safe and idempotent. +// i.e. the info field is safe to inspect. func (ii *importInfo) awaitCompletion() { - ii.mu.Lock() - for ii.info == nil && ii.err == nil { - ii.complete.Wait() - } - ii.mu.Unlock() + <-ii.complete // wait for close } // Complete marks ii as complete. // Its info and err fields will not be subsequently updated. -func (ii *importInfo) Complete(info *PackageInfo, err error) { - if info == nil && err == nil { - panic("Complete(nil, nil)") +func (ii *importInfo) Complete(info *PackageInfo) { + if info == nil { + panic("info == nil") } - ii.mu.Lock() ii.info = info - ii.err = err - ii.complete.Broadcast() - ii.mu.Unlock() + close(ii.complete) +} + +type importError struct { + path string // import path + err error // reason for failure to create a package } // Load creates the initial packages specified by conf.{Create,Import}Pkgs, @@ -455,12 +461,9 @@ func (conf *Config) Load() (*Program, error) { // Install default FindPackage hook using go/build logic. if conf.FindPackage == nil { - conf.FindPackage = func(ctxt *build.Context, path string) (*build.Package, error) { - // TODO(adonovan): cache calls to build.Import - // so we don't do it three times per test package. - // (Note that this is difficult due to vendoring.) + conf.FindPackage = func(ctxt *build.Context, fromDir, path string) (*build.Package, error) { ioLimit <- true - bp, err := ctxt.Import(path, conf.Cwd, importMode) + bp, err := ctxt.Import(path, fromDir, buildutil.AllowVendor) <-ioLimit if _, ok := err.(*build.NoGoError); ok { return bp, nil // empty directory is not an error @@ -479,6 +482,7 @@ func (conf *Config) Load() (*Program, error) { imp := importer{ conf: conf, prog: prog, + findpkg: make(map[[2]string]findpkgValue), imported: make(map[string]*importInfo), start: time.Now(), graph: make(map[string]map[string]bool), @@ -490,24 +494,24 @@ func (conf *Config) Load() (*Program, error) { // Load the initially imported packages and their dependencies, // in parallel. - for _, ii := range imp.loadAll("", conf.ImportPkgs) { - if ii.err != nil { - conf.TypeChecker.Error(ii.err) // failed to create package - errpkgs = append(errpkgs, ii.path) - continue - } - prog.Imported[ii.info.Pkg.Path()] = ii.info + infos, importErrors := imp.importAll("", conf.Cwd, conf.ImportPkgs) + for _, ie := range importErrors { + conf.TypeChecker.Error(ie.err) // failed to create package + errpkgs = append(errpkgs, ie.path) + } + for _, info := range infos { + prog.Imported[info.Pkg.Path()] = info } // Augment the designated initial packages by their tests. // Dependencies are loaded in parallel. var xtestPkgs []*build.Package - for path, augment := range conf.ImportPkgs { + for importPath, augment := range conf.ImportPkgs { if !augment { continue } - bp, err := conf.FindPackage(conf.build(), path) + bp, err := imp.findPackage(conf.Cwd, importPath) if err != nil { // Package not found, or can't even parse package declaration. // Already reported by previous loop; ignore it. @@ -519,12 +523,14 @@ func (conf *Config) Load() (*Program, error) { xtestPkgs = append(xtestPkgs, bp) } + // Consult the cache using the canonical package path. + path := bp.ImportPath imp.importedMu.Lock() // (unnecessary, we're sequential here) ii, ok := imp.imported[path] // Paranoid checks added due to issue #11012. if !ok { // Unreachable. - // The previous loop called loadAll and thus + // The previous loop called importAll and thus // startLoad for each path in ImportPkgs, which // populates imp.imported[path] with a non-zero value. panic(fmt.Sprintf("imported[%q] not found", path)) @@ -536,19 +542,10 @@ func (conf *Config) Load() (*Program, error) { // that at least one of ii.err and ii.info is non-nil. panic(fmt.Sprintf("imported[%q] == nil", path)) } - if ii.err != nil { - // The sole possible cause is failure of the - // FindPackage call in (*importer).load, - // but we rechecked that condition above. - // Perhaps the state of the file system changed - // in between? Seems unlikely. - panic(fmt.Sprintf("imported[%q].err = %v", path, ii.err)) - } if ii.info == nil { // Unreachable. - // Complete has this postcondition: - // ii.err != nil || ii.info != nil - // and we know that ii.err == nil here. + // awaitCompletion has the postcondition + // ii.info != nil. panic(fmt.Sprintf("imported[%q].info = nil", path)) } info := ii.info @@ -567,7 +564,8 @@ func (conf *Config) Load() (*Program, error) { } createPkg := func(path string, files []*ast.File, errs []error) { - info := imp.newPackageInfo(path) + // TODO(adonovan): fix: use dirname of files, not cwd. + info := imp.newPackageInfo(path, conf.Cwd) for _, err := range errs { info.appendError(err) } @@ -750,7 +748,7 @@ func (conf *Config) parsePackageFiles(bp *build.Package, which rune) ([]*ast.Fil // func (imp *importer) doImport(from *PackageInfo, to string) (*types.Package, error) { // Package unsafe is handled specially, and has no PackageInfo. - // TODO(adonovan): move this check into go/types? + // (Let's assume there is no "vendor/unsafe" package.) if to == "unsafe" { return types.Unsafe, nil } @@ -762,14 +760,18 @@ func (imp *importer) doImport(from *PackageInfo, to string) (*types.Package, err from.Pkg.Path()) } + bp, err := imp.findPackage(from.dir, to) + if err != nil { + return nil, err + } + + // Look for the package in the cache using its canonical path. + path := bp.ImportPath imp.importedMu.Lock() - ii := imp.imported[to] + ii := imp.imported[path] imp.importedMu.Unlock() if ii == nil { - panic("internal error: unexpected import: " + to) - } - if ii.err != nil { - return nil, ii.err + panic("internal error: unexpected import: " + path) } if ii.info != nil { return ii.info.Pkg, nil @@ -777,7 +779,7 @@ func (imp *importer) doImport(from *PackageInfo, to string) (*types.Package, err // Import of incomplete package: this indicates a cycle. fromPath := from.Pkg.Path() - if cycle := imp.findPath(to, fromPath); cycle != nil { + if cycle := imp.findPath(path, fromPath); cycle != nil { cycle = append([]string{fromPath}, cycle...) return nil, fmt.Errorf("import cycle: %s", strings.Join(cycle, " -> ")) } @@ -785,16 +787,46 @@ func (imp *importer) doImport(from *PackageInfo, to string) (*types.Package, err panic("internal error: import of incomplete (yet acyclic) package: " + fromPath) } -// loadAll loads, parses, and type-checks the specified packages in +// findPackage locates the package denoted by the importPath in the +// specified directory. +func (imp *importer) findPackage(fromDir, importPath string) (*build.Package, error) { + // TODO(adonovan): opt: non-blocking duplicate-suppressing cache. + // i.e. don't hold the lock around FindPackage. + key := [2]string{fromDir, importPath} + imp.findpkgMu.Lock() + defer imp.findpkgMu.Unlock() + v, ok := imp.findpkg[key] + if !ok { + bp, err := imp.conf.FindPackage(imp.conf.build(), fromDir, importPath) + v = findpkgValue{bp, err} + imp.findpkg[key] = v + } + return v.bp, v.err +} + +// importAll loads, parses, and type-checks the specified packages in // parallel and returns their completed importInfos in unspecified order. // -// fromPath is the import path of the importing package, if it is +// fromPath is the package path of the importing package, if it is // importable, "" otherwise. It is used for cycle detection. // -func (imp *importer) loadAll(fromPath string, paths map[string]bool) []*importInfo { - result := make([]*importInfo, 0, len(paths)) - for path := range paths { - result = append(result, imp.startLoad(path)) +// fromDir is the directory containing the import declaration that +// caused these imports. +// +func (imp *importer) importAll(fromPath, fromDir string, imports map[string]bool) (infos []*PackageInfo, errors []importError) { + // TODO(adonovan): opt: do the loop in parallel once + // findPackage is non-blocking. + var pending []*importInfo + for importPath := range imports { + bp, err := imp.findPackage(fromDir, importPath) + if err != nil { + errors = append(errors, importError{ + path: importPath, + err: err, + }) + continue + } + pending = append(pending, imp.startLoad(bp)) } if fromPath != "" { @@ -808,13 +840,13 @@ func (imp *importer) loadAll(fromPath string, paths map[string]bool) []*importIn deps = make(map[string]bool) imp.graph[fromPath] = deps } - for path := range paths { - deps[path] = true + for _, ii := range pending { + deps[ii.path] = true } imp.graphMu.Unlock() } - for _, ii := range result { + for _, ii := range pending { if fromPath != "" { if cycle := imp.findPath(ii.path, fromPath); cycle != nil { // Cycle-forming import: we must not await its @@ -832,8 +864,10 @@ func (imp *importer) loadAll(fromPath string, paths map[string]bool) []*importIn } } ii.awaitCompletion() + infos = append(infos, ii.info) } - return result + + return infos, errors } // findPath returns an arbitrary path from 'from' to 'to' in the import @@ -866,20 +900,20 @@ func (imp *importer) findPath(from, to string) []string { // specified package and its dependencies, if it has not already begun. // // It returns an importInfo, not necessarily in a completed state. The -// caller must call awaitCompletion() before accessing its info and err -// fields. +// caller must call awaitCompletion() before accessing its info field. // // startLoad is concurrency-safe and idempotent. // -func (imp *importer) startLoad(path string) *importInfo { +func (imp *importer) startLoad(bp *build.Package) *importInfo { + path := bp.ImportPath imp.importedMu.Lock() ii, ok := imp.imported[path] if !ok { - ii = &importInfo{path: path} - ii.complete.L = &ii.mu + ii = &importInfo{path: path, complete: make(chan struct{})} imp.imported[path] = ii go func() { - ii.Complete(imp.load(path)) + info := imp.load(bp) + ii.Complete(info) }() } imp.importedMu.Unlock() @@ -889,13 +923,8 @@ func (imp *importer) startLoad(path string) *importInfo { // load implements package loading by parsing Go source files // located by go/build. -// -func (imp *importer) load(path string) (*PackageInfo, error) { - bp, err := imp.conf.FindPackage(imp.conf.build(), path) - if err != nil { - return nil, err // package not found - } - info := imp.newPackageInfo(bp.ImportPath) +func (imp *importer) load(bp *build.Package) *PackageInfo { + info := imp.newPackageInfo(bp.ImportPath, bp.Dir) info.Importable = true files, errs := imp.conf.parsePackageFiles(bp, 'g') for _, err := range errs { @@ -905,10 +934,10 @@ func (imp *importer) load(path string) (*PackageInfo, error) { imp.addFiles(info, files, true) imp.progMu.Lock() - imp.prog.importMap[path] = info.Pkg + imp.prog.importMap[bp.ImportPath] = info.Pkg imp.progMu.Unlock() - return info, nil + return info } // addFiles adds and type-checks the specified files to info, loading @@ -927,7 +956,9 @@ func (imp *importer) addFiles(info *PackageInfo, files []*ast.File, cycleCheck b if cycleCheck { fromPath = info.Pkg.Path() } - imp.loadAll(fromPath, scanImports(files)) + // TODO(adonovan): opt: make the caller do scanImports. + // Callers with a build.Package can skip it. + imp.importAll(fromPath, info.dir, scanImports(files)) if trace { fmt.Fprintf(os.Stderr, "%s: start %q (%d)\n", @@ -944,7 +975,7 @@ func (imp *importer) addFiles(info *PackageInfo, files []*ast.File, cycleCheck b } } -func (imp *importer) newPackageInfo(path string) *PackageInfo { +func (imp *importer) newPackageInfo(path, dir string) *PackageInfo { pkg := types.NewPackage(path, "") info := &PackageInfo{ Pkg: pkg, @@ -957,6 +988,7 @@ func (imp *importer) newPackageInfo(path string) *PackageInfo { Selections: make(map[*ast.SelectorExpr]*types.Selection), }, errorFunc: imp.conf.TypeChecker.Error, + dir: dir, } // Copy the types.Config so we can vary it across PackageInfos. diff --git a/go/loader/loader_go15.go b/go/loader/loader_go15.go deleted file mode 100644 index 5dff7ca7edc..00000000000 --- a/go/loader/loader_go15.go +++ /dev/null @@ -1,11 +0,0 @@ -// 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. - -// +build !go1.6 - -package loader - -import "go/build" - -var importMode build.ImportMode = 0 diff --git a/go/loader/loader_go16.go b/go/loader/loader_go16.go deleted file mode 100644 index cd310637360..00000000000 --- a/go/loader/loader_go16.go +++ /dev/null @@ -1,11 +0,0 @@ -// 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. - -// +build go1.6 - -package loader - -import "go/build" - -var importMode build.ImportMode = build.AllowVendor diff --git a/go/loader/loader_test.go b/go/loader/loader_test.go index 602590e9f89..80823444e89 100644 --- a/go/loader/loader_test.go +++ b/go/loader/loader_test.go @@ -393,6 +393,39 @@ func TestCwd(t *testing.T) { } } +func TestLoad_vendor(t *testing.T) { + pkgs := map[string]string{ + "a": `package a; import _ "x"`, + "a/vendor": ``, // mkdir a/vendor + "a/vendor/x": `package xa`, + "b": `package b; import _ "x"`, + "b/vendor": ``, // mkdir b/vendor + "b/vendor/x": `package xb`, + "c": `package c; import _ "x"`, + "x": `package xc`, + } + conf := loader.Config{Build: fakeContext(pkgs)} + conf.Import("a") + conf.Import("b") + conf.Import("c") + + prog, err := conf.Load() + if err != nil { + t.Fatal(err) + } + + // Check that a, b, and c see different versions of x. + for _, r := range "abc" { + name := string(r) + got := prog.Package(name).Pkg.Imports()[0] + want := "x" + name + if got.Name() != want { + t.Errorf("package %s import %q = %s, want %s", + name, "x", got.Name(), want) + } + } +} + // TODO(adonovan): more Load tests: // // failures: diff --git a/go/loader/util.go b/go/loader/util.go index 3b648565715..4e014627390 100644 --- a/go/loader/util.go +++ b/go/loader/util.go @@ -88,7 +88,7 @@ func parseFiles(fset *token.FileSet, ctxt *build.Context, displayPath func(strin return parsed, errors } -// scanImports returns the set of all package import paths from all +// scanImports returns the set of all import paths from all // import specs in the specified files. func scanImports(files []*ast.File) map[string]bool { imports := make(map[string]bool)