go/loader: changes for vendor support
Prior to this change, the loader made the simplifying assumption
that an import path is equal to a package path, that is, a
subdirectory of src/. (This assumption was already false because
relative imports "../foo" are possible, though discouraged.)
Now, an import "foo" may denote "a/vendor/foo" or "b/vendor/foo"
depending on whether it appears beneath a/ or b/. Thus import
paths and package paths are no longer the same, and the directory
containing an import declaration is a necessary input to the
import resolution function.
This change makes the loader hygienic w.r.t. the directory of
each import declaration, and cleans up the terminology.
The only API change is to the FindPackage hook, which most
clients never use.
Details:
- Add a 'fromDir string' parameter to the FindPackage hook function.
- Add a dir field to each PackageInfo.
- doImport (called by go/types) now consists of two steps:
use FindPackage(dir, importPath) to locate the package,
then consult the import map using the canonical package path.
Only the first step can fail.
- Memoize FindPackage.
- Simplify importInfo now that it no longer has to deal with errors.
Replace a condition variable with a channel.
- Use a separate type to report importErrors.
- Rename loadAll to importAll
- Delete the importMode constant.
- Test.
Change-Id: I3defab51bfa12b48b1511a2172fb48dc8e9150e6
Reviewed-on: https://go-review.googlesource.com/18053
Reviewed-by: Robert Griesemer <gri@golang.org>
diff --git a/go/loader/loader.go b/go/loader/loader.go
index 9830527..f7bbd3a 100644
--- a/go/loader/loader.go
+++ b/go/loader/loader.go
@@ -20,6 +20,7 @@
"time"
"golang.org/x/tools/go/ast/astutil"
+ "golang.org/x/tools/go/buildutil"
"golang.org/x/tools/go/types"
)
@@ -45,13 +46,13 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
// 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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
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 @@
// 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 @@
}
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 (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 @@
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 @@
// 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 @@
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 @@
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 @@
}
}
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 @@
// 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 @@
// 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 @@
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 @@
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) newPackageInfo(path string) *PackageInfo {
+func (imp *importer) newPackageInfo(path, dir string) *PackageInfo {
pkg := types.NewPackage(path, "")
info := &PackageInfo{
Pkg: pkg,
@@ -957,6 +988,7 @@
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.
