]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: localize computation of deps/depserrors in list
authorMichael Matloob <matloob@golang.org>
Sat, 8 Apr 2023 01:13:30 +0000 (21:13 -0400)
committerMichael Matloob <matloob@golang.org>
Mon, 10 Apr 2023 22:33:50 +0000 (22:33 +0000)
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
src/cmd/go/internal/load/pkg.go
src/cmd/go/internal/load/test.go

index a05fca9deea0291e7d0562aea06240a5150a4092..1fd42ccfc707beb141048040c69e5479be41d8b8 100644 (file)
@@ -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"),
+               IgnoreImports:      *listFind,
+               ModResolveTests:    *listTest,
+               AutoVCS:            true,
                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 {
-                       // 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)
-                       }
+       }
+
+       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 {
+                       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.
index 5cf8e071e5e8168cb687bde7d2af61c130243465..24f32ee8257983df327208b7d5949dca51b3ba9d 100644 (file)
@@ -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
-               }
-               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 pkg.Incomplete {
+                       anyIncomplete = true
+               }
+       }
+       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
                }
        }
 
index 938fb35cdb35e0e649612dac8be3660821a9487a..71ae0b6e0f11c9246acc698f3ec68cc25e32ddb0 100644 (file)
@@ -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
+                               }
+                       }
                        break
                }
        }
-       if pmain.Error != nil || len(pmain.DepsErrors) > 0 {
+       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.