go/loader: improve robustness in face of input errors

    Before this change, many kinds of error would cause the loader to stop.
    making it brittle when analyzing large codebases, as in "godoc -analysis".

    This change moves operations that used to occur during
    configuration---(*build.Context).Import, loading, and parsing of
    initial packages---into the Load call, and ensures that all failures
    during Loading are reported at the end so that the maximum amount of
    progress is made.

    Also: redesign the tests and add many new cases.

Change-Id: Ia8cd99416af7c5d4a5fe133908adfa83676d401f
Reviewed-on: https://go-review.googlesource.com/3626
Reviewed-by: Robert Griesemer <gri@golang.org>

13 files changed, 485 insertions, 243 deletions

diff --git a/cmd/eg/eg.go b/cmd/eg/eg.go
index 0d9cf91..cc3ea64 100644
--- a/cmd/eg/eg.go
+++ b/cmd/eg/eg.go
@@ -61,9 +61,7 @@ func doMain() error {
 	}
 
 	// The first Created package is the template.
-	if err := conf.CreateFromFilenames("template", *templateFlag); err != nil {
-		return err //  e.g. "foo.go:1: syntax error"
-	}
+	conf.CreateFromFilenames("template", *templateFlag)
 
 	if len(args) == 0 {
 		fmt.Fprint(os.Stderr, usage)
diff --git a/cmd/ssadump/main.go b/cmd/ssadump/main.go
index 2050780..1901b30 100644
--- a/cmd/ssadump/main.go
+++ b/cmd/ssadump/main.go
@@ -203,7 +203,7 @@ func doMain() error {
 		}
 
 		if runtime.GOARCH != build.Default.GOARCH {
-			return fmt.Errorf("cross-interpretation is not yet supported (target has GOARCH %s, interpreter has %s)",
+			return fmt.Errorf("cross-interpretation is not supported (target has GOARCH %s, interpreter has %s)",
 				build.Default.GOARCH, runtime.GOARCH)
 		}
 
diff --git a/go/loader/loader.go b/go/loader/loader.go
index 26a6ae2..0fdda05 100644
--- a/go/loader/loader.go
+++ b/go/loader/loader.go
@@ -27,19 +27,19 @@
 //
 //      // Parse the specified files and create an ad-hoc package with path "foo".
 //      // All files must have the same 'package' declaration.
-//      err := conf.CreateFromFilenames("foo", "foo.go", "bar.go")
+//      conf.CreateFromFilenames("foo", "foo.go", "bar.go")
 //
 //      // Create an ad-hoc package with path "foo" from
 //      // the specified already-parsed files.
 //      // All ASTs must have the same 'package' declaration.
-//      err := conf.CreateFromFiles("foo", parsedFiles)
+//      conf.CreateFromFiles("foo", parsedFiles)
 //
 //      // Add "runtime" to the set of packages to be loaded.
 //      conf.Import("runtime")
 //
 //      // Adds "fmt" and "fmt_test" to the set of packages
 //      // to be loaded.  "fmt" will include *_test.go files.
-//      err := conf.ImportWithTests("fmt")
+//      conf.ImportWithTests("fmt")
 //
 //      // Finally, load all the packages specified by the configuration.
 //      prog, err := conf.Load()
@@ -182,15 +182,9 @@ package loader
 // The result of using concurrency is about a 2.5x speedup for stdlib_test.
 
 // TODO(adonovan):
-// - (*Config).ParseFile is very handy, but feels like feature creep.
-//   (*Config).CreateFromFiles has a nasty precondition.
 // - cache the calls to build.Import so we don't do it three times per
 //   test package.
 // - Thorough overhaul of package documentation.
-// - Certain errors (e.g. parse error in x_test.go files, or failure to
-//   import an initial package) still cause Load() to fail hard.
-//   Fix that.  (It's tricky because of the way x_test files are parsed
-//   eagerly.)
 
 import (
 	"errors"
@@ -200,6 +194,7 @@ import (
 	"go/parser"
 	"go/token"
 	"os"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -215,7 +210,7 @@ const trace = false // show timing info for type-checking
 // The zero value for Config is a ready-to-use default configuration.
 type Config struct {
 	// Fset is the file set for the parser to use when loading the
-	// program.  If nil, it will be lazily initialized by any
+	// program.  If nil, it may be lazily initialized by any
 	// method of Config.
 	Fset *token.FileSet
 
@@ -280,21 +275,19 @@ type Config struct {
 	AllowErrors bool
 
 	// CreatePkgs specifies a list of non-importable initial
-	// packages to create.  Each element specifies a list of
-	// parsed files to be type-checked into a new package, and a
-	// path for that package.  If the path is "", the package's
-	// name will be used instead.  The path needn't be globally
-	// unique.
-	//
-	// The resulting packages will appear in the corresponding
-	// elements of the Program.Created slice.
-	CreatePkgs []CreatePkg
+	// packages to create.  The resulting packages will appear in
+	// 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.  The corresponding
-	// values indicate whether to augment the package by *_test.go
-	// files in a second pass.
+	// 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:
+	// non-test files followed by in-package *_test.go files.  In
+	// addition, it will append the external test package (if any)
+	// to Program.Created.
 	ImportPkgs map[string]bool
 
 	// PackageCreated is a hook called when a types.Package
@@ -311,9 +304,14 @@ type Config struct {
 	PackageCreated func(*types.Package)
 }
 
-type CreatePkg struct {
-	Path  string // the import path of the resulting (non-importable) types.Package
-	Files []*ast.File
+// A PkgSpec specifies a non-importable package to be created by Load.
+// Files are processed first, but typically only one of Files and
+// Filenames is provided.  The path needn't be globally unique.
+//
+type PkgSpec struct {
+	Path      string      // import path ("" => use package declaration)
+	Files     []*ast.File // ASTs of already-parsed files
+	Filenames []string    // names of files to be parsed
 }
 
 // A Program is a Go program loaded from source or binary
@@ -321,8 +319,10 @@ type CreatePkg struct {
 type Program struct {
 	Fset *token.FileSet // the file set for this program
 
-	// Created[i] contains the initial package whose ASTs were
-	// supplied by Config.CreatePkgs[i].
+	// Created[i] contains the initial package whose ASTs or
+	// filenames were supplied by Config.CreatePkgs[i], followed by
+	// the external test package, if any, of each package in
+	// Config.ImportPkgs ordered by ImportPath.
 	Created []*PackageInfo
 
 	// Imported contains the initially imported packages,
@@ -376,8 +376,12 @@ func (conf *Config) fset() *token.FileSet {
 	return conf.Fset
 }
 
-// ParseFile is a convenience function that invokes the parser using
-// the Config's FileSet, which is initialized if nil.
+// ParseFile is a convenience function (intended for testing) that invokes
+// the parser using the Config's FileSet, which is initialized if nil.
+//
+// src specifies the parser input as a string, []byte, or io.Reader, and
+// filename is its apparent name.  If src is nil, the contents of
+// filename are read from the file system.
 //
 func (conf *Config) ParseFile(filename string, src interface{}) (*ast.File, error) {
 	// TODO(adonovan): use conf.build() etc like parseFiles does.
@@ -410,9 +414,6 @@ It may take one of two forms:
    import path may denote two packages.  (Whether this behaviour is
    enabled is tool-specific, and may depend on additional flags.)
 
-   Due to current limitations in the type-checker, only the first
-   import path of the command line will contribute any tests.
-
 A '--' argument terminates the list of packages.
 `
 
@@ -424,7 +425,11 @@ A '--' argument terminates the list of packages.
 // set of initial packages to be specified; see FromArgsUsage message
 // for details.
 //
-func (conf *Config) FromArgs(args []string, xtest bool) (rest []string, err error) {
+// Only superficial errors are reported at this stage; errors dependent
+// on I/O are detected during Load.
+//
+func (conf *Config) FromArgs(args []string, xtest bool) ([]string, error) {
+	var rest []string
 	for i, arg := range args {
 		if arg == "--" {
 			rest = args[i+1:]
@@ -441,53 +446,35 @@ func (conf *Config) FromArgs(args []string, xtest bool) (rest []string, err erro
 				return nil, fmt.Errorf("named files must be .go files: %s", arg)
 			}
 		}
-		err = conf.CreateFromFilenames("", args...)
+		conf.CreateFromFilenames("", args...)
 	} else {
 		// Assume args are directories each denoting a
 		// package and (perhaps) an external test, iff xtest.
 		for _, arg := range args {
 			if xtest {
-				err = conf.ImportWithTests(arg)
-				if err != nil {
-					break
-				}
+				conf.ImportWithTests(arg)
 			} else {
 				conf.Import(arg)
 			}
 		}
 	}
 
-	return
+	return rest, nil
 }
 
-// CreateFromFilenames is a convenience function that parses the
-// specified *.go files and adds a package entry for them to
-// conf.CreatePkgs.
-//
-// It fails if any file could not be loaded or parsed.
-// TODO(adonovan): make it not return an error, by making CreatePkg
-// store filenames and defer parsing until Load.
+// CreateFromFilenames is a convenience function that adds
+// a conf.CreatePkgs entry to create a package of the specified *.go
+// files.
 //
-func (conf *Config) CreateFromFilenames(path string, filenames ...string) error {
-	files, errs := parseFiles(conf.fset(), conf.build(), nil, ".", filenames, conf.ParserMode)
-	if len(errs) > 0 {
-		return errs[0]
-	}
-	conf.CreateFromFiles(path, files...)
-	return nil
+func (conf *Config) CreateFromFilenames(path string, filenames ...string) {
+	conf.CreatePkgs = append(conf.CreatePkgs, PkgSpec{Path: path, Filenames: filenames})
 }
 
-// CreateFromFiles is a convenience function that adds a CreatePkgs
+// CreateFromFiles is a convenience function that adds a conf.CreatePkgs
 // entry to create package of the specified path and parsed files.
 //
-// Precondition: conf.Fset is non-nil and was the fileset used to parse
-// the files.  (e.g. the files came from conf.ParseFile().)
-//
 func (conf *Config) CreateFromFiles(path string, files ...*ast.File) {
-	if conf.Fset == nil {
-		panic("nil Fset")
-	}
-	conf.CreatePkgs = append(conf.CreatePkgs, CreatePkg{path, files})
+	conf.CreatePkgs = append(conf.CreatePkgs, PkgSpec{Path: path, Files: files})
 }
 
 // ImportWithTests is a convenience function that adds path to
@@ -500,45 +487,21 @@ func (conf *Config) CreateFromFiles(path string, files ...*ast.File) {
 // declaration, an additional package comprising just those files will
 // be added to CreatePkgs.
 //
-func (conf *Config) ImportWithTests(path string) error {
-	if path == "unsafe" {
-		return nil // ignore; not a real package
-	}
-	conf.Import(path)
-
-	// Load the external test package.
-	bp, err := conf.findSourcePackage(path)
-	if err != nil {
-		return err // package not found
-	}
-	xtestFiles, errs := conf.parsePackageFiles(bp, 'x')
-	if len(errs) > 0 {
-		// TODO(adonovan): fix: parse errors in x_test.go files
-		// cause FromArgs() to fail completely.
-		return errs[0] // I/O or parse error
-	}
-	if len(xtestFiles) > 0 {
-		conf.CreateFromFiles(path+"_test", xtestFiles...)
-	}
-
-	// Mark the non-xtest package for augmentation with
-	// in-package *_test.go files when we import it below.
-	conf.ImportPkgs[path] = true
-	return nil
-}
+func (conf *Config) ImportWithTests(path string) { conf.addImport(path, true) }
 
 // Import is a convenience function that adds path to ImportPkgs, the
 // set of initial packages that will be imported from source.
 //
-func (conf *Config) Import(path string) {
+func (conf *Config) Import(path string) { conf.addImport(path, false) }
+
+func (conf *Config) addImport(path string, tests bool) {
 	if path == "unsafe" {
 		return // ignore; not a real package
 	}
 	if conf.ImportPkgs == nil {
 		conf.ImportPkgs = make(map[string]bool)
 	}
-	// Subtle: adds value 'false' unless value is already true.
-	conf.ImportPkgs[path] = conf.ImportPkgs[path] // unaugmented source package
+	conf.ImportPkgs[path] = conf.ImportPkgs[path] || tests
 }
 
 // PathEnclosingInterval returns the PackageInfo and ast.Node that
@@ -679,59 +642,91 @@ func (conf *Config) Load() (*Program, error) {
 
 	// -- loading proper (concurrent phase) --------------------------------
 
+	var errpkgs []string // packages that contained errors
+
 	// Load the initially imported packages and their dependencies,
 	// in parallel.
 	for _, ii := range imp.loadAll("", conf.ImportPkgs) {
 		if ii.err != nil {
-			return nil, ii.err // failed to create package
+			conf.TypeChecker.Error(ii.err) // failed to create package
+			errpkgs = append(errpkgs, ii.path)
+			continue
 		}
 		prog.Imported[ii.info.Pkg.Path()] = ii.info
 	}
 
-	// Augment the initial packages that need it.
+	// Augment the designated initial packages by their tests.
 	// Dependencies are loaded in parallel.
+	var xtestPkgs []*build.Package
 	for path, augment := range conf.ImportPkgs {
-		if augment {
-			// Find and create the actual package.
-			bp, err := conf.findSourcePackage(path)
-			if err != nil {
-				// "Can't happen" because of previous loop.
-				return nil, err // package not found
-			}
+		if !augment {
+			continue
+		}
 
-			imp.importedMu.Lock()           // (unnecessary, we're sequential here)
-			info := imp.imported[path].info // must be non-nil, see above
-			imp.importedMu.Unlock()
+		bp, err := conf.findSourcePackage(path)
+		if err != nil {
+			// Package not found, or can't even parse package declaration.
+			// Already reported by previous loop; ignore it.
+			continue
+		}
 
-			files, errs := imp.conf.parsePackageFiles(bp, 't')
-			for _, err := range errs {
-				info.appendError(err)
-			}
-			// The test files augmenting package P cannot be imported,
-			// but may import packages that import P,
-			// so we must disable the cycle check.
-			imp.addFiles(info, files, false)
+		// Needs external test package?
+		if len(bp.XTestGoFiles) > 0 {
+			xtestPkgs = append(xtestPkgs, bp)
 		}
-	}
 
-	// CreatePkgs includes all external test packages,
-	// so they must be processed after augmentation.
-	// Dependencies are loaded in parallel.
-	for _, create := range conf.CreatePkgs {
-		path := create.Path
-		if create.Path == "" && len(create.Files) > 0 {
-			path = create.Files[0].Name.Name
+		imp.importedMu.Lock()           // (unnecessary, we're sequential here)
+		info := imp.imported[path].info // must be non-nil, see above
+		imp.importedMu.Unlock()
+
+		// Parse the in-package test files.
+		files, errs := imp.conf.parsePackageFiles(bp, 't')
+		for _, err := range errs {
+			info.appendError(err)
 		}
+
+		// The test files augmenting package P cannot be imported,
+		// but may import packages that import P,
+		// so we must disable the cycle check.
+		imp.addFiles(info, files, false)
+	}
+
+	createPkg := func(path string, files []*ast.File, errs []error) {
 		info := imp.newPackageInfo(path)
-		// Ad-hoc packages are non-importable; no cycle check is needed.
-		imp.addFiles(info, create.Files, false)
+		for _, err := range errs {
+			info.appendError(err)
+		}
+
+		// Ad-hoc packages are non-importable,
+		// so no cycle check is needed.
+		// addFiles loads dependencies in parallel.
+		imp.addFiles(info, files, false)
 		prog.Created = append(prog.Created, info)
 	}
 
+	// Create packages specified by conf.CreatePkgs.
+	for _, cp := range conf.CreatePkgs {
+		files, errs := parseFiles(conf.fset(), conf.build(), nil, ".", cp.Filenames, conf.ParserMode)
+		files = append(files, cp.Files...)
+
+		path := cp.Path
+		if path == "" && len(files) > 0 {
+			path = files[0].Name.Name
+		}
+		createPkg(path, files, errs)
+	}
+
+	// Create external test packages.
+	sort.Sort(byImportPath(xtestPkgs))
+	for _, bp := range xtestPkgs {
+		files, errs := imp.conf.parsePackageFiles(bp, 'x')
+		createPkg(bp.ImportPath+"_test", files, errs)
+	}
+
 	// -- finishing up (sequential) ----------------------------------------
 
 	if len(prog.Imported)+len(prog.Created) == 0 {
-		return nil, errors.New("no initial packages were specified")
+		return nil, errors.New("no initial packages were loaded")
 	}
 
 	// Create infos for indirectly imported packages.
@@ -749,7 +744,6 @@ func (conf *Config) Load() (*Program, error) {
 
 	if !conf.AllowErrors {
 		// Report errors in indirectly imported packages.
-		var errpkgs []string
 		for _, info := range prog.AllPackages {
 			if len(info.Errors) > 0 {
 				errpkgs = append(errpkgs, info.Pkg.Path())
@@ -771,6 +765,12 @@ func (conf *Config) Load() (*Program, error) {
 	return prog, nil
 }
 
+type byImportPath []*build.Package
+
+func (b byImportPath) Len() int           { return len(b) }
+func (b byImportPath) Less(i, j int) bool { return b[i].ImportPath < b[j].ImportPath }
+func (b byImportPath) Swap(i, j int)      { b[i], b[j] = b[j], b[i] }
+
 // markErrorFreePackages sets the TransitivelyErrorFree flag on all
 // applicable packages.
 func markErrorFreePackages(allPackages map[*types.Package]*PackageInfo) {
diff --git a/go/loader/loader_test.go b/go/loader/loader_test.go
index 5cc3bdb..2972ff5 100644
--- a/go/loader/loader_test.go
+++ b/go/loader/loader_test.go
@@ -5,8 +5,8 @@
 package loader_test
 
 import (
-	"fmt"
 	"go/build"
+	"reflect"
 	"sort"
 	"strings"
 	"sync"
@@ -16,112 +16,320 @@ import (
 	"golang.org/x/tools/go/loader"
 )
 
-func loadFromArgs(args []string) (prog *loader.Program, rest []string, err error) {
-	conf := &loader.Config{}
-	rest, err = conf.FromArgs(args, true)
+// TestFromArgs checks that conf.FromArgs populates conf correctly.
+// It does no I/O.
+func TestFromArgs(t *testing.T) {
+	type result struct {
+		Err        string
+		Rest       []string
+		ImportPkgs map[string]bool
+		CreatePkgs []loader.PkgSpec
+	}
+	for _, test := range []struct {
+		args  []string
+		tests bool
+		want  result
+	}{
+		// Mix of existing and non-existent packages.
+		{
+			args: []string{"nosuchpkg", "errors"},
+			want: result{
+				ImportPkgs: map[string]bool{"errors": false, "nosuchpkg": false},
+			},
+		},
+		// Same, with -test flag.
+		{
+			args:  []string{"nosuchpkg", "errors"},
+			tests: true,
+			want: result{
+				ImportPkgs: map[string]bool{"errors": true, "nosuchpkg": true},
+			},
+		},
+		// Surplus arguments.
+		{
+			args: []string{"fmt", "errors", "--", "surplus"},
+			want: result{
+				Rest:       []string{"surplus"},
+				ImportPkgs: map[string]bool{"errors": false, "fmt": false},
+			},
+		},
+		// Ad hoc package specified as *.go files.
+		{
+			args: []string{"foo.go", "bar.go"},
+			want: result{CreatePkgs: []loader.PkgSpec{{
+				Filenames: []string{"foo.go", "bar.go"},
+			}}},
+		},
+		// Mixture of *.go and import paths.
+		{
+			args: []string{"foo.go", "fmt"},
+			want: result{
+				Err: "named files must be .go files: fmt",
+			},
+		},
+	} {
+		var conf loader.Config
+		rest, err := conf.FromArgs(test.args, test.tests)
+		got := result{
+			Rest:       rest,
+			ImportPkgs: conf.ImportPkgs,
+			CreatePkgs: conf.CreatePkgs,
+		}
+		if err != nil {
+			got.Err = err.Error()
+		}
+		if !reflect.DeepEqual(got, test.want) {
+			t.Errorf("FromArgs(%q) = %+v, want %+v", test.args, got, test.want)
+		}
+	}
+}
+
+func TestLoad_NoInitialPackages(t *testing.T) {
+	var conf loader.Config
+
+	const wantErr = "no initial packages were loaded"
+
+	prog, err := conf.Load()
 	if err == nil {
-		prog, err = conf.Load()
+		t.Errorf("Load succeeded unexpectedly, want %q", wantErr)
+	} else if err.Error() != wantErr {
+		t.Errorf("Load failed with wrong error %q, want %q", err, wantErr)
+	}
+	if prog != nil {
+		t.Errorf("Load unexpectedly returned a Program")
 	}
-	return
 }
 
-func TestLoadFromArgs(t *testing.T) {
-	// Failed load: bad first import path causes parsePackageFiles to fail.
-	args := []string{"nosuchpkg", "errors"}
-	if _, _, err := loadFromArgs(args); err == nil {
-		t.Errorf("loadFromArgs(%q) succeeded, want failure", args)
-	} else {
-		// cannot find package: ok.
+func TestLoad_MissingInitialPackage(t *testing.T) {
+	var conf loader.Config
+	conf.Import("nosuchpkg")
+	conf.Import("errors")
+
+	const wantErr = "couldn't load packages due to errors: nosuchpkg"
+
+	prog, err := conf.Load()
+	if err == nil {
+		t.Errorf("Load succeeded unexpectedly, want %q", wantErr)
+	} else if err.Error() != wantErr {
+		t.Errorf("Load failed with wrong error %q, want %q", err, wantErr)
+	}
+	if prog != nil {
+		t.Errorf("Load unexpectedly returned a Program")
 	}
+}
+
+func TestLoad_MissingInitialPackage_AllowErrors(t *testing.T) {
+	var conf loader.Config
+	conf.AllowErrors = true
+	conf.Import("nosuchpkg")
+	conf.ImportWithTests("errors")
 
-	// Failed load: bad second import path proceeds to doImport0, which fails.
-	args = []string{"errors", "nosuchpkg"}
-	if _, _, err := loadFromArgs(args); err == nil {
-		t.Errorf("loadFromArgs(%q) succeeded, want failure", args)
-	} else {
-		// cannot find package: ok
+	prog, err := conf.Load()
+	if err != nil {
+		t.Errorf("Load failed unexpectedly: %v", err)
+	}
+	if prog == nil {
+		t.Fatalf("Load returned a nil Program")
+	}
+	if got, want := created(prog), "errors_test"; got != want {
+		t.Errorf("Created = %s, want %s", got, want)
+	}
+	if got, want := imported(prog), "errors"; got != want {
+		t.Errorf("Imported = %s, want %s", got, want)
 	}
+}
+
+func TestLoad_ParseError(t *testing.T) {
+	var conf loader.Config
+	conf.CreateFromFilenames("badpkg", "testdata/badpkgdecl.go")
 
-	// Successful load.
-	args = []string{"fmt", "errors", "--", "surplus"}
-	prog, rest, err := loadFromArgs(args)
+	const wantErr = "couldn't load packages due to errors: badpkg"
+
+	prog, err := conf.Load()
+	if err == nil {
+		t.Errorf("Load succeeded unexpectedly, want %q", wantErr)
+	} else if err.Error() != wantErr {
+		t.Errorf("Load failed with wrong error %q, want %q", err, wantErr)
+	}
+	if prog != nil {
+		t.Errorf("Load unexpectedly returned a Program")
+	}
+}
+
+func TestLoad_ParseError_AllowErrors(t *testing.T) {
+	var conf loader.Config
+	conf.AllowErrors = true
+	conf.CreateFromFilenames("badpkg", "testdata/badpkgdecl.go")
+
+	prog, err := conf.Load()
 	if err != nil {
-		t.Fatalf("loadFromArgs(%q) failed: %s", args, err)
+		t.Errorf("Load failed unexpectedly: %v", err)
+	}
+	if prog == nil {
+		t.Fatalf("Load returned a nil Program")
 	}
-	if got, want := fmt.Sprint(rest), "[surplus]"; got != want {
-		t.Errorf("loadFromArgs(%q) rest: got %s, want %s", args, got, want)
+	if got, want := created(prog), "badpkg"; got != want {
+		t.Errorf("Created = %s, want %s", got, want)
 	}
-	// Check list of Created packages.
-	var pkgnames []string
-	for _, info := range prog.Created {
-		pkgnames = append(pkgnames, info.Pkg.Path())
+
+	badpkg := prog.Created[0]
+	if len(badpkg.Files) != 1 {
+		t.Errorf("badpkg has %d files, want 1", len(badpkg.Files))
 	}
-	// All import paths may contribute tests.
-	if got, want := fmt.Sprint(pkgnames), "[fmt_test errors_test]"; got != want {
-		t.Errorf("Created: got %s, want %s", got, want)
+	wantErr := "testdata/badpkgdecl.go:1:34: expected 'package', found 'EOF'"
+	if !hasError(badpkg.Errors, wantErr) {
+		t.Errorf("badpkg.Errors = %v, want %s", badpkg.Errors, wantErr)
 	}
+}
+
+func TestLoad_FromSource_Success(t *testing.T) {
+	var conf loader.Config
+	conf.CreateFromFilenames("P", "testdata/a.go", "testdata/b.go")
 
-	// Check set of Imported packages.
-	pkgnames = nil
-	for path := range prog.Imported {
-		pkgnames = append(pkgnames, path)
+	prog, err := conf.Load()
+	if err != nil {
+		t.Errorf("Load failed unexpectedly: %v", err)
 	}
-	sort.Strings(pkgnames)
-	// All import paths may contribute tests.
-	if got, want := fmt.Sprint(pkgnames), "[errors fmt]"; got != want {
-		t.Errorf("Loaded: got %s, want %s", got, want)
+	if prog == nil {
+		t.Fatalf("Load returned a nil Program")
 	}
+	if got, want := created(prog), "P"; got != want {
+		t.Errorf("Created = %s, want %s", got, want)
+	}
+}
 
+func TestLoad_FromImports_Success(t *testing.T) {
+	var conf loader.Config
+	conf.ImportWithTests("fmt")
+	conf.ImportWithTests("errors")
+
+	prog, err := conf.Load()
+	if err != nil {
+		t.Errorf("Load failed unexpectedly: %v", err)
+	}
+	if prog == nil {
+		t.Fatalf("Load returned a nil Program")
+	}
+	if got, want := created(prog), "errors_test fmt_test"; got != want {
+		t.Errorf("Created = %q, want %s", got, want)
+	}
+	if got, want := imported(prog), "errors fmt"; got != want {
+		t.Errorf("Imported = %s, want %s", got, want)
+	}
 	// Check set of transitive packages.
 	// There are >30 and the set may grow over time, so only check a few.
-	all := map[string]struct{}{}
-	for _, info := range prog.AllPackages {
-		all[info.Pkg.Path()] = struct{}{}
+	want := map[string]bool{
+		"strings": true,
+		"time":    true,
+		"runtime": true,
+		"testing": true,
+		"unicode": true,
 	}
-	want := []string{"strings", "time", "runtime", "testing", "unicode"}
-	for _, w := range want {
-		if _, ok := all[w]; !ok {
-			t.Errorf("AllPackages: want element %s, got set %v", w, all)
-		}
+	for _, path := range all(prog) {
+		delete(want, path)
+	}
+	if len(want) > 0 {
+		t.Errorf("AllPackages is missing these keys: %q", keys(want))
 	}
 }
 
-func TestLoadFromArgsSource(t *testing.T) {
-	// mixture of *.go/non-go.
-	args := []string{"testdata/a.go", "fmt"}
-	prog, _, err := loadFromArgs(args)
-	if err == nil {
-		t.Errorf("loadFromArgs(%q) succeeded, want failure", args)
-	} else {
-		// "named files must be .go files: fmt": ok
+func TestLoad_MissingIndirectImport(t *testing.T) {
+	pkgs := map[string]string{
+		"a": `package a; import _ "b"`,
+		"b": `package b; import _ "c"`,
 	}
-
-	// successful load
-	args = []string{"testdata/a.go", "testdata/b.go"}
-	prog, _, err = loadFromArgs(args)
-	if err != nil {
-		t.Fatalf("loadFromArgs(%q) failed: %s", args, err)
+	conf := loader.Config{
+		SourceImports: true,
+		Build:         fakeContext(pkgs),
 	}
-	if len(prog.Created) != 1 {
-		t.Errorf("loadFromArgs(%q): got %d items, want 1", args, len(prog.Created))
+	conf.Import("a")
+
+	const wantErr = "couldn't load packages due to errors: b"
+
+	prog, err := conf.Load()
+	if err == nil {
+		t.Errorf("Load succeeded unexpectedly, want %q", wantErr)
+	} else if err.Error() != wantErr {
+		t.Errorf("Load failed with wrong error %q, want %q", err, wantErr)
 	}
-	if len(prog.Created) > 0 {
-		path := prog.Created[0].Pkg.Path()
-		if path != "P" {
-			t.Errorf("loadFromArgs(%q): got %v, want [P]", prog.Created, path)
-		}
+	if prog != nil {
+		t.Errorf("Load unexpectedly returned a Program")
 	}
 }
 
-// Simplifying wrapper around buildutil.FakeContext for single-file packages.
-func fakeContext(pkgs map[string]string) *build.Context {
-	pkgs2 := make(map[string]map[string]string)
-	for path, content := range pkgs {
-		pkgs2[path] = map[string]string{"x.go": content}
+func TestLoad_BadDependency_AllowErrors(t *testing.T) {
+	for _, test := range []struct {
+		descr    string
+		pkgs     map[string]string
+		wantPkgs string
+	}{
+
+		{
+			descr: "missing dependency",
+			pkgs: map[string]string{
+				"a": `package a; import _ "b"`,
+				"b": `package b; import _ "c"`,
+			},
+			wantPkgs: "a b",
+		},
+		{
+			descr: "bad package decl in dependency",
+			pkgs: map[string]string{
+				"a": `package a; import _ "b"`,
+				"b": `package b; import _ "c"`,
+				"c": `package`,
+			},
+			wantPkgs: "a b",
+		},
+		{
+			descr: "parse error in dependency",
+			pkgs: map[string]string{
+				"a": `package a; import _ "b"`,
+				"b": `package b; import _ "c"`,
+				"c": `package c; var x = `,
+			},
+			wantPkgs: "a b c",
+		},
+	} {
+		conf := loader.Config{
+			AllowErrors:   true,
+			SourceImports: true,
+			Build:         fakeContext(test.pkgs),
+		}
+		conf.Import("a")
+
+		prog, err := conf.Load()
+		if err != nil {
+			t.Errorf("%s: Load failed unexpectedly: %v", test.descr, err)
+		}
+		if prog == nil {
+			t.Fatalf("%s: Load returned a nil Program", test.descr)
+		}
+
+		if got, want := imported(prog), "a"; got != want {
+			t.Errorf("%s: Imported = %s, want %s", test.descr, got, want)
+		}
+		if got := all(prog); strings.Join(got, " ") != test.wantPkgs {
+			t.Errorf("%s: AllPackages = %s, want %s", test.descr, got, test.wantPkgs)
+		}
 	}
-	return buildutil.FakeContext(pkgs2)
 }
 
+// TODO(adonovan): more Load tests:
+//
+// failures:
+// - to parse package decl of *_test.go files
+// - to parse package decl of external *_test.go files
+// - to parse whole of *_test.go files
+// - to parse whole of external *_test.go files
+// - to open a *.go file during import scanning
+// - to import from binary
+
+// features:
+// - InitialPackages
+// - PackageCreated hook
+// - TypeCheckFuncBodies hook
+
 func TestTransitivelyErrorFreeFlag(t *testing.T) {
 	// Create an minimal custom build.Context
 	// that fakes the following packages:
@@ -182,20 +390,11 @@ func TestTransitivelyErrorFreeFlag(t *testing.T) {
 	}
 }
 
-func hasError(errors []error, substr string) bool {
-	for _, err := range errors {
-		if strings.Contains(err.Error(), substr) {
-			return true
-		}
-	}
-	return false
-}
-
-// Test that both syntax (scan/parse) and type errors are both recorded
+// Test that syntax (scan/parse), type, and loader errors are recorded
 // (in PackageInfo.Errors) and reported (via Config.TypeChecker.Error).
 func TestErrorReporting(t *testing.T) {
 	pkgs := map[string]string{
-		"a": `package a; import _ "b"; var x int = false`,
+		"a": `package a; import (_ "b"; _ "c"); var x int = false`,
 		"b": `package b; 'syntax error!`,
 	}
 	conf := loader.Config{
@@ -229,6 +428,9 @@ func TestErrorReporting(t *testing.T) {
 			if !hasError(info.Errors, "cannot convert false") {
 				t.Errorf("a.Errors = %v, want bool conversion (type) error", info.Errors)
 			}
+			if !hasError(info.Errors, "could not import c") {
+				t.Errorf("a.Errors = %v, want import (loader) error", info.Errors)
+			}
 		case "b":
 			if !hasError(info.Errors, "rune literal not terminated") {
 				t.Errorf("b.Errors = %v, want unterminated literal (syntax) error", info.Errors)
@@ -238,8 +440,9 @@ func TestErrorReporting(t *testing.T) {
 
 	// Check errors reported via error handler.
 	if !hasError(allErrors, "cannot convert false") ||
-		!hasError(allErrors, "rune literal not terminated") {
-		t.Errorf("allErrors = %v, want both syntax and type errors", allErrors)
+		!hasError(allErrors, "rune literal not terminated") ||
+		!hasError(allErrors, "could not import c") {
+		t.Errorf("allErrors = %v, want syntax, type and loader errors", allErrors)
 	}
 }
 
@@ -337,3 +540,60 @@ func TestCycles(t *testing.T) {
 	// - Test that in a legal test cycle, none of the symbols
 	//   defined by augmentation are visible via import.
 }
+
+// ---- utilities ----
+
+// Simplifying wrapper around buildutil.FakeContext for single-file packages.
+func fakeContext(pkgs map[string]string) *build.Context {
+	pkgs2 := make(map[string]map[string]string)
+	for path, content := range pkgs {
+		pkgs2[path] = map[string]string{"x.go": content}
+	}
+	return buildutil.FakeContext(pkgs2)
+}
+
+func hasError(errors []error, substr string) bool {
+	for _, err := range errors {
+		if strings.Contains(err.Error(), substr) {
+			return true
+		}
+	}
+	return false
+}
+
+func keys(m map[string]bool) (keys []string) {
+	for key := range m {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	return
+}
+
+// Returns all loaded packages.
+func all(prog *loader.Program) []string {
+	var pkgs []string
+	for _, info := range prog.AllPackages {
+		pkgs = append(pkgs, info.Pkg.Path())
+	}
+	sort.Strings(pkgs)
+	return pkgs
+}
+
+// Returns initially imported packages, as a string.
+func imported(prog *loader.Program) string {
+	var pkgs []string
+	for _, info := range prog.Imported {
+		pkgs = append(pkgs, info.Pkg.Path())
+	}
+	sort.Strings(pkgs)
+	return strings.Join(pkgs, " ")
+}
+
+// Returns initially created packages, as a string.
+func created(prog *loader.Program) string {
+	var pkgs []string
+	for _, info := range prog.Created {
+		pkgs = append(pkgs, info.Pkg.Path())
+	}
+	return strings.Join(pkgs, " ")
+}
diff --git a/go/loader/stdlib_test.go b/go/loader/stdlib_test.go
index eb439a1..d14928a 100644
--- a/go/loader/stdlib_test.go
+++ b/go/loader/stdlib_test.go
@@ -40,9 +40,7 @@ func TestStdlib(t *testing.T) {
 	ctxt.GOPATH = ""      // disable GOPATH
 	conf := loader.Config{Build: &ctxt}
 	for _, path := range buildutil.AllPackages(conf.Build) {
-		if err := conf.ImportWithTests(path); err != nil {
-			t.Error(err)
-		}
+		conf.ImportWithTests(path)
 	}
 
 	prog, err := conf.Load()
diff --git a/go/loader/testdata/badpkgdecl.go b/go/loader/testdata/badpkgdecl.go
new file mode 100644
index 0000000..1e39359
--- /dev/null
+++ b/go/loader/testdata/badpkgdecl.go
@@ -0,0 +1 @@
+// this file has no package decl
diff --git a/go/loader/util.go b/go/loader/util.go
index 1d782e1..1166c92 100644
--- a/go/loader/util.go
+++ b/go/loader/util.go
@@ -11,9 +11,10 @@ import (
 	"go/token"
 	"io"
 	"os"
-	"path/filepath"
 	"strconv"
 	"sync"
+
+	"golang.org/x/tools/go/buildutil"
 )
 
 // parseFiles parses the Go source files within directory dir and
@@ -27,21 +28,13 @@ func parseFiles(fset *token.FileSet, ctxt *build.Context, displayPath func(strin
 	if displayPath == nil {
 		displayPath = func(path string) string { return path }
 	}
-	isAbs := filepath.IsAbs
-	if ctxt.IsAbsPath != nil {
-		isAbs = ctxt.IsAbsPath
-	}
-	joinPath := filepath.Join
-	if ctxt.JoinPath != nil {
-		joinPath = ctxt.JoinPath
-	}
 	var wg sync.WaitGroup
 	n := len(files)
 	parsed := make([]*ast.File, n)
 	errors := make([]error, n)
 	for i, file := range files {
-		if !isAbs(file) {
-			file = joinPath(dir, file)
+		if !buildutil.IsAbsPath(ctxt, file) {
+			file = buildutil.JoinPath(ctxt, dir, file)
 		}
 		wg.Add(1)
 		go func(i int, file string) {
diff --git a/go/pointer/example_test.go b/go/pointer/example_test.go
index eecfd30..f6cca48 100644
--- a/go/pointer/example_test.go
+++ b/go/pointer/example_test.go
@@ -44,7 +44,8 @@ func main() {
 	// Construct a loader.
 	conf := loader.Config{SourceImports: true}
 
-	// Parse the input file.
+	// Parse the input file, a string.
+	// (Command-line tools should use conf.FromArgs.)
 	file, err := conf.ParseFile("myprog.go", myprog)
 	if err != nil {
 		fmt.Print(err) // parse error
diff --git a/go/ssa/interp/interp_test.go b/go/ssa/interp/interp_test.go
index 5d458f3..19f2ca0 100644
--- a/go/ssa/interp/interp_test.go
+++ b/go/ssa/interp/interp_test.go
@@ -340,9 +340,7 @@ func TestTestmainPackage(t *testing.T) {
 // CreateTestMainPackage should return nil if there were no tests.
 func TestNullTestmainPackage(t *testing.T) {
 	var conf loader.Config
-	if err := conf.CreateFromFilenames("", "testdata/b_test.go"); err != nil {
-		t.Fatalf("ParseFile failed: %s", err)
-	}
+	conf.CreateFromFilenames("", "testdata/b_test.go")
 	iprog, err := conf.Load()
 	if err != nil {
 		t.Fatalf("CreatePackages failed: %s", err)
diff --git a/oracle/oracle.go b/oracle/oracle.go
index 60cc2be..4bb8a9d 100644
--- a/oracle/oracle.go
+++ b/oracle/oracle.go
@@ -322,7 +322,7 @@ func reduceScope(pos string, conf *loader.Config) {
 	// (and possibly its corresponding tests/production code).
 	// TODO(adonovan): set 'augment' based on which file list
 	// contains
-	_ = conf.ImportWithTests(importPath) // ignore error
+	conf.ImportWithTests(importPath)
 }
 
 func pkgContainsFile(bp *build.Package, filename string) bool {
diff --git a/refactor/eg/eg_test.go b/refactor/eg/eg_test.go
index 6e9fc4b..bb96faf 100644
--- a/refactor/eg/eg_test.go
+++ b/refactor/eg/eg_test.go
@@ -72,9 +72,7 @@ func Test(t *testing.T) {
 		"testdata/expr_type_mismatch.template",
 	} {
 		pkgname := strings.TrimSuffix(filepath.Base(filename), ".go")
-		if err := conf.CreateFromFilenames(pkgname, filename); err != nil {
-			t.Fatal(err)
-		}
+		conf.CreateFromFilenames(pkgname, filename)
 	}
 	iprog, err := conf.Load()
 	if err != nil {
diff --git a/refactor/lexical/lexical_test.go b/refactor/lexical/lexical_test.go
index ada6595..1b772d6 100644
--- a/refactor/lexical/lexical_test.go
+++ b/refactor/lexical/lexical_test.go
@@ -37,11 +37,8 @@ func TestStdlib(t *testing.T) {
 		SourceImports: true,
 	}
 	for _, path := range pkgs {
-		if err := conf.ImportWithTests(path); err != nil {
-			t.Error(err)
-		}
+		conf.ImportWithTests(path)
 	}
-
 	iprog, err := conf.Load()
 	if err != nil {
 		t.Fatalf("Load failed: %v", err)
diff --git a/refactor/rename/rename.go b/refactor/rename/rename.go
index 6543685..fc76b32 100644
--- a/refactor/rename/rename.go
+++ b/refactor/rename/rename.go
@@ -365,9 +365,7 @@ func loadProgram(ctxt *build.Context, pkgs map[string]bool) (*loader.Program, er
 	}
 
 	for pkg := range pkgs {
-		if err := conf.ImportWithTests(pkg); err != nil {
-			return nil, err
-		}
+		conf.ImportWithTests(pkg)
 	}
 	return conf.Load()
 }