go/types: continue type-checking with fake packages if imports failed

This will make type-checking more robust in the presence of import errors.

Also:
- import is now relative to directory containing teh file containing the import
  (matters for relative imports)
- factored out package import code from main resolver loop
- fixed a couple of minor bugs

Fixes #16088.

Change-Id: I1ace45c13cd0fa675d1762877cec0a30afd9ecdc
Reviewed-on: https://go-review.googlesource.com/37697
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/go/types/api.go

@@ -57,10 +57,9 @@ func (err Error) Error() string {
 // vendored packages. See https://golang.org/s/go15vendor.
 // If possible, external implementations should implement ImporterFrom.
 type Importer interface {
-	// Import returns the imported package for the given import
-	// path, or an error if the package couldn't be imported.
-	// Two calls to Import with the same path return the same
-	// package.
+	// Import returns the imported package for the given import path.
+	// The semantics is like for ImporterFrom.ImportFrom except that
+	// dir and mode are ignored (since they are not present).
 	Import(path string) (*Package, error)
 }
 
@@ -79,12 +78,15 @@ type ImporterFrom interface {
 	Importer
 
 	// ImportFrom returns the imported package for the given import
-	// path when imported by the package in srcDir, or an error
-	// if the package couldn't be imported. The mode value must
-	// be 0; it is reserved for future use.
-	// Two calls to ImportFrom with the same path and srcDir return
-	// the same package.
-	ImportFrom(path, srcDir string, mode ImportMode) (*Package, error)
+	// path when imported by a package file located in dir.
+	// If the import failed, besides returning an error, ImportFrom
+	// is encouraged to cache and return a package anyway, if one
+	// was created. This will reduce package inconsistencies and
+	// follow-on type checker errors due to the missing package.
+	// The mode value must be 0; it is reserved for future use.
+	// Two calls to ImportFrom with the same path and dir must
+	// return the same package.
+	ImportFrom(path, dir string, mode ImportMode) (*Package, error)
 }
 
 // A Config specifies the configuration for type checking.
@@ -99,7 +101,7 @@ type Config struct {
 	// identifiers referring to package C (which won't find an object).
 	// This feature is intended for the standard library cmd/api tool.
 	//
-	// Caution: Effects may be unpredictable due to follow-up errors.
+	// Caution: Effects may be unpredictable due to follow-on errors.
 	//          Do not use casually!
 	FakeImportC bool
 

src/go/types/api_test.go

@@ -1295,3 +1295,74 @@ func f(x int) { y := x; print(y) }
 		}
 	}
 }
