cmd/go: localize computation of deps/depserrors in list

Stop depending on DepsErrors to report errors to the user and instead
only use it and compute it in list. Instead, use Incomplete to figure
out when a package or its depencies have an error, and only if they
do, do the work of finding all those errors.

For #59157

Change-Id: Ied927f53e7b1f66fad9248b40dd11ed960b3ef91
Reviewed-on: https://go-review.googlesource.com/c/go/+/483495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>

src/cmd/go/internal/list/list.go

@@ -598,17 +598,12 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
 		base.Fatalf("go list -export cannot be used with -find")
 	}
 
+	suppressDeps := !listJsonFields.needAny("Deps", "DepsErrors")
+
 	pkgOpts := load.PackageOpts{
 		IgnoreImports:      *listFind,
 		ModResolveTests:    *listTest,
 		AutoVCS:            true,
-		// SuppressDeps is set if the user opts to explicitly ask for the json fields they
-		// need, don't ask for Deps or DepsErrors. It's not set when using a template string,
-		// even if *listFmt doesn't contain .Deps because Deps are used to find import cycles
-		// for test variants of packages and users who have been providing format strings
-		// might not expect those errors to stop showing up.
-		// See issue #52443.
-		SuppressDeps:       !listJsonFields.needAny("Deps", "DepsErrors"),
 		SuppressBuildInfo:  !*listExport && !listJsonFields.needAny("Stale", "StaleReason"),
 		SuppressEmbedFiles: !*listExport && !listJsonFields.needAny("EmbedFiles", "TestEmbedFiles", "XTestEmbedFiles"),
 	}
@@ -770,22 +765,17 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
 				delete(m, old)
 			}
 		}
-		if !pkgOpts.SuppressDeps {
+	}
+
+	if !suppressDeps {
+		all := pkgs
+		if !*listDeps {
+			// if *listDeps, then all is already in PackageList order.
+			all = load.PackageList(pkgs)
+		}
 		// Recompute deps lists using new strings, from the leaves up.
 		for _, p := range all {
-				deps := make(map[string]bool)
-				for _, p1 := range p.Internal.Imports {
-					deps[p1.ImportPath] = true
-					for _, d := range p1.Deps {
-						deps[d] = true
-					}
-				}
-				p.Deps = make([]string, 0, len(deps))
-				for d := range deps {
-					p.Deps = append(p.Deps, d)
-				}
-				sort.Strings(p.Deps)
-			}
+			collectDeps(p)
 		}
 	}
 
@@ -888,6 +878,48 @@ func loadPackageList(roots []*load.Package) []*load.Package {
 	return pkgs
 }
 
+// collectDeps populates p.Deps and p.DepsErrors by iterating over
+// p.Internal.Imports.
+//
+// TODO(jayconrod): collectDeps iterates over transitive imports for every
+// package. We should only need to visit direct imports.
+func collectDeps(p *load.Package) {
+	deps := make(map[string]*load.Package)
+	var q []*load.Package
+	q = append(q, p.Internal.Imports...)
+	for i := 0; i < len(q); i++ {
+		p1 := q[i]
+		path := p1.ImportPath
+		// The same import path could produce an error or not,
+		// depending on what tries to import it.
+		// Prefer to record entries with errors, so we can report them.
+		p0 := deps[path]
+		if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
+			deps[path] = p1
+			for _, p2 := range p1.Internal.Imports {
+				if deps[p2.ImportPath] != p2 {
+					q = append(q, p2)
+				}
+			}
+		}
+	}
+
+	p.Deps = make([]string, 0, len(deps))
+	for dep := range deps {
+		p.Deps = append(p.Deps, dep)
+	}
+	sort.Strings(p.Deps)
+	for _, dep := range p.Deps {
+		p1 := deps[dep]
+		if p1 == nil {
+			panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
+		}
+		if p1.Error != nil {
+			p.DepsErrors = append(p.DepsErrors, p1.Error)
+		}
+	}
+}
+
 // TrackingWriter tracks the last byte written on every write so
 // we can avoid printing a newline if one was already written or
 // if there is no output at all.

src/cmd/go/internal/load/pkg.go

