go/packages: fix incorrect needtypes and needsrcs logic
This logic is really fiddly and I'm not really 100% sure it's right, though
I've thought about it for quite abit (and added comments to help me reason
through it).
Also always request CompiledGoFiles when NeedTypes is true because we might
need to fall back to loading from source when type data is incorrect.
Fixes golang/go#36441
Fixes golang/go#36547
Change-Id: I1cc27ca2e4401a9abc8502990b0da7d0480f6f84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214943
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index dce7b19..27ef864 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -879,7 +879,7 @@
const findFlags = NeedImports | NeedTypes | NeedSyntax | NeedTypesInfo
fullargs := []string{
"-e", "-json",
- fmt.Sprintf("-compiled=%t", cfg.Mode&(NeedCompiledGoFiles|NeedSyntax|NeedTypesInfo|NeedTypesSizes) != 0),
+ fmt.Sprintf("-compiled=%t", cfg.Mode&(NeedCompiledGoFiles|NeedSyntax|NeedTypes|NeedTypesInfo|NeedTypesSizes) != 0),
fmt.Sprintf("-test=%t", cfg.Tests),
fmt.Sprintf("-export=%t", usesExportData(cfg)),
fmt.Sprintf("-deps=%t", cfg.Mode&NeedImports != 0),
diff --git a/go/packages/packages.go b/go/packages/packages.go
index bc11505..b0e8ba2 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -510,12 +510,23 @@
if i, found := rootMap[pkg.ID]; found {
rootIndex = i
}
+
+ // Overlays can invalidate export data.
+ // TODO(matloob): make this check fine-grained based on dependencies on overlaid files
+ exportDataInvalid := len(ld.Overlay) > 0 || pkg.ExportFile == "" && pkg.PkgPath != "unsafe"
+ // This package needs type information if the caller requested types and the package is
+ // either a root, or it's a non-root and the user requested dependencies ...
+ needtypes := (ld.Mode&NeedTypes|NeedTypesInfo != 0 && (rootIndex >= 0 || ld.Mode&NeedDeps != 0))
+ // This package needs source if the call requested source (or types info, which implies source)
+ // and the package is either a root, or itas a non- root and the user requested dependencies...
+ needsrc := ((ld.Mode&(NeedSyntax|NeedTypesInfo) != 0 && (rootIndex >= 0 || ld.Mode&NeedDeps != 0)) ||
+ // ... or if we need types and the exportData is invalid. We fall back to (incompletely)
+ // typechecking packages from source if they fail to compile.
+ (ld.Mode&NeedTypes|NeedTypesInfo != 0 && exportDataInvalid)) && pkg.PkgPath != "unsafe"
lpkg := &loaderPackage{
Package: pkg,
- needtypes: (ld.Mode&(NeedTypes|NeedTypesInfo) != 0 && ld.Mode&NeedDeps != 0 && rootIndex < 0) || rootIndex >= 0,
- needsrc: (ld.Mode&(NeedSyntax|NeedTypesInfo) != 0 && ld.Mode&NeedDeps != 0 && rootIndex < 0) || rootIndex >= 0 ||
- len(ld.Overlay) > 0 || // Overlays can invalidate export data. TODO(matloob): make this check fine-grained based on dependencies on overlaid files
- pkg.ExportFile == "" && pkg.PkgPath != "unsafe",
+ needtypes: needtypes,
+ needsrc: needsrc,
}
ld.pkgs[lpkg.ID] = lpkg
if rootIndex >= 0 {
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index fe8ef01..cce6ec3 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -2635,6 +2635,74 @@
}
}
+func TestCgoNoSyntax(t *testing.T) {
+ packagestest.TestAll(t, testCgoNoSyntax)
+}
+
+// Stolen from internal/testenv package in core.
+// hasGoBuild reports whether the current system can build programs with ``go build''
+// and then run them with os.StartProcess or exec.Command.
+func hasGoBuild() bool {
+ if os.Getenv("GO_GCFLAGS") != "" {
+ // It's too much work to require every caller of the go command
+ // to pass along "-gcflags="+os.Getenv("GO_GCFLAGS").
+ // For now, if $GO_GCFLAGS is set, report that we simply can't
+ // run go build.
+ return false
+ }
+ switch runtime.GOOS {
+ case "android", "js":
+ return false
+ case "darwin":
+ if strings.HasPrefix(runtime.GOARCH, "arm") {
+ return false
+ }
+ }
+ return true
+}
+
+func testCgoNoSyntax(t *testing.T, exporter packagestest.Exporter) {
+ // The android builders have a complex setup which causes this test to fail. See discussion on
+ // golang.org/cl/214943 for more details.
+ if !hasGoBuild() {
+ t.Skip("this test can't run on platforms without go build. See discussion on golang.org/cl/214943 for more details.")
+ }
+
+ exported := packagestest.Export(t, exporter, []packagestest.Module{{
+ Name: "golang.org/fake",
+ Files: map[string]interface{}{
+ "c/c.go": `package c; import "C"`,
+ },
+ }})
+
+ // Explicitly enable cgo.
+ exported.Config.Env = append(exported.Config.Env, "CGO_ENABLED=1")
+
+ modes := []packages.LoadMode{
+ packages.NeedTypes,
+ packages.NeedName | packages.NeedTypes,
+ packages.NeedName | packages.NeedTypes | packages.NeedImports,
+ packages.NeedName | packages.NeedTypes | packages.NeedImports | packages.NeedDeps,
+ packages.NeedName | packages.NeedImports,
+ }
+ for _, mode := range modes {
+ t.Run(fmt.Sprint(mode), func(t *testing.T) {
+ exported.Config.Mode = mode
+ pkgs, err := packages.Load(exported.Config, "golang.org/fake/c")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(pkgs) != 1 {
+ t.Fatalf("Expected 1 package, got %v", pkgs)
+ }
+ pkg := pkgs[0]
+ if len(pkg.Errors) != 0 {
+ t.Fatalf("Expected no errors in package, got %v", pkg.Errors)
+ }
+ })
+ }
+}
+
func errorMessages(errors []packages.Error) []string {
var msgs []string
for _, err := range errors {
diff --git a/internal/lsp/mod/testdata/unchanged/go.mod b/internal/lsp/mod/testdata/unchanged/go.mod
index 40d2d02..e5bdaef 100644
--- a/internal/lsp/mod/testdata/unchanged/go.mod
+++ b/internal/lsp/mod/testdata/unchanged/go.mod
@@ -1 +1,3 @@
-module unchanged
\ No newline at end of file
+module unchanged
+
+go 1.14
