From 449e69f1597273f13db684f4a629f3e43beb2987 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 3 Apr 2023 17:02:49 -0400 Subject: [PATCH] cmd/go: suppress calls to collectDeps for test packages Instead, do the cycle checking in recompileForTest once the test variant packages have been poked in the right places in the dependency tree(graph?). (Pair programming with bcmills@.) For #59157. Change-Id: I0c644cb9f2c0dac3a5b0189e2aa0eef083c669f6 Reviewed-on: https://go-review.googlesource.com/c/go/+/482237 Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- src/cmd/go/internal/list/list.go | 28 ++-- src/cmd/go/internal/load/test.go | 120 +++++++++--------- .../go/testdata/script/list_test_cycle.txt | 33 +++++ src/cmd/go/testdata/script/list_test_err.txt | 27 +--- 4 files changed, 112 insertions(+), 96 deletions(-) create mode 100644 src/cmd/go/testdata/script/list_test_cycle.txt diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go index 672c3c122f6..a05fca9deea 100644 --- a/src/cmd/go/internal/list/list.go +++ b/src/cmd/go/internal/list/list.go @@ -653,7 +653,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) { } else { pmain, ptest, pxtest, err = load.TestPackagesFor(ctx, pkgOpts, p, nil) if err != nil { - base.Errorf("can't load test package: %s", err) + base.Fatalf("can't load test package: %s", err) } } if pmain != nil { @@ -770,20 +770,22 @@ func runList(ctx context.Context, cmd *base.Command, args []string) { delete(m, old) } } - // 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 + 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) } - p.Deps = make([]string, 0, len(deps)) - for d := range deps { - p.Deps = append(p.Deps, d) - } - sort.Strings(p.Deps) } } diff --git a/src/cmd/go/internal/load/test.go b/src/cmd/go/internal/load/test.go index 7a40cc6b454..ec7fe10c350 100644 --- a/src/cmd/go/internal/load/test.go +++ b/src/cmd/go/internal/load/test.go @@ -112,22 +112,12 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co rawTestImports := str.StringList(p.TestImports) for i, path := range p.TestImports { p1 := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport) - if str.Contains(p1.Deps, p.ImportPath) || p1.ImportPath == p.ImportPath { - // Same error that loadPackage returns (via reusePackage) in pkg.go. - // Can't change that code, because that code is only for loading the - // non-test copy of a package. - ptestErr = &PackageError{ - ImportStack: importCycleStack(p1, p.ImportPath), - Err: errors.New("import cycle not allowed in test"), - IsImportCycle: true, - } - } p.TestImports[i] = p1.ImportPath imports = append(imports, p1) } var err error p.TestEmbedFiles, testEmbed, err = resolveEmbed(p.Dir, p.TestEmbedPatterns) - if err != nil && ptestErr == nil { + if err != nil { ptestErr = &PackageError{ ImportStack: stk.Copy(), Err: err, @@ -208,7 +198,9 @@ 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...) - ptest.collectDeps() + if !opts.SuppressDeps { + ptest.collectDeps() + } } else { ptest = p } @@ -250,7 +242,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co if pxtestNeedsPtest { pxtest.Internal.Imports = append(pxtest.Internal.Imports, ptest) } - pxtest.collectDeps() + if !opts.SuppressDeps { + pxtest.collectDeps() + } } // Arrange for testing.Testing to report true. @@ -341,7 +335,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co pmain.Imports = append(pmain.Imports, pxtest.ImportPath) t.ImportXtest = true } - pmain.collectDeps() + if !opts.SuppressDeps { + pmain.collectDeps() + } // Sort and dedup pmain.Imports. // Only matters for go list -test output. @@ -357,7 +353,10 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co pmain.Internal.RawImports = str.StringList(pmain.Imports) // Replace pmain's transitive dependencies with test copies, as necessary. - recompileForTest(pmain, p, ptest, pxtest) + cycleErr := recompileForTest(pmain, p, ptest, pxtest) + if cycleErr != nil { + ptest.Error = cycleErr + } if cover != nil { if cfg.Experiment.CoverageRedesign { @@ -403,46 +402,6 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co return pmain, ptest, pxtest } -// importCycleStack returns an import stack from p to the package whose import -// path is target. -func importCycleStack(p *Package, target string) []string { - // importerOf maps each import path to its importer nearest to p. - importerOf := map[string]string{p.ImportPath: ""} - - // q is a breadth-first queue of packages to search for target. - // Every package added to q has a corresponding entry in pathTo. - // - // We search breadth-first for two reasons: - // - // 1. We want to report the shortest cycle. - // - // 2. If p contains multiple cycles, the first cycle we encounter might not - // contain target. To ensure termination, we have to break all cycles - // other than the first. - q := []*Package{p} - - for len(q) > 0 { - p := q[0] - q = q[1:] - if path := p.ImportPath; path == target { - var stk []string - for path != "" { - stk = append(stk, path) - path = importerOf[path] - } - return stk - } - for _, dep := range p.Internal.Imports { - if _, ok := importerOf[dep.ImportPath]; !ok { - importerOf[dep.ImportPath] = p.ImportPath - q = append(q, dep) - } - } - } - - panic("lost path to cycle") -} - // recompileForTest copies and replaces certain packages in pmain's dependency // graph. This is necessary for two reasons. First, if ptest is different than // preal, packages that import the package under test should get ptest instead @@ -452,7 +411,7 @@ func importCycleStack(p *Package, target string) []string { // clear p.Internal.BuildInfo in the test copy to prevent link conflicts. // This may happen if both -coverpkg and the command line patterns include // multiple main packages. -func recompileForTest(pmain, preal, ptest, pxtest *Package) { +func recompileForTest(pmain, preal, ptest, pxtest *Package) *PackageError { // The "test copy" of preal is ptest. // For each package that depends on preal, make a "test copy" // that depends on ptest. And so on, up the dependency tree. @@ -462,7 +421,7 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) { continue } // Copy on write. - didSplit := p == pmain || p == pxtest + didSplit := p == pmain || p == pxtest || p == ptest split := func() { if didSplit { return @@ -489,6 +448,10 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) { for i, imp := range p.Internal.Imports { if p1 := testCopy[imp]; p1 != nil && p1 != imp { split() + + // If the test dependencies cause a cycle with pmain, this is + // where it is introduced. + // (There are no cycles in the graph until this assignment occurs.) p.Internal.Imports[i] = p1 } } @@ -503,6 +466,49 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) { split() } } + + // Do search to find cycle. + // importerOf maps each import path to its importer nearest to p. + importerOf := map[*Package]*Package{} + for _, p := range ptest.Internal.Imports { + importerOf[p] = nil + } + + // q is a breadth-first queue of packages to search for target. + // Every package added to q has a corresponding entry in pathTo. + // + // We search breadth-first for two reasons: + // + // 1. We want to report the shortest cycle. + // + // 2. If p contains multiple cycles, the first cycle we encounter might not + // contain target. To ensure termination, we have to break all cycles + // other than the first. + q := slices.Clip(ptest.Internal.Imports) + for len(q) > 0 { + p := q[0] + q = q[1:] + if p == ptest { + var stk []string + for p != nil { + stk = append(stk, p.ImportPath) + p = importerOf[p] + } + return &PackageError{ + ImportStack: stk, + Err: errors.New("import cycle not allowed in test"), + IsImportCycle: true, + } + } + for _, dep := range p.Internal.Imports { + if _, ok := importerOf[dep]; !ok { + importerOf[dep] = p + q = append(q, dep) + } + } + } + + return nil } // isTestFunc tells whether fn has the type of a testing function. arg diff --git a/src/cmd/go/testdata/script/list_test_cycle.txt b/src/cmd/go/testdata/script/list_test_cycle.txt new file mode 100644 index 00000000000..2ab85289260 --- /dev/null +++ b/src/cmd/go/testdata/script/list_test_cycle.txt @@ -0,0 +1,33 @@ +go list ./p +stdout 'example/p' + +! go list -json=ImportPath -test ./p +cmp stderr wanterr.txt + +! go list -json=ImportPath,Deps -test ./p +cmp stderr wanterr.txt + +! go list -json=ImportPath,Deps -deps -test ./p +cmp stderr wanterr.txt + +! go list -json=ImportPath -deps -test ./p +cmp stderr wanterr.txt + +-- wanterr.txt -- +can't load test package: package example/p + imports example/r + imports example/q: import cycle not allowed in test +-- go.mod -- +module example +go 1.20 +-- p/p.go -- +package p +-- p/p_test.go -- +package p +import "example/q" +-- q/q.go -- +package q +import "example/r" +-- r/r.go -- +package r +import "example/p" diff --git a/src/cmd/go/testdata/script/list_test_err.txt b/src/cmd/go/testdata/script/list_test_err.txt index 25dbb969b01..02bd6a16d41 100644 --- a/src/cmd/go/testdata/script/list_test_err.txt +++ b/src/cmd/go/testdata/script/list_test_err.txt @@ -3,19 +3,6 @@ env GO111MODULE=off # issue 28491: errors in test source files should not prevent # "go list -test" from returning useful information. -# go list prints information for package, internal test, -# external test, but not testmain package when there is a -# syntax error in test sources. -! go list -test -deps syntaxerr -stdout pkgdep -stdout testdep_a -stdout testdep_b -stdout ^syntaxerr$ -stdout '^syntaxerr \[syntaxerr.test\]' -stdout '^syntaxerr_test \[syntaxerr.test\]' -! stdout '^syntaxerr\.test' -stderr 'expected declaration' - # go list -e prints information for all test packages. # The syntax error is shown in the package error field. go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' syntaxerr @@ -30,13 +17,6 @@ stdout 'syntaxerr\.test "[^"]*expected declaration' [short] stop -# go list prints partial information with test naming error -! go list -test -deps nameerr -stdout pkgdep -stdout testdep_a -stdout testdep_b -stderr 'wrong signature for TestBad' - go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' nameerr stdout 'pkgdep ' stdout 'testdep_a ' @@ -48,15 +28,10 @@ stdout 'nameerr\.test "[^"]*wrong signature for TestBad' ! go list -test -deps genericerr stderr 'wrong signature for TestGeneric, test functions cannot have type parameters' -# go list prints partial information with error if test has cyclic import -! go list -test -deps cycleerr -stdout cycleerr -stderr 'import cycle not allowed in test' - go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' cycleerr stdout 'cycleerr ' stdout 'testdep_a ' -stdout 'testdep_cycle ' +stdout 'testdep_cycle \[cycleerr.test\] ' stdout 'cycleerr \[cycleerr.test\] "[^"]*import cycle not allowed in test' ! stderr 'import cycle not allowed in test'