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>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index aa354027f400d304977401ee1019c4593201c2d2..ffedcff3d9bdc12a124efae41674e242d1175a51 100644 (file)
--- a/src/cmd/go/go_test.go
+++ b/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) {

diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index 02cbb94bc7cd4d8e25328a14a68085c134afbb53..590de6c49b940de7fd0a180e9734fb83b4c3cd85 100644 (file)
--- a/src/cmd/go/internal/load/pkg.go
+++ b/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
+                       }
+               }
+       }
+}

diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index 42bff352c508064fdcca070f79f4b45d4606ca92..7f14ce3cd73fba3c5003c21207a164dec45e0e6b 100644 (file)
--- a/src/cmd/go/internal/test/test.go
+++ b/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 {