cmd/go: rebuild as needed when vetting test packages

If A's external test package imports B, which imports A, and A's
internal test code adds something to A that invalidates anything in A's
export data, then we need to build B against the test-augmented version
of A before using it to build A's external test package.

https://golang.org/cl/92215 taught 'go test' to do this rebuilding
properly, but 'go vet' was not taught the same trick when it learned to
vet test packages in https://golang.org/cl/87636. This commit moves the
necessary logic into the load.TestPackagesFor function so it can be
shared by 'go test' and 'go vet'.

Fixes #23701.

Change-Id: I1086d447eca02933af53de693384eac99a08d9bd
Reviewed-on: https://go-review.googlesource.com/104315
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

src/cmd/go/go_test.go

@@ -5497,7 +5497,7 @@ func TestTestVet(t *testing.T) {
 	tg.grepStdout(`ok\s+vetfail/p2`, "did not run vetfail/p2")
 }
 
-func TestTestRebuild(t *testing.T) {
+func TestTestVetRebuild(t *testing.T) {
 	tg := testgo(t)
 	defer tg.cleanup()
 	tg.parallel()
@@ -5533,6 +5533,7 @@ func TestTestRebuild(t *testing.T) {
 
 	tg.setenv("GOPATH", tg.path("."))
 	tg.run("test", "b")
+	tg.run("vet", "b")
 }
 
 func TestInstallDeps(t *testing.T) {

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

@@ -1712,6 +1712,18 @@ func TestPackagesFor(p *Package, forceTest bool) (ptest, pxtest *Package, err er
 		}
 	}
 
+	if p != ptest && pxtest != nil {
+		// We have made modifications to the package p being tested
+		// and are rebuilding p (as ptest).
+		// Arrange to rebuild all packages q such that
+		// pxtest depends on q and q depends on p.
+		// This makes sure that q sees the modifications to p.
+		// Strictly speaking, the rebuild is only necessary if the
+		// modifications to p change its export metadata, but
+		// determining that is a bit tricky, so we rebuild always.
+		recompileForTest(p, ptest, pxtest)
+	}
+
 	return ptest, pxtest, nil
 }
 
@@ -1732,3 +1744,44 @@ Search:
 	}
 	return stk
 }
+
+func recompileForTest(preal, ptest, pxtest *Package) {
+	// 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.
+	testCopy := map[*Package]*Package{preal: ptest}
+	// Only pxtest and its dependencies can legally depend on p.
+	// If ptest or its dependencies depended on p, the dependency
+	// would be circular.
+	for _, p := range PackageList([]*Package{pxtest}) {
+		if p == preal {
+			continue
+		}
+		// Copy on write.
+		didSplit := p == pxtest
+		split := func() {
+			if didSplit {
+				return
+			}
+			didSplit = true
+			if testCopy[p] != nil {
+				panic("recompileForTest loop")
+			}
+			p1 := new(Package)
+			testCopy[p] = p1
+			*p1 = *p
+			p1.Internal.Imports = make([]*Package, len(p.Internal.Imports))
+			copy(p1.Internal.Imports, p.Internal.Imports)
+			p = p1
+			p.Target = ""
+		}
+
+		// Update p.Internal.Imports to use test copies.
+		for i, imp := range p.Internal.Imports {
+			if p1 := testCopy[imp]; p1 != nil && p1 != imp {
+				split()
+				p.Internal.Imports[i] = p1
+			}
+		}
+	}
+}

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

@@ -911,18 +911,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin
 		t.ImportXtest = true
 	}
 
-	if ptest != p {
-		// We have made modifications to the package p being tested
-		// and are rebuilding p (as ptest).
-		// Arrange to rebuild all packages q such that
-		// the test depends on q and q depends on p.
-		// This makes sure that q sees the modifications to p.
-		// Strictly speaking, the rebuild is only necessary if the
-		// modifications to p change its export metadata, but
-		// determining that is a bit tricky, so we rebuild always.
-		recompileForTest(pmain, p, ptest, pxtest)
-	}
-
 	for _, cp := range pmain.Internal.Imports {
 		if len(cp.Internal.CoverVars) > 0 {
 			t.Cover = append(t.Cover, coverInfo{cp, cp.Internal.CoverVars})
@@ -1058,44 +1046,6 @@ func addTestVet(b *work.Builder, p *load.Package, runAction, installAction *work
 	}
 }
 
-func recompileForTest(pmain, preal, ptest, pxtest *load.Package) {
-	// 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.
-	testCopy := map[*load.Package]*load.Package{preal: ptest}
-	for _, p := range load.PackageList([]*load.Package{pmain}) {
-		if p == preal {
-			continue
-		}
-		// Copy on write.
-		didSplit := p == pmain || p == pxtest
-		split := func() {
-			if didSplit {
-				return
-			}
-			didSplit = true
-			if testCopy[p] != nil {
-				panic("recompileForTest loop")
-			}
-			p1 := new(load.Package)
-			testCopy[p] = p1
-			*p1 = *p
-			p1.Internal.Imports = make([]*load.Package, len(p.Internal.Imports))
-			copy(p1.Internal.Imports, p.Internal.Imports)
-			p = p1
-			p.Target = ""
-		}
-
-		// Update p.Internal.Imports to use test copies.
-		for i, imp := range p.Internal.Imports {
-			if p1 := testCopy[imp]; p1 != nil && p1 != imp {
-				split()
-				p.Internal.Imports[i] = p1
-			}
-		}
-	}
-}
-
 // isTestFile reports whether the source file is a set of tests and should therefore
 // be excluded from coverage analysis.
 func isTestFile(file string) bool {