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>

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
+// spec.  Every importable package is uniquely identified by its
-// Program.
+// 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.
 //

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
+	// TypeCheckFuncBodies is a predicate over package paths.
-	// paths.  A package for which the predicate is false will
+	// 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
+	// ImportPkgs specifies a set of initial packages to load.
-	// source.  The map keys are package import paths, used to
+	// The map keys are package paths.
-	// locate the package relative to $GOROOT.
 	//
 	// 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
+	path     string        // import path
-	mu       sync.Mutex   // guards the following fields prior to completion
+	info     *PackageInfo  // results of typechecking (including errors)
-	info     *PackageInfo // results of typechecking (including errors)
+	complete chan struct{} // closed to broadcast that info is set.
-	err      error        // reason for failure to create a package
-	complete sync.Cond    // complete condition is that one of info, err is non-nil.
 }
 
 // awaitCompletion blocks until ii is complete,
-// i.e. the info and err fields are safe to inspect without a lock.
+// i.e. the info field is safe to inspect.
-// It is concurrency-safe and idempotent.
 func (ii *importInfo) awaitCompletion() {
-	ii.mu.Lock()
+	<-ii.complete // wait for close
-	for ii.info == nil && ii.err == nil {
-		ii.complete.Wait()
-	}
-	ii.mu.Unlock()
 }
 
 // Complete marks ii as complete.
 // Its info and err fields will not be subsequently updated.
-func (ii *importInfo) Complete(info *PackageInfo, err error) {
+func (ii *importInfo) Complete(info *PackageInfo) {
-	if info == nil && err == nil {
+	if info == nil {
-		panic("Complete(nil, nil)")
+		panic("info == nil")
 	}
-	ii.mu.Lock()
 	ii.info = info
-	ii.err = err
+	close(ii.complete)
-	ii.complete.Broadcast()
+}
-	ii.mu.Unlock()
+
+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) {
+		conf.FindPackage = func(ctxt *build.Context, fromDir, 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.)
 			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) {
+	infos, importErrors := imp.importAll("", conf.Cwd, conf.ImportPkgs)
-		if ii.err != nil {
+	for _, ie := range importErrors {
-			conf.TypeChecker.Error(ii.err) // failed to create package
+		conf.TypeChecker.Error(ie.err) // failed to create package
-			errpkgs = append(errpkgs, ii.path)
+		errpkgs = append(errpkgs, ie.path)
-			continue
+	}
-		}
+	for _, info := range infos {
-		prog.Imported[ii.info.Pkg.Path()] = ii.info
+		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:
+			// awaitCompletion has the postcondition
-			// 	ii.err != nil || ii.info != nil
+			// ii.info != nil.
-			// and we know that ii.err == nil here.
 			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)
+		panic("internal error: unexpected import: " + path)
-	}
-	if ii.err != nil {
-		return nil, ii.err
 	}
 	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 {
+// fromDir is the directory containing the import declaration that
-	result := make([]*importInfo, 0, len(paths))
+// caused these imports.
-	for path := range paths {
+//
-		result = append(result, imp.startLoad(path))
+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 {
+		for _, ii := range pending {
-			deps[path] = true
+			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
+// caller must call awaitCompletion() before accessing its info field.
-// fields.
 //
 // 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 = &importInfo{path: path, complete: make(chan struct{})}
-		ii.complete.L = &ii.mu
 		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(bp *build.Package) *PackageInfo {
-func (imp *importer) load(path string) (*PackageInfo, error) {
+	info := imp.newPackageInfo(bp.ImportPath, bp.Dir)
-	bp, err := imp.conf.FindPackage(imp.conf.build(), path)
-	if err != nil {
-		return nil, err // package not found
-	}
-	info := imp.newPackageInfo(bp.ImportPath)
 	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.

go/loader/loader_go15.go

@@ -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

go/loader/loader_go16.go

@@ -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

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:

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)