cmd/go: rebuild as needed for tests of packages that add methods

If A's external test package imports B, which imports A,
and A's (internal) test code also adds something to A that
invalidates anything in the export data from a build of A
without its test code, then strictly speaking we need to
rebuild B against the test-augmented version of A before
using it to build A's external test package.

We've been skating by without doing this for a very long time,
but I knew we'd need to handle it better eventually,
I planned for it in the new build cache simplifications,
and the code was ready. Now that we have a real-world
test case that needs it, turn on the "proper rebuilding" code.

It doesn't really matter how much things slow down, since
a real-world test cases that caused an internal compiler error
before is now handled correctly, but it appears to be small:
I wasn't able to measure an effect on "go test -a -c fmt".
And of course most builds won't use -a and will be cached well.

Fixes #6204.
Fixes #23701.

Change-Id: I2cd60cf400d1928428979ab05831f48ff7cee6ca
Reviewed-on: https://go-review.googlesource.com/92215
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 7db62da34e076ad9631ac5202d835b312ecc2191..92600b6238fcd5c32fa2236e5f77bef180c735df 100644 (file)
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -5439,6 +5439,44 @@ func TestTestVet(t *testing.T) {
        tg.grepStdout(`ok\s+vetfail/p2`, "did not run vetfail/p2")
 }
 
+func TestTestRebuild(t *testing.T) {
+       tg := testgo(t)
+       defer tg.cleanup()
+       tg.parallel()
+
+       // golang.org/issue/23701.
+       // b_test imports b with augmented method from export_test.go.
+       // b_test also imports a, which imports b.
+       // Must not accidentally see un-augmented b propagate through a to b_test.
+       tg.tempFile("src/a/a.go", `package a
+               import "b"
+               type Type struct{}
+               func (*Type) M() b.T {return 0}
+       `)
+       tg.tempFile("src/b/b.go", `package b
+               type T int
+               type I interface {M() T}
+       `)
+       tg.tempFile("src/b/export_test.go", `package b
+               func (*T) Method() *T { return nil }
+       `)
+       tg.tempFile("src/b/b_test.go", `package b_test
+               import (
+                       "testing"
+                       "a"
+                       . "b"
+               )
+               func TestBroken(t *testing.T) {
+                       x := new(T)
+                       x.Method()
+                       _ = new(a.Type)
+               }
+       `)
+
+       tg.setenv("GOPATH", tg.path("."))
+       tg.run("test", "b")
+}
+
 func TestInstallDeps(t *testing.T) {
        tooSlow(t)
        tg := testgo(t)

diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index bf684809e3a0c82e303c83f6ae2b966627cb8a3f..a99c6a5ec220200b8398f4ae76b38501c573d6e1 100644 (file)
--- a/src/cmd/go/internal/test/test.go
+++ b/src/cmd/go/internal/test/test.go
@@ -897,7 +897,7 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin
                t.ImportXtest = true
        }
 
-       if ptest != p && localCover {
+       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
@@ -906,13 +906,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin
                // 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.
-               // TODO(rsc): Once we get export metadata changes
-               // handled properly, look into the expense of dropping
-               // "&& localCover" above.
-               //
-               // This will cause extra compilation, so for now we only do it
-               // when testCover is set. The conditions are more general, though,
-               // and we may find that we need to do it always in the future.
                recompileForTest(pmain, p, ptest, pxtest)
        }