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>

2 files changed, 39 insertions, 8 deletions

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 7db62da34e..92600b6238 100644
--- 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 bf684809e3..a99c6a5ec2 100644
--- 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)
 	}