go/packages: two clean-ups

1. Move visit recursion into "if NeedImports" block. 2. Merge two loops to avoid the need to gather srcpkgs. No behavior change is intended Change-Id: I085e8ba0d2acd1f6fae074c0db188e0b953c2fcd Reviewed-on: https://go-review.googlesource.com/c/tools/+/537117 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Matloob <matloob@golang.org>
golang · Oct 30, 2023 · ec032e3 · ec032e3
1 parent 9482e85
commit ec032e3
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 79 deletions.
diff --git a/go/packages/packages.go b/go/packages/packages.go
@@ -699,92 +699,91 @@ func (ld *loader) refine(response *driverResponse) ([]*Package, error) {
 		}
 	}
 
-	// Materialize the import graph (if NeedImports).
-
-	const (
-		white = 0 // new
-		grey  = 1 // in progress
-		black = 2 // complete
-	)
-
-	// visit traverses the import graph, depth-first,
-	// and materializes the graph as Packages.Imports.
-	//
-	// Valid imports are saved in the Packages.Import map.
-	// Invalid imports (cycles and missing nodes) are saved in the importErrors map.
-	// Thus, even in the presence of both kinds of errors, the Import graph remains a DAG.
-	//
-	// visit returns whether the package needs src or has a transitive
-	// dependency on a package that does. These are the only packages
-	// for which we load source code.
-	var stack, srcPkgs []*loaderPackage
-	var visit func(lpkg *loaderPackage) bool
-	visit = func(lpkg *loaderPackage) bool {
-		switch lpkg.color {
-		case black:
-			return lpkg.needsrc
-		case grey:
-			panic("internal error: grey node")
-		}
-		lpkg.color = grey
-		stack = append(stack, lpkg) // push
-		stubs := lpkg.Imports       // the structure form has only stubs with the ID in the Imports
-		lpkg.Imports = make(map[string]*Package, len(stubs))
-		for importPath, ipkg := range stubs {
-			var importErr error
-			imp := ld.pkgs[ipkg.ID]
-			if imp == nil {
-				// (includes package "C" when DisableCgo)
-				importErr = fmt.Errorf("missing package: %q", ipkg.ID)
-			} else if imp.color == grey {
-				importErr = fmt.Errorf("import cycle: %s", stack)
+	if ld.Mode&NeedImports != 0 {
+		// Materialize the import graph.
+
+		const (
+			white = 0 // new
+			grey  = 1 // in progress
+			black = 2 // complete
+		)
+
+		// visit traverses the import graph, depth-first,
+		// and materializes the graph as Packages.Imports.
+		//
+		// Valid imports are saved in the Packages.Import map.
+		// Invalid imports (cycles and missing nodes) are saved in the importErrors map.
+		// Thus, even in the presence of both kinds of errors,
+		// the Import graph remains a DAG.
+		//
+		// visit returns whether the package needs src or has a transitive
+		// dependency on a package that does. These are the only packages
+		// for which we load source code.
+		var stack []*loaderPackage
+		var visit func(lpkg *loaderPackage) bool
+		visit = func(lpkg *loaderPackage) bool {
+			switch lpkg.color {
+			case black:
+				return lpkg.needsrc
+			case grey:
+				panic("internal error: grey node")
 			}
-			if importErr != nil {
-				if lpkg.importErrors == nil {
-					lpkg.importErrors = make(map[string]error)
+			lpkg.color = grey
+			stack = append(stack, lpkg) // push
+			stubs := lpkg.Imports       // the structure form has only stubs with the ID in the Imports
+			lpkg.Imports = make(map[string]*Package, len(stubs))
+			for importPath, ipkg := range stubs {
+				var importErr error
+				imp := ld.pkgs[ipkg.ID]
+				if imp == nil {
+					// (includes package "C" when DisableCgo)
+					importErr = fmt.Errorf("missing package: %q", ipkg.ID)
+				} else if imp.color == grey {
+					importErr = fmt.Errorf("import cycle: %s", stack)
 				}
-				lpkg.importErrors[importPath] = importErr
-				continue
+				if importErr != nil {
+					if lpkg.importErrors == nil {
+						lpkg.importErrors = make(map[string]error)
+					}
+					lpkg.importErrors[importPath] = importErr
+					continue
+				}
+
+				if visit(imp) {
+					lpkg.needsrc = true
+				}
+				lpkg.Imports[importPath] = imp.Package
 			}
 
-			if visit(imp) {
-				lpkg.needsrc = true
+			// Complete type information is required for the
+			// immediate dependencies of each source package.
+			if lpkg.needsrc && ld.Mode&NeedTypes != 0 {
+				for _, ipkg := range lpkg.Imports {
+					ld.pkgs[ipkg.ID].needtypes = true
+				}
 			}
-			lpkg.Imports[importPath] = imp.Package
-		}
-		if lpkg.needsrc {
-			srcPkgs = append(srcPkgs, lpkg)
-		}
-		// NeedTypeSizes causes TypeSizes to be set even
-		// on packages for which types aren't needed.
-		if ld.Mode&NeedTypesSizes != 0 {
-			lpkg.TypesSizes = ld.sizes
-		}
-		stack = stack[:len(stack)-1] // pop
-		lpkg.color = black
 
-		return lpkg.needsrc
-	}
+			// NeedTypeSizes causes TypeSizes to be set even
+			// on packages for which types aren't needed.
+			if ld.Mode&NeedTypesSizes != 0 {
+				lpkg.TypesSizes = ld.sizes
+			}
+			stack = stack[:len(stack)-1] // pop
+			lpkg.color = black
 
-	if ld.Mode&NeedImports == 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
+			return lpkg.needsrc
 		}
-	} else {
+
 		// For each initial package, create its import DAG.
 		for _, lpkg := range initial {
 			visit(lpkg)
 		}
 
-		if ld.Mode&NeedTypes != 0 {
-			// Complete type information is required for the
-			// immediate dependencies of each source package.
-			for _, lpkg := range srcPkgs {
-				for _, ipkg := range lpkg.Imports {
-					ld.pkgs[ipkg.ID].needtypes = true
-				}
-			}
+	} else {
+		// !NeedImports: drop the stub (ID-only) import packages
+		// that we are not even going to try to resolve.
+		for _, lpkg := range initial {
+			lpkg.Imports = nil
 		}
 	}
 

diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
@@ -1217,12 +1217,13 @@ func testSizes(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
-// This is a regression test for the root cause of
-// github.com/golang/vscode-go/issues/3021.
-// If types are needed (any of NeedTypes{,Info,Sizes}
-// and the types.Sizes cannot be obtained (e.g. due to a bad GOARCH)
-// then the Load operation must fail. It must not return a nil
-// TypesSizes, or use the default (wrong) size.
+// This is a regression test for a bug related to
+// github.com/golang/vscode-go/issues/3021: if types are needed (any
+// of NeedTypes{,Info,Sizes} and the types.Sizes cannot be obtained
+// (e.g. due to a bad GOARCH) then the Load operation must fail. It
+// must not return a nil TypesSizes, or use the default (wrong) size.
+// (The root cause of that issue turned out to be due to skew in the
+// Bazel GOPACKAGESDRIVER; see CL 537876.)
 //
 // We use a file=... query because it suppresses the bad-GOARCH check
 // that the go command would otherwise perform eagerly.