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 <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

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)
 		}
 	}
 

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

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"

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 <nil>'
 stdout 'testdep_a <nil>'
@@ -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 <nil>'
 stdout 'testdep_a <nil>'
-stdout 'testdep_cycle <nil>'
+stdout 'testdep_cycle \[cycleerr.test\] <nil>'
 stdout 'cycleerr \[cycleerr.test\] "[^"]*import cycle not allowed in test'
 ! stderr 'import cycle not allowed in test'