From 2ad7453bf42654a6a1615fa1cc867123570c4595 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 2 Mar 2017 14:06:35 -0800 Subject: [PATCH] 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 Reviewed-by: Alan Donovan TryBot-Result: Gobot Gobot --- src/go/types/api.go | 24 +++++---- src/go/types/api_test.go | 71 +++++++++++++++++++++++++ src/go/types/check.go | 14 ++++- src/go/types/resolver.go | 111 +++++++++++++++++++++++++-------------- 4 files changed, 170 insertions(+), 50 deletions(-) diff --git a/src/go/types/api.go b/src/go/types/api.go index 1e99f4fb13..7202828f32 100644 --- a/src/go/types/api.go +++ b/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 diff --git a/src/go/types/api_test.go b/src/go/types/api_test.go index 92c6d75e70..d4f3f35717 100644 --- a/src/go/types/api_test.go +++ b/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) + } + } + } + } +} diff --git a/src/go/types/check.go b/src/go/types/check.go index 28e94f1940..26db5769b9 100644 --- a/src/go/types/check.go +++ b/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), } } diff --git a/src/go/types/resolver.go b/src/go/types/resolver.go index 9b6e767758..04389916f9 100644 --- a/src/go/types/resolver.go +++ b/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