go/packages: allow types loading without NeedDeps

Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes #golang/go#33077

Author: jirfag

go/packages/golist.go

@@ -733,7 +733,7 @@ func golistargs(cfg *Config, words []string) []string {
 		fmt.Sprintf("-compiled=%t", cfg.Mode&(NeedCompiledGoFiles|NeedSyntax|NeedTypesInfo|NeedTypesSizes) != 0),
 		fmt.Sprintf("-test=%t", cfg.Tests),
 		fmt.Sprintf("-export=%t", usesExportData(cfg)),
-		fmt.Sprintf("-deps=%t", cfg.Mode&NeedDeps != 0),
+		fmt.Sprintf("-deps=%t", cfg.Mode&NeedImports != 0),
 		// go list doesn't let you pass -test and -find together,
 		// probably because you'd just get the TestMain.
 		fmt.Sprintf("-find=%t", !cfg.Tests && cfg.Mode&findFlags == 0),

go/packages/packages.go

@@ -75,7 +75,7 @@ const (
 
 	// Deprecated: LoadImports exists for historical compatibility
 	// and should not be used. Please directly specify the needed fields using the Need values.
-	LoadImports = LoadFiles | NeedImports | NeedDeps
+	LoadImports = LoadFiles | NeedImports
 
 	// Deprecated: LoadTypes exists for historical compatibility
 	// and should not be used. Please directly specify the needed fields using the Need values.
@@ -87,7 +87,7 @@ const (
 
 	// Deprecated: LoadAllSyntax exists for historical compatibility
 	// and should not be used. Please directly specify the needed fields using the Need values.
-	LoadAllSyntax = LoadSyntax
+	LoadAllSyntax = LoadSyntax | NeedDeps
 )
 
 // A Config specifies details about how packages should be loaded.
@@ -479,8 +479,8 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 		}
 		lpkg := &loaderPackage{
 			Package:   pkg,
-			needtypes: (ld.Mode&(NeedTypes|NeedTypesInfo) != 0 && rootIndex < 0) || rootIndex >= 0,
-			needsrc: (ld.Mode&(NeedSyntax|NeedTypesInfo) != 0 && rootIndex < 0) || rootIndex >= 0 ||
+			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",
 		}
@@ -528,8 +528,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 		stack = append(stack, lpkg) // push
 		stubs := lpkg.Imports       // the structure form has only stubs with the ID in the Imports
 		// If NeedImports isn't set, the imports fields will all be zeroed out.
-		// If NeedDeps isn't also set we want to keep the stubs.
-		if ld.Mode&NeedImports != 0 && ld.Mode&NeedDeps != 0 {
+		if ld.Mode&NeedImports != 0 {
 			lpkg.Imports = make(map[string]*Package, len(stubs))
 			for importPath, ipkg := range stubs {
 				var importErr error
@@ -577,7 +576,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 			visit(lpkg)
 		}
 	}
-	if ld.Mode&NeedDeps != 0 { // TODO(matloob): This is only the case if NeedTypes is also set, right?
+	if ld.Mode&NeedImports != 0 && ld.Mode&NeedTypes != 0 {
 		for _, lpkg := range srcPkgs {
 			// Complete type information is required for the
 			// immediate dependencies of each source package.
@@ -602,7 +601,6 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 	}
 
 	result := make([]*Package, len(initial))
-	importPlaceholders := make(map[string]*Package)
 	for i, lpkg := range initial {
 		result[i] = lpkg.Package
 	}
@@ -639,16 +637,6 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 		if ld.Mode&NeedTypesSizes == 0 {
 			ld.pkgs[i].TypesSizes = nil
 		}
-		if ld.Mode&NeedDeps == 0 {
-			for j, pkg := range ld.pkgs[i].Imports {
-				ph, ok := importPlaceholders[pkg.ID]
-				if !ok {
-					ph = &Package{ID: pkg.ID}
-					importPlaceholders[pkg.ID] = ph
-				}
-				ld.pkgs[i].Imports[j] = ph
-			}
-		}
 	}
 	return result, nil
 }
@@ -670,7 +658,6 @@ func (ld *loader) loadRecursive(lpkg *loaderPackage) {
 			}(imp)
 		}
 		wg.Wait()
-
 		ld.loadPackage(lpkg)
 	})
 }
@@ -797,7 +784,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 		if ipkg.Types != nil && ipkg.Types.Complete() {
 			return ipkg.Types, nil
 		}
-		log.Fatalf("internal error: nil Pkg importing %q from %q", path, lpkg)
+		log.Fatalf("internal error: package %q without types was imported from %q", path, lpkg)
 		panic("unreachable")
 	})
 
@@ -808,7 +795,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 		// Type-check bodies of functions only in non-initial packages.
 		// Example: for import graph A->B->C and initial packages {A,C},
 		// we can ignore function bodies in B.
