From 1aadbdfdbb7ad749c12012e6b5ea3eecd3d4257b Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Tue, 2 Apr 2019 17:08:11 -0400 Subject: [PATCH] go/packages: make sure TypesSizes are requested when Types are This code fixes a bug when a user specifies NeedTypes, which implicitly requires TypesSizes, but TypesSizes isn't fetched, which causes typechecking to explode. Also fix a similar issue where NeedImports isn't implicitly fetched for NeedDeps. I added a TODO for a better fix, which is to have an "implicitMode" in the loader type containing all the data that's needed as a prerequisite for other fields. Then we can use implicitMode when fetching data, and cfg.Mode to clear out the fields the user didn't request. Fixes golang/go#31163 Change-Id: If3506765470af43dfb24d06fcbd31b66a623f2e0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/170342 Reviewed-by: Ian Cottrell --- go/packages/golist.go | 2 +- go/packages/packages.go | 42 +++++++++++-------- go/packages/packages_test.go | 78 ++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 18 deletions(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index be58c5ba..a25c3acb 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -78,7 +78,7 @@ func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) { var sizes types.Sizes var sizeserr error var sizeswg sync.WaitGroup - if cfg.Mode&NeedTypesSizes != 0 { + if cfg.Mode&NeedTypesSizes != 0 || cfg.Mode&NeedTypes != 0 { sizeswg.Add(1) go func() { sizes, sizeserr = getSizes(cfg) diff --git a/go/packages/packages.go b/go/packages/packages.go index b0e827fb..4639fcdd 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -420,6 +420,12 @@ type loader struct { Config sizes types.Sizes exportMu sync.Mutex // enforces mutual exclusion of exportdata operations + + // TODO(matloob): Add an implied mode here and use that instead of mode. + // Implied mode would contain all the fields we need the data for so we can + // get the actually requested fields. We'll zero them out before returning + // packages to the user. This will make it easier for us to get the conditions + // where we need certain modes right. } func newLoader(cfg *Config) *loader { @@ -563,7 +569,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { return lpkg.needsrc } - if ld.Mode&NeedImports == 0 { + if ld.Mode&(NeedImports|NeedDeps) == 0 { // We do this to drop the stub import packages that we are not even going to try to resolve. for _, lpkg := range initial { lpkg.Imports = nil @@ -574,7 +580,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { visit(lpkg) } } - if ld.Mode&NeedDeps != 0 { + if ld.Mode&NeedDeps != 0 { // TODO(matloob): This is only the case if NeedTypes is also set, right? for _, lpkg := range srcPkgs { // Complete type information is required for the // immediate dependencies of each source package. @@ -602,46 +608,48 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { importPlaceholders := make(map[string]*Package) for i, lpkg := range initial { result[i] = lpkg.Package + } + for i := range ld.pkgs { // Clear all unrequested fields, for extra de-Hyrum-ization. if ld.Mode&NeedName == 0 { - result[i].Name = "" - result[i].PkgPath = "" + ld.pkgs[i].Name = "" + ld.pkgs[i].PkgPath = "" } if ld.Mode&NeedFiles == 0 { - result[i].GoFiles = nil - result[i].OtherFiles = nil + ld.pkgs[i].GoFiles = nil + ld.pkgs[i].OtherFiles = nil } if ld.Mode&NeedCompiledGoFiles == 0 { - result[i].CompiledGoFiles = nil + ld.pkgs[i].CompiledGoFiles = nil } if ld.Mode&NeedImports == 0 { - result[i].Imports = nil + ld.pkgs[i].Imports = nil } if ld.Mode&NeedExportsFile == 0 { - result[i].ExportFile = "" + ld.pkgs[i].ExportFile = "" } if ld.Mode&NeedTypes == 0 { - result[i].Types = nil - result[i].Fset = nil - result[i].IllTyped = false + ld.pkgs[i].Types = nil + ld.pkgs[i].Fset = nil + ld.pkgs[i].IllTyped = false } if ld.Mode&NeedSyntax == 0 { - result[i].Syntax = nil + ld.pkgs[i].Syntax = nil } if ld.Mode&NeedTypesInfo == 0 { - result[i].TypesInfo = nil + ld.pkgs[i].TypesInfo = nil } if ld.Mode&NeedTypesSizes == 0 { - result[i].TypesSizes = nil + ld.pkgs[i].TypesSizes = nil } if ld.Mode&NeedDeps == 0 { - for j, pkg := range result[i].Imports { + for j, pkg := range ld.pkgs[i].Imports { ph, ok := importPlaceholders[pkg.ID] if !ok { ph = &Package{ID: pkg.ID} importPlaceholders[pkg.ID] = ph } - result[i].Imports[j] = ph + ld.pkgs[i].Imports[j] = ph } } } diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 85a6b31c..0421cfbf 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -576,6 +576,84 @@ func testLoadTypes(t *testing.T, exporter packagestest.Exporter) { } } +// TestLoadTypesBits is equivalent to TestLoadTypes except that it only requests +// the types using the NeedTypes bit. +func TestLoadTypesBits(t *testing.T) { packagestest.TestAll(t, testLoadTypesBits) } +func testLoadTypesBits(t *testing.T, exporter packagestest.Exporter) { + exported := packagestest.Export(t, exporter, []packagestest.Module{{ + Name: "golang.org/fake", + Files: map[string]interface{}{ + "a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`, + "b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`, + "c/c.go": `package c; import "golang.org/fake/d"; const C = "c" + d.D`, + "d/d.go": `package d; import "golang.org/fake/e"; const D = "d" + e.E`, + "e/e.go": `package e; import "golang.org/fake/f"; const E = "e" + f.F`, + "f/f.go": `package f; const F = "f"`, + }}}) + defer exported.Cleanup() + + exported.Config.Mode = packages.NeedTypes | packages.NeedDeps | packages.NeedImports + initial, err := packages.Load(exported.Config, "golang.org/fake/a", "golang.org/fake/c") + if err != nil { + t.Fatal(err) + } + + graph, all := importGraph(initial) + wantGraph := ` +* golang.org/fake/a + golang.org/fake/b +* golang.org/fake/c + golang.org/fake/d + golang.org/fake/e + golang.org/fake/f + golang.org/fake/a -> golang.org/fake/b + golang.org/fake/b -> golang.org/fake/c + golang.org/fake/c -> golang.org/fake/d + golang.org/fake/d -> golang.org/fake/e + golang.org/fake/e -> golang.org/fake/f +`[1:] + if graph != wantGraph { + t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph) + } + + for _, test := range []struct { + id string + }{ + {"golang.org/fake/a"}, + {"golang.org/fake/b"}, + {"golang.org/fake/c"}, + {"golang.org/fake/d"}, + {"golang.org/fake/e"}, + {"golang.org/fake/f"}, + } { + p := all[test.id] + if p == nil { + t.Errorf("missing package: %s", test.id) + continue + } + if p.Types == nil { + t.Errorf("missing types.Package for %s", p) + continue + } + // We don't request the syntax, so we shouldn't get it. + if p.Syntax != nil { + t.Errorf("Syntax unexpectedly provided for %s", p) + } + if p.Errors != nil { + t.Errorf("errors in package: %s: %s", p, p.Errors) + } + } + + // Check value of constant. + aA := constant(all["golang.org/fake/a"], "A") + if aA == nil { + t.Fatalf("a.A: got nil") + } + if got, want := fmt.Sprintf("%v %v", aA, aA.Val()), `const golang.org/fake/a.A untyped string "abcdef"`; got != want { + t.Errorf("a.A: got %s, want %s", got, want) + } +} + func TestLoadSyntaxOK(t *testing.T) { packagestest.TestAll(t, testLoadSyntaxOK) } func testLoadSyntaxOK(t *testing.T, exporter packagestest.Exporter) { exported := packagestest.Export(t, exporter, []packagestest.Module{{