]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: continue type-checking with fake packages if imports failed
authorRobert Griesemer <gri@golang.org>
Thu, 2 Mar 2017 22:06:35 +0000 (14:06 -0800)
committerRobert Griesemer <gri@golang.org>
Mon, 6 Mar 2017 18:50:56 +0000 (18:50 +0000)
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
src/go/types/api_test.go
src/go/types/check.go
src/go/types/resolver.go

index 1e99f4fb13712a89fab3f89c22b8f48c3bed3963..7202828f32a40dab0c4623deacb4bdcad87013c6 100644 (file)
@@ -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
 
index 92c6d75e704fbefd5fa69433672365746171da8d..d4f3f3571706ea946046542fa41a012aeeaf6d56 100644 (file)
@@ -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)
+                               }
+                       }
+               }
+       }
+}
index 28e94f1940e189ba52a818e05f12ad2a3b39f16d..26db5769b98d3fb024eef6d8edbf9164f2982a03 100644 (file)
@@ -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),
        }
 }
 
index 9b6e767758a66b1f0816c5d3fb8234222d081791..04389916f94da8b682e7d21cdc86cced22225e64 100644 (file)
@@ -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