-		IgnoreFuncBodies: (ld.Mode&(NeedDeps|NeedTypesInfo) == 0) && !lpkg.initial,
+		IgnoreFuncBodies: ld.Mode&NeedDeps == 0 && !lpkg.initial,
 
 		Error: appendError,
 		Sizes: ld.sizes,
@@ -1071,5 +1058,5 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 }
 
 func usesExportData(cfg *Config) bool {
-	return cfg.Mode&NeedExportsFile != 0 || cfg.Mode&NeedTypes != 0 && cfg.Mode&NeedTypesInfo == 0
+	return cfg.Mode&NeedExportsFile != 0 || cfg.Mode&NeedTypes != 0 && cfg.Mode&NeedDeps == 0
 }

go/packages/packages_test.go

@@ -569,7 +569,7 @@ func testLoadTypesBits(t *testing.T, exporter packagestest.Exporter) {
 		}}})
 	defer exported.Cleanup()
 
-	exported.Config.Mode = packages.NeedTypes | packages.NeedDeps | packages.NeedImports
+	exported.Config.Mode = packages.NeedTypes | packages.NeedImports
 	initial, err := packages.Load(exported.Config, "golang.org/fake/a", "golang.org/fake/c")
 	if err != nil {
 		t.Fatal(err)
@@ -1236,7 +1236,7 @@ func testContainsOverlayXTest(t *testing.T, exporter packagestest.Exporter) {
 		}}})
 	defer exported.Cleanup()
 	bOverlayXTestFile := filepath.Join(filepath.Dir(exported.File("golang.org/fake", "b/b.go")), "b_overlay_x_test.go")
-	exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedImports | packages.NeedDeps
+	exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedImports
 	exported.Config.Overlay = map[string][]byte{bOverlayXTestFile: []byte(`package b_test; import "golang.org/fake/b"`)}
 	initial, err := packages.Load(exported.Config, "file="+bOverlayXTestFile)
 	if err != nil {
@@ -1928,6 +1928,77 @@ func testAdHocContains(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+func TestLoadTypesInfoWithoutNeedDeps(t *testing.T) {
+	packagestest.TestAll(t, testLoadTypesInfoWithoutNeedDeps)
+}
+func testLoadTypesInfoWithoutNeedDeps(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"`,
+			"b/b.go": `package b`,
+		}}})
+	defer exported.Cleanup()
+
+	exported.Config.Mode = packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports
+	pkgs, err := packages.Load(exported.Config, "golang.org/fake/a")
+	if err != nil {
+		t.Fatal(err)
+	}
+	pkg := pkgs[0]
+	if pkg.IllTyped {
+		t.Fatal("Loaded package is ill typed")
+	}
+	const expectedImport = "golang.org/fake/b"
+	if _, ok := pkg.Imports[expectedImport]; !ok || len(pkg.Imports) != 1 {
+		t.Fatalf("Imports of loaded package: want [%s], got %v", expectedImport, pkg.Imports)
+	}
+}
+
+func TestLoadWithNeedDeps(t *testing.T) {
+	packagestest.TestAll(t, testLoadWithNeedDeps)
+}
+func testLoadWithNeedDeps(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"`,
+			"b/b.go": `package b; import _ "golang.org/fake/c"`,
+			"c/c.go": `package c`,
+		}}})
+	defer exported.Cleanup()
+
+	exported.Config.Mode = packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports | packages.NeedDeps
+	pkgs, err := packages.Load(exported.Config, "golang.org/fake/a")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(pkgs) != 1 {
+		t.Fatalf("Expected 1 package, got %d", len(pkgs))
+	}
+
+	pkgA := pkgs[0]
+	if pkgA.IllTyped {
+		t.Fatal("Loaded package is ill typed")
+	}
+
+	pkgB := pkgA.Imports["golang.org/fake/b"]
+	if pkgB == nil || len(pkgA.Imports) != 1 {
+		t.Fatalf("Imports of loaded package 'a' are invalid: %v", pkgA.Imports)
+	}
+	if pkgB.Types == nil || !pkgB.Types.Complete() || pkgB.TypesInfo == nil {
+		t.Fatalf("Types of package 'b' are nil or incomplete: %v, %v", pkgB.Types, pkgB.TypesInfo)
+	}
+
+	pkgC := pkgB.Imports["golang.org/fake/c"]
+	if pkgC == nil || len(pkgB.Imports) != 1 {
+		t.Fatalf("Imports of loaded package 'c' are invalid: %v", pkgB.Imports)
+	}
+	if pkgC.Types == nil || !pkgC.Types.Complete() || pkgC.TypesInfo == nil {
+		t.Fatalf("Types of package 'b' are nil or incomplete: %v, %v", pkgC.Types, pkgC.TypesInfo)
+	}
+}
+
 func errorMessages(errors []packages.Error) []string {
 	var msgs []string
 	for _, err := range errors {