@@ -127,7 +127,7 @@ type PackagePublic struct {
 	// Error information
 	// Incomplete is above, packed into the other bools
 	Error      *PackageError   `json:",omitempty"` // error loading this package (not dependencies)
-	DepsErrors []*PackageError `json:",omitempty"` // errors loading dependencies
+	DepsErrors []*PackageError `json:",omitempty"` // errors loading dependencies, collected by go list before output
 
 	// Test information
 	// If you add to this list you MUST add to p.AllFiles (below) too.
@@ -336,6 +336,7 @@ func (p *Package) setLoadPackageDataError(err error, path string, stk *ImportSta
 		Pos:         pos,
 		Err:         err,
 	}
+	p.Incomplete = true
 
 	if path != stk.Top() {
 		p.Error.setPos(importPos)
@@ -1776,6 +1777,7 @@ func (p *Package) load(ctx context.Context, opts PackageOpts, path string, stk *
 				ImportStack: stk.Copy(),
 				Err:         err,
 			}
+			p.Incomplete = true
 
 			// Add the importer's position information if the import position exists, and
 			// the current package being examined is the importer.
@@ -2017,10 +2019,7 @@ func (p *Package) load(ctx context.Context, opts PackageOpts, path string, stk *
 		}
 	}
 	p.Internal.Imports = imports
-	if !opts.SuppressDeps {
-		p.collectDeps()
-	}
-	if p.Error == nil && p.Name == "main" && !p.Internal.ForceLibrary && len(p.DepsErrors) == 0 && !opts.SuppressBuildInfo {
+	if p.Error == nil && p.Name == "main" && !p.Internal.ForceLibrary && !p.Incomplete && !opts.SuppressBuildInfo {
 		// TODO(bcmills): loading VCS metadata can be fairly slow.
 		// Consider starting this as a background goroutine and retrieving the result
 		// asynchronously when we're actually ready to build the package, or when we
@@ -2256,48 +2255,6 @@ func isBadEmbedName(name string) bool {
 	return false
 }
 
-// collectDeps populates p.Deps and p.DepsErrors by iterating over
-// p.Internal.Imports.
-//
-// TODO(jayconrod): collectDeps iterates over transitive imports for every
-// package. We should only need to visit direct imports.
-func (p *Package) collectDeps() {
-	deps := make(map[string]*Package)
-	var q []*Package
-	q = append(q, p.Internal.Imports...)
-	for i := 0; i < len(q); i++ {
-		p1 := q[i]
-		path := p1.ImportPath
-		// The same import path could produce an error or not,
-		// depending on what tries to import it.
-		// Prefer to record entries with errors, so we can report them.
-		p0 := deps[path]
-		if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
-			deps[path] = p1
-			for _, p2 := range p1.Internal.Imports {
-				if deps[p2.ImportPath] != p2 {
-					q = append(q, p2)
-				}
-			}
-		}
-	}
-
-	p.Deps = make([]string, 0, len(deps))
-	for dep := range deps {
-		p.Deps = append(p.Deps, dep)
-	}
-	sort.Strings(p.Deps)
-	for _, dep := range p.Deps {
-		p1 := deps[dep]
-		if p1 == nil {
-			panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
-		}
-		if p1.Error != nil {
-			p.DepsErrors = append(p.DepsErrors, p1.Error)
-		}
-	}
-}
-
 // vcsStatusCache maps repository directories (string)
 // to their VCS information.
 var vcsStatusCache par.ErrCache[string, vcs.Status]
@@ -2314,6 +2271,7 @@ func (p *Package) setBuildInfo(autoVCS bool) {
 	setPkgErrorf := func(format string, args ...any) {
 		if p.Error == nil {
 			p.Error = &PackageError{Err: fmt.Errorf(format, args...)}
+			p.Incomplete = true
 		}
 	}
 
@@ -2836,12 +2794,6 @@ type PackageOpts struct {
 	// when -buildvcs=auto (the default).
 	AutoVCS bool
 
-	// SuppressDeps is true if the caller does not need Deps and DepsErrors to be populated
-	// on the package. TestPackagesAndErrors examines the  Deps field to determine if the test
-	// variant has an import cycle, so SuppressDeps should not be set if TestPackagesAndErrors
-	// will be called on the package.
-	SuppressDeps bool
-
 	// SuppressBuildInfo is true if the caller does not need p.Stale, p.StaleReason, or p.Internal.BuildInfo
 	// to be populated on the package.
 	SuppressBuildInfo bool
@@ -3037,20 +2989,17 @@ func setPGOProfilePath(pkgs []*Package) {
 // CheckPackageErrors prints errors encountered loading pkgs and their
 // dependencies, then exits with a non-zero status if any errors were found.
 func CheckPackageErrors(pkgs []*Package) {
-	printed := map[*PackageError]bool{}
+	var anyIncomplete bool
 	for _, pkg := range pkgs {
-		if pkg.Error != nil {
-			base.Errorf("%v", pkg.Error)
-			printed[pkg.Error] = true
+		if pkg.Incomplete {
+			anyIncomplete = true
 		}
-		for _, err := range pkg.DepsErrors {
-			// Since these are errors in dependencies,
-			// the same error might show up multiple times,
-			// once in each package that depends on it.
-			// Only print each once.
-			if !printed[err] {
-				printed[err] = true
-				base.Errorf("%v", err)
+	}
+	if anyIncomplete {
+		all := PackageList(pkgs)
+		for _, p := range all {
+			if p.Error != nil {
+				base.Errorf("%v", p.Error)
 			}
 		}
 	}
@@ -3118,6 +3067,7 @@ func mainPackagesOnly(pkgs []*Package, matches []*search.Match) []*Package {
 		if treatAsMain[pkg.ImportPath] {
 			if pkg.Error == nil {
 				pkg.Error = &PackageError{Err: &mainPackageError{importPath: pkg.ImportPath}}
+				pkg.Incomplete = true
 			}
 			mains = append(mains, pkg)
 		}
@@ -3178,6 +3128,7 @@ func GoFilesPackage(ctx context.Context, opts PackageOpts, gofiles []string) *Pa
 			pkg.Error = &PackageError{
 				Err: fmt.Errorf("named files must be .go files: %s", pkg.Name),
 			}
+			pkg.Incomplete = true
 			return pkg
 		}
 	}
@@ -3247,6 +3198,7 @@ func GoFilesPackage(ctx context.Context, opts PackageOpts, gofiles []string) *Pa
 
 	if opts.MainOnly && pkg.Name != "main" && pkg.Error == nil {
 		pkg.Error = &PackageError{Err: &mainPackageError{importPath: pkg.ImportPath}}
+		pkg.Incomplete = true
 	}
 	setToolFlags(pkg)
 
@@ -3369,6 +3321,7 @@ func PackagesAndErrorsOutsideModule(ctx context.Context, opts PackageOpts, args
 		}
 		if pkgErr != nil && pkg.Error == nil {
 			pkg.Error = &PackageError{Err: pkgErr}
+			pkg.Incomplete = true
 		}
 	}
 

src/cmd/go/internal/load/test.go

@@ -58,19 +58,24 @@ func TestPackagesFor(ctx context.Context, opts PackageOpts, p *Package, cover *T
 			err = p1.Error
 			break
 		}
-		if len(p1.DepsErrors) > 0 {
-			perr := p1.DepsErrors[0]
-			err = perr
+		if p1.Incomplete {
+			ps := PackageList([]*Package{p1})
+			for _, p := range ps {
+				if p.Error != nil {
+					err = p.Error
 					break
 				}
 			}
-	if pmain.Error != nil || len(pmain.DepsErrors) > 0 {
+			break
+		}
+	}
+	if pmain.Error != nil || pmain.Incomplete {
 		pmain = nil
 	}
-	if ptest.Error != nil || len(ptest.DepsErrors) > 0 {
+	if ptest.Error != nil || ptest.Incomplete {
 		ptest = nil
 	}
-	if pxtest != nil && (pxtest.Error != nil || len(pxtest.DepsErrors) > 0) {
+	if pxtest != nil && (pxtest.Error != nil || pxtest.Incomplete) {
 		pxtest = nil
 	}
 	return pmain, ptest, pxtest, err
@@ -108,12 +113,17 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 	var imports, ximports []*Package
 	var stk ImportStack
 	var testEmbed, xtestEmbed map[string][]string
+	var incomplete bool
 	stk.Push(p.ImportPath + " (test)")
 	rawTestImports := str.StringList(p.TestImports)
 	for i, path := range p.TestImports {
 		p1, err := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
 		if err != nil && ptestErr == nil {
 			ptestErr = err
+			incomplete = true
+		}
+		if p1.Incomplete {
+			incomplete = true
 		}
 		p.TestImports[i] = p1.ImportPath
 		imports = append(imports, p1)
@@ -125,6 +135,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 			ImportStack: stk.Copy(),
 			Err:         err,
 		}
+		incomplete = true
 		embedErr := err.(*EmbedError)
 		ptestErr.setPos(p.Internal.Build.TestEmbedPatternPos[embedErr.Pattern])
 	}
@@ -132,12 +143,16 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 
 	stk.Push(p.ImportPath + "_test")
 	pxtestNeedsPtest := false
+	var pxtestIncomplete bool
 	rawXTestImports := str.StringList(p.XTestImports)
 	for i, path := range p.XTestImports {
 		p1, err := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.XTestImportPos[path], ResolveImport)
 		if err != nil && pxtestErr == nil {
 			pxtestErr = err
 		}
+		if p1.Incomplete {
+			pxtestIncomplete = true
+		}
 		if p1.ImportPath == p.ImportPath {
 			pxtestNeedsPtest = true
 		} else {
@@ -154,6 +169,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 		embedErr := err.(*EmbedError)
 		pxtestErr.setPos(p.Internal.Build.XTestEmbedPatternPos[embedErr.Pattern])
 	}
+	pxtestIncomplete = pxtestIncomplete || pxtestErr != nil
 	stk.Pop()
 
 	// Test package.
@@ -161,6 +177,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 		ptest = new(Package)
 		*ptest = *p
 		ptest.Error = ptestErr
+		ptest.Incomplete = incomplete
 		ptest.ForTest = p.ImportPath
 		ptest.GoFiles = nil
 		ptest.GoFiles = append(ptest.GoFiles, p.GoFiles...)
@@ -204,9 +221,6 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 		ptest.Internal.OrigImportPath = p.Internal.OrigImportPath
 		ptest.Internal.PGOProfile = p.Internal.PGOProfile
 		ptest.Internal.Build.Directives = append(slices.Clip(p.Internal.Build.Directives), p.Internal.Build.TestDirectives...)
-		if !opts.SuppressDeps {
-			ptest.collectDeps()
-		}
 	} else {
 		ptest = p
 	}
@@ -225,6 +239,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 				ForTest:    p.ImportPath,
 				Module:     p.Module,
 				Error:      pxtestErr,
+				Incomplete: pxtestIncomplete,
 				EmbedFiles: p.XTestEmbedFiles,
 			},
 			Internal: PackageInternal{
@@ -248,9 +263,6 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 		if pxtestNeedsPtest {
 			pxtest.Internal.Imports = append(pxtest.Internal.Imports, ptest)
 		}
-		if !opts.SuppressDeps {
-			pxtest.collectDeps()
-		}
 	}
 
 	// Arrange for testing.Testing to report true.
@@ -297,6 +309,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 			p1, err := loadImport(ctx, opts, pre, dep, "", nil, &stk, nil, 0)
 			if err != nil && pmain.Error == nil {
 				pmain.Error = err
+				pmain.Incomplete = true
 			}
 			pmain.Internal.Imports = append(pmain.Internal.Imports, p1)
 		}
@@ -344,9 +357,6 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 		pmain.Imports = append(pmain.Imports, pxtest.ImportPath)
 		t.ImportXtest = true
 	}
-	if !opts.SuppressDeps {
-		pmain.collectDeps()
-	}
 
 	// Sort and dedup pmain.Imports.
 	// Only matters for go list -test output.
@@ -365,6 +375,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 	cycleErr := recompileForTest(pmain, p, ptest, pxtest)
 	if cycleErr != nil {
 		ptest.Error = cycleErr
+		ptest.Incomplete = true
 	}
 
 	if cover != nil {
@@ -403,6 +414,7 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
 	data, err := formatTestmain(t)
 	if err != nil && pmain.Error == nil {
 		pmain.Error = &PackageError{Err: err}
+		pmain.Incomplete = true
 	}
 	// Set TestmainGo even if it is empty: the presence of a TestmainGo
 	// indicates that this package is, in fact, a test main.