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>

go/packages/golist.go

@@ -879,7 +879,7 @@ func golistargs(cfg *Config, words []string) []string {
 	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),

go/packages/packages.go

@@ -510,12 +510,23 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 		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 {

go/packages/packages_test.go

@@ -2635,6 +2635,74 @@ func testForTestField(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+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 {

internal/lsp/mod/testdata/unchanged/go.mod

@@ -1 +1,3 @@
-module unchanged
+module unchanged
+
+go 1.14