diff --git a/go/packages/packages.go b/go/packages/packages.go index dd62414e..55a466e8 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -38,7 +38,8 @@ const ( // Package fields added: Imports. LoadImports - // LoadTypes adds type information for the package's exported symbols. + // LoadTypes adds type information for the package's exported symbols for + // packages matching the patterns. // Package fields added: Types, Fset, IllTyped. // This will use the type information provided by the build system if // possible, and the ExportFile field may be filled in. @@ -50,7 +51,8 @@ const ( // LoadAllSyntax adds typed syntax trees for the packages matching the patterns // and all dependencies. - // Package fields added: Syntax, TypesInfo, for all packages in import graph. + // Package fields added: Types, Fset, IllTyped, Syntax, TypesInfo, for all + // packages in import graph. LoadAllSyntax ) @@ -218,24 +220,22 @@ type Package struct { Imports map[string]*Package // Types is the type information for the package. - // Modes LoadTypes and above set this field for all packages. - // - // TODO(adonovan): all packages? In Types mode this entails - // asymptotically more export data processing than is required - // to load the requested packages. Is that what we want? + // Modes LoadTypes and above set this field for packages matching the + // patterns; type information for dependencies may be missing or incomplete. + // Mode LoadSyntaxAll sets this field for all packages, including dependencies. Types *types.Package // Fset provides position information for Types, TypesInfo, and Syntax. - // Modes LoadTypes and above set this field for all packages. + // It is set only when Types is set. Fset *token.FileSet // IllTyped indicates whether the package has any type errors. - // Modes LoadTypes and above set this field for all packages. + // It is set only when Types is set. IllTyped bool // Syntax is the package's syntax trees, for the files listed in GoFiles. // - // Mode LoadSyntax set this field for packages matching the patterns. + // Mode LoadSyntax sets this field for packages matching the patterns. // Mode LoadSyntaxAll sets this field for all packages, including dependencies. Syntax []*ast.File @@ -349,6 +349,7 @@ type loaderPackage struct { loadOnce sync.Once color uint8 // for cycle detection needsrc bool // load from source (Mode >= LoadTypes) + needtypes bool // type information is either requested or depended on initial bool // package was matched by a pattern } @@ -413,9 +414,11 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { for _, pkg := range list { lpkg := &loaderPackage{ Package: pkg, + needtypes: ld.Mode >= LoadAllSyntax || + ld.Mode >= LoadTypes && isRoot[pkg.ID], needsrc: ld.Mode >= LoadAllSyntax || - (ld.Mode >= LoadSyntax && isRoot[pkg.ID]) || - (pkg.ExportFile == "" && pkg.PkgPath != "unsafe"), + ld.Mode >= LoadSyntax && isRoot[pkg.ID] || + pkg.ExportFile == "" && pkg.PkgPath != "unsafe", } ld.pkgs[lpkg.ID] = lpkg if isRoot[lpkg.ID] { @@ -444,6 +447,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { // for which we load source code. var stack []*loaderPackage var visit func(lpkg *loaderPackage) bool + var srcPkgs []*loaderPackage visit = func(lpkg *loaderPackage) bool { switch lpkg.color { case black: @@ -477,7 +481,9 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { } lpkg.Imports[importPath] = imp.Package } - + if lpkg.needsrc { + srcPkgs = append(srcPkgs, lpkg) + } stack = stack[:len(stack)-1] // pop lpkg.color = black @@ -495,6 +501,14 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { visit(lpkg) } } + for _, lpkg := range srcPkgs { + // Complete type information is required for the + // immediate dependencies of each source package. + for _, ipkg := range lpkg.Imports { + imp := ld.pkgs[ipkg.ID] + imp.needtypes = true + } + } // Load type data if needed, starting at // the initial packages (roots of the import DAG). if ld.Mode >= LoadTypes { @@ -557,6 +571,15 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) { // package declarations are inconsistent. lpkg.Types = types.NewPackage(lpkg.PkgPath, lpkg.Name) + // Subtle: we populate all Types fields with an empty Package + // before loading export data so that export data processing + // never has to create a types.Package for an indirect dependency, + // which would then require that such created packages be explicitly + // inserted back into the Import graph as a final step after export data loading. + // The Diamond test exercises this case. + if !lpkg.needtypes { + return + } if !lpkg.needsrc { ld.loadFromExportData(lpkg) return // not a source package, don't get syntax trees @@ -783,7 +806,11 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error // package that might possibly be mentioned by the // current package---its transitive closure. // - // TODO(adonovan): it would be more simpler and more efficient + // In loadPackage, we unconditionally create a types.Package for + // each dependency so that export data loading does not + // create new ones. + // + // TODO(adonovan): it would be simpler and more efficient // if the export data machinery invoked a callback to // get-or-create a package instead of a map. // @@ -802,12 +829,16 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error } visit(lpkg.Imports) + viewLen := len(view) + 1 // adding the self package // Parse the export data. - // (May create/modify packages in view.) + // (May modify incomplete packages in view but not create new ones.) tpkg, err := gcexportdata.Read(r, ld.Fset, view, lpkg.PkgPath) if err != nil { return nil, fmt.Errorf("reading %s: %v", lpkg.ExportFile, err) } + if viewLen != len(view) { + log.Fatalf("Unexpected package creation during export data loading") + } lpkg.Types = tpkg lpkg.IllTyped = false diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 6624e16c..dab4169a 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -666,15 +666,16 @@ func TestLoadSyntaxOK(t *testing.T) { // Can we simulate its existence? for _, test := range []struct { - id string - wantSyntax bool + id string + wantSyntax bool + wantComplete bool }{ - {"a", true}, // source package - {"b", true}, // source package - {"c", true}, // source package - {"d", false}, // export data package - {"e", false}, // export data package - {"f", false}, // export data package + {"a", true, true}, // source package + {"b", true, true}, // source package + {"c", true, true}, // source package + {"d", false, true}, // export data package + {"e", false, false}, // export data package + {"f", false, false}, // export data package } { if usesOldGolist && !test.wantSyntax { // legacy go list always upgrades to LoadAllSyntax, syntax will be filled in. @@ -688,8 +689,12 @@ func TestLoadSyntaxOK(t *testing.T) { if p.Types == nil { t.Errorf("missing types.Package for %s", p) continue - } else if !p.Types.Complete() { - t.Errorf("incomplete types.Package for %s", p) + } else if p.Types.Complete() != test.wantComplete { + if test.wantComplete { + t.Errorf("incomplete types.Package for %s", p) + } else { + t.Errorf("unexpected complete types.Package for %s", p) + } } if (p.Syntax != nil) != test.wantSyntax { if test.wantSyntax { @@ -710,6 +715,62 @@ func TestLoadSyntaxOK(t *testing.T) { } } +func TestLoadDiamondTypes(t *testing.T) { + // We make a diamond dependency and check the type d.D is the same through both paths + tmp, cleanup := makeTree(t, map[string]string{ + "src/a/a.go": `package a; import ("b"; "c"); var _ = b.B == c.C`, + "src/b/b.go": `package b; import "d"; var B d.D`, + "src/c/c.go": `package c; import "d"; var C d.D`, + "src/d/d.go": `package d; type D int`, + }) + defer cleanup() + + cfg := &packages.Config{ + Mode: packages.LoadSyntax, + Env: append(os.Environ(), "GOPATH="+tmp, "GO111MODULE=off"), + Error: func(err error) { + t.Errorf("Error during load: %v", err) + }, + } + initial, err := packages.Load(cfg, "a") + if err != nil { + t.Fatal(err) + } + + var visit func(pkg *packages.Package) + seen := make(map[string]bool) + visit = func(pkg *packages.Package) { + if seen[pkg.ID] { + return + } + seen[pkg.ID] = true + for _, err := range pkg.Errors { + t.Errorf("Error on package %v: %v", pkg.ID, err) + } + for _, imp := range pkg.Imports { + visit(imp) + } + } + for _, pkg := range initial { + visit(pkg) + } + + graph, _ := importGraph(initial) + wantGraph := ` +* a + b + c + d + a -> b + a -> c + b -> d + c -> d +`[1:] + if graph != wantGraph { + t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph) + } +} + func TestLoadSyntaxError(t *testing.T) { // A type error in a lower-level package (e) prevents go list // from producing export data for all packages that depend on it