+
+// TestFailedImport tests that we don't get follow-on errors
+// elsewhere in a package due to failing to import a package.
+func TestFailedImport(t *testing.T) {
+	testenv.MustHaveGoBuild(t)
+
+	const src = `
+package p
+
+import "foo" // should only see an error here
+
+const c = foo.C
+type T = foo.T
+var v T = c
+func f(x T) T { return foo.F(x) }
+`
+	fset := token.NewFileSet()
+	f, err := parser.ParseFile(fset, "src", src, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	files := []*ast.File{f}
+
+	// type-check using all possible importers
+	for _, compiler := range []string{"gc", "gccgo", "source"} {
+		errcount := 0
+		conf := Config{
+			Error: func(err error) {
+				// we should only see the import error
+				if errcount > 0 || !strings.Contains(err.Error(), "could not import foo") {
+					t.Errorf("for %s importer, got unexpected error: %v", compiler, err)
+				}
+				errcount++
+			},
+			Importer: importer.For(compiler, nil),
+		}
+
+		info := &Info{
+			Uses: make(map[*ast.Ident]Object),
+		}
+		pkg, _ := conf.Check("p", fset, files, info)
+		if pkg == nil {
+			t.Errorf("for %s importer, type-checking failed to return a package", compiler)
+			continue
+		}
+
+		imports := pkg.Imports()
+		if len(imports) != 1 {
+			t.Errorf("for %s importer, got %d imports, want 1", compiler, len(imports))
+			continue
+		}
+		imp := imports[0]
+		if imp.Name() != "foo" {
+			t.Errorf(`for %s importer, got %q, want "foo"`, compiler, imp.Name())
+			continue
+		}
+
+		// verify that all uses of foo refer to the imported package foo (imp)
+		for ident, obj := range info.Uses {
+			if ident.Name == "foo" {
+				if obj, ok := obj.(*PkgName); ok {
+					if obj.Imported() != imp {
+						t.Errorf("%s resolved to %v; want %v", ident, obj.Imported(), imp)
+					}
+				} else {
+					t.Errorf("%s resolved to %v; want package name", ident, obj)
+				}
+			}
+		}
+	}
+}

src/go/types/check.go

@@ -57,6 +57,16 @@ type context struct {
 	hasCallOrRecv bool           // set if an expression contains a function call or channel receive operation
 }
 
+// An importKey identifies an imported package by import path and source directory
+// (directory containing the file containing the import). In practice, the directory
+// may always be the same, or may not matter. Given an (import path, directory), an
+// importer must always return the same package (but given two different import paths,
+// an importer may still return the same package by mapping them to the same package
+// paths).
+type importKey struct {
+	path, dir string
+}
+
 // A Checker maintains the state of the type checker.
 // It must be created with NewChecker.
 type Checker struct {
@@ -66,7 +76,8 @@ type Checker struct {
 	fset *token.FileSet
 	pkg  *Package
 	*Info
-	objMap map[Object]*declInfo // maps package-level object to declaration info
+	objMap map[Object]*declInfo   // maps package-level object to declaration info
+	impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package
 
 	// information collected during type-checking of a set of package files
 	// (initialized by Files, valid only for the duration of check.Files;
@@ -162,6 +173,7 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch
 		pkg:    pkg,
 		Info:   info,
 		objMap: make(map[Object]*declInfo),
+		impMap: make(map[importKey]*Package),
 	}
 }
 

src/go/types/resolver.go

@@ -125,6 +125,67 @@ func (check *Checker) filename(fileNo int) string {
 	return fmt.Sprintf("file[%d]", fileNo)
 }
 
+func (check *Checker) importPackage(pos token.Pos, path, dir string) *Package {
+	// If we already have a package for the given (path, dir)
+	// pair, use it instead of doing a full import.
+	// Checker.impMap only caches packages that are marked Complete
+	// or fake (dummy packages for failed imports). Incomplete but
+	// non-fake packages do require an import to complete them.
+	key := importKey{path, dir}
+	imp := check.impMap[key]
+	if imp != nil {
+		return imp
+	}
+
+	// no package yet => import it
+	if path == "C" && check.conf.FakeImportC {
+		imp = NewPackage("C", "C")
+		imp.fake = true
+	} else {
+		// ordinary import
+		var err error
+		if importer := check.conf.Importer; importer == nil {
+			err = fmt.Errorf("Config.Importer not installed")
+		} else if importerFrom, ok := importer.(ImporterFrom); ok {
+			imp, err = importerFrom.ImportFrom(path, dir, 0)
+			if imp == nil && err == nil {
+				err = fmt.Errorf("Config.Importer.ImportFrom(%s, %s, 0) returned nil but no error", path, dir)
+			}
+		} else {
+			imp, err = importer.Import(path)
+			if imp == nil && err == nil {
+				err = fmt.Errorf("Config.Importer.Import(%s) returned nil but no error", path)
+			}
+		}
+		if err != nil {
+			check.errorf(pos, "could not import %s (%s)", path, err)
+			if imp == nil {
+				// create a new fake package
+				// come up with a sensible package name (heuristic)
+				name := path
+				if i := len(name); i > 0 && name[i-1] == '/' {
+					name = name[:i-1]
+				}
+				if i := strings.LastIndex(name, "/"); i >= 0 {
+					name = name[i+1:]
+				}
+				imp = NewPackage(path, name)
+			}
+			// continue to use the package as best as we can
+			imp.fake = true // avoid follow-up lookup failures
+		}
+	}
+
+	// package should be complete or marked fake, but be cautious
+	if imp.complete || imp.fake {
+		check.impMap[key] = imp
+		return imp
+	}
+
+	// something went wrong (importer may have returned incomplete package without error)
+	return nil
+}
+
 // collectObjects collects all file and package objects and inserts them
 // into their respective scopes. It also performs imports and associates
 // methods with receiver base type names.
@@ -134,25 +195,14 @@ func (check *Checker) collectObjects() {
 	// pkgImports is the set of packages already imported by any package file seen
 	// so far. Used to avoid duplicate entries in pkg.imports. Allocate and populate
 	// it (pkg.imports may not be empty if we are checking test files incrementally).
+	// Note that pkgImports is keyed by package (and thus package path), not by an
+	// importKey value. Two different importKey values may map to the same package
+	// which is why we cannot use the check.impMap here.
 	var pkgImports = make(map[*Package]bool)
 	for _, imp := range pkg.imports {
 		pkgImports[imp] = true
 	}
 
-	// srcDir is the directory used by the Importer to look up packages.
-	// The typechecker itself doesn't need this information so it is not
-	// explicitly provided. Instead, we extract it from position info of
-	// the source files as needed.
-	// This is the only place where the type-checker (just the importer)
-	// needs to know the actual source location of a file.
-	// TODO(gri) can we come up with a better API instead?
-	var srcDir string
-	if len(check.files) > 0 {
-		// FileName may be "" (typically for tests) in which case
-		// we get "." as the srcDir which is what we would want.
-		srcDir = dir(check.fset.Position(check.files[0].Name.Pos()).Filename)
-	}
-
 	for fileNo, file := range check.files {
 		// The package identifier denotes the current package,
 		// but there is no corresponding package object.
@@ -168,6 +218,11 @@ func (check *Checker) collectObjects() {
 		fileScope := NewScope(check.pkg.scope, pos, end, check.filename(fileNo))
 		check.recordScope(file, fileScope)
 
+		// determine file directory, necessary to resolve imports
+		// FileName may be "" (typically for tests) in which case
+		// we get "." as the directory which is what we would want.
+		fileDir := dir(check.fset.Position(file.Name.Pos()).Filename)
+
 		for _, decl := range file.Decls {
 			switch d := decl.(type) {
 			case *ast.BadDecl:
@@ -179,35 +234,15 @@ func (check *Checker) collectObjects() {
 					switch s := spec.(type) {
 					case *ast.ImportSpec:
 						// import package
-						var imp *Package
 						path, err := validatedImportPath(s.Path.Value)
 						if err != nil {
 							check.errorf(s.Path.Pos(), "invalid import path (%s)", err)
 							continue
 						}
-						if path == "C" && check.conf.FakeImportC {
-							// TODO(gri) shouldn't create a new one each time
-							imp = NewPackage("C", "C")
-							imp.fake = true
-						} else {
-							// ordinary import
-							if importer := check.conf.Importer; importer == nil {
-								err = fmt.Errorf("Config.Importer not installed")
-							} else if importerFrom, ok := importer.(ImporterFrom); ok {
-								imp, err = importerFrom.ImportFrom(path, srcDir, 0)
-								if imp == nil && err == nil {
-									err = fmt.Errorf("Config.Importer.ImportFrom(%s, %s, 0) returned nil but no error", path, pkg.path)
-								}
-							} else {
-								imp, err = importer.Import(path)
-								if imp == nil && err == nil {
-									err = fmt.Errorf("Config.Importer.Import(%s) returned nil but no error", path)
-								}
-							}
-							if err != nil {
-								check.errorf(s.Path.Pos(), "could not import %s (%s)", path, err)
-								continue
-							}
+
+						imp := check.importPackage(s.Path.Pos(), path, fileDir)
+						if imp == nil {
+							continue
 						}
 
 						// add package to list of explicit imports