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 <iancottrell@google.com>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index be58c5b..a25c3ac 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -78,7 +78,7 @@
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 b0e827f..4639fcd 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -420,6 +420,12 @@
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 @@
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 @@
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 @@
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 85a6b31..0421cfb 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -576,6 +576,84 @@
}
}
+// 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{{
