go/analysis/passes/testinggoroutine: report by enclosing regions

This adds support for t.Run() statements. The new version
performs two passes. The first pass collects all of the
`go callee` and `t.Run(name, callee)` statements. The second pass
inspects each callee FuncDecl or FuncLit for a call to a
tb.Forbidden() function.

When the enclosing callee function called from an t.Run() statement,
it reports when tb is declared outside of the body of the
enclosing function.

Fixes golang/go#63799
Updates golang/go#63849
Updates golang/go#48124

Change-Id: I10d5f2a0af9b985126dbfcc98eaab97757a7f71d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/541155
TryBot-Result: Gopher Robot <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Tim King <[email protected]>

Author: timothy-king

go/analysis/passes/testinggoroutine/doc.go

@@ -7,7 +7,7 @@
 //
 // # Analyzer testinggoroutine
 //
-// testinggoroutine: report calls to (*testing.T).Fatal from goroutines started by a test.
+// testinggoroutine: report calls to (*testing.T).Fatal from goroutines started by a test
 //
 // Functions that abruptly terminate a test, such as the Fatal, Fatalf, FailNow, and
 // Skip{,f,Now} methods of *testing.T, must be called from the test goroutine itself.

go/analysis/passes/testinggoroutine/testdata/src/a/a.go

@@ -275,6 +275,212 @@ func TestWithCustomType(t *testing.T) {
 	}
 }
 
+func helpTB(tb testing.TB) {
+	tb.FailNow()
+}
+
+func TestTB(t *testing.T) {
+	go helpTB(t) // want "call to .+TB.+FailNow from a non-test goroutine"
+}
+
 func TestIssue48124(t *testing.T) {
-	go h()
+	go helper(t) // want "call to .+T.+Skip from a non-test goroutine"
+}
+
+func TestEachCall(t *testing.T) {
+	go helper(t) // want "call to .+T.+Skip from a non-test goroutine"
+	go helper(t) // want "call to .+T.+Skip from a non-test goroutine"
+}
+
+func TestWithSubtest(t *testing.T) {
+	t.Run("name", func(t2 *testing.T) {
+		t.FailNow() // want "call to .+T.+FailNow on t defined outside of the subtest"
+		t2.Fatal()
+	})
+
+	f := func(t3 *testing.T) {
+		t.FailNow()
+		t3.Fatal()
+	}
+	t.Run("name", f) // want "call to .+T.+FailNow on t defined outside of the subtest"
+
+	g := func(t4 *testing.T) {
+		t.FailNow()
+		t4.Fatal()
+	}
+	g(t)
+
+	t.Run("name", helper)
+
+	go t.Run("name", func(t2 *testing.T) {
+		t.FailNow() // want "call to .+T.+FailNow on t defined outside of the subtest"
+		t2.Fatal()
+	})
+}
+
+func TestMultipleVariables(t *testing.T) {
+	{ // short decl
+		f, g := func(t1 *testing.T) {
+			t1.Fatal()
+		}, func(t2 *testing.T) {
+			t2.Error()
+		}
+
+		go f(t) // want "call to .+T.+Fatal from a non-test goroutine"
+		go g(t)
+
+		t.Run("name", f)
+		t.Run("name", g)
+	}
+
+	{ // var decl
+		var f, g = func(t1 *testing.T) {
+			t1.Fatal()
+		}, func(t2 *testing.T) {
+			t2.Error()
+		}
+
+		go f(t) // want "call to .+T.+Fatal from a non-test goroutine"
+		go g(t)
+
+		t.Run("name", f)
+		t.Run("name", g)
+	}
+}
+
+func BadIgnoresMultipleAssignments(t *testing.T) {
+	{
+		f := func(t1 *testing.T) {
+			t1.Fatal()
+		}
+		go f(t) // want "call to .+T.+Fatal from a non-test goroutine"
+
+		f = func(t2 *testing.T) {
+			t2.Error()
+		}
+		go f(t) // want "call to .+T.+Fatal from a non-test goroutine"
+	}
+	{
+		f := func(t1 *testing.T) {
+			t1.Error()
+		}
+		go f(t)
+
+		f = func(t2 *testing.T) {
+			t2.FailNow()
+		}
+		go f(t) // false negative
+	}
+}
+
+func TestGoDoesNotDescendIntoSubtest(t *testing.T) {
+	f := func(t2 *testing.T) {
+		g := func(t3 *testing.T) {
+			t3.Fatal() // fine
+		}
+		t2.Run("name", g)
+		t2.FailNow() // bad
+	}
+	go f(t) // want "call to .+T.+FailNow from a non-test goroutine"
+}
+
+func TestFreeVariableAssignedWithinEnclosing(t *testing.T) {
+	f := func(t2 *testing.T) {
+		inner := t
+		inner.FailNow()
+	}
+
+	go f(nil) // want "call to .+T.+FailNow from a non-test goroutine"
+
+	t.Run("name", func(t3 *testing.T) {
+		go f(nil) // want "call to .+T.+FailNow from a non-test goroutine"
+	})
+
+	// Without pointer analysis we cannot tell if inner is t or t2.
+	// So we accept a false negatives on the following examples.
+	t.Run("name", f)
+
+	go func(_ *testing.T) {
+		t.Run("name", f)
+	}(nil)
+
+	go t.Run("name", f)
+}
+
+func TestWithUnusedSelection(t *testing.T) {
+	go func() {
+		_ = t.FailNow
+	}()
+	t.Run("name", func(t2 *testing.T) {
+		_ = t.FailNow
+	})
+}
+
+func TestMethodExprsAreIgnored(t *testing.T) {
+	go func() {
+		(*testing.T).FailNow(t)
+	}()
+}
+
+func TestRecursive(t *testing.T) {
+	t.SkipNow()
+
+	go TestRecursive(t) // want "call to .+T.+SkipNow from a non-test goroutine"
+
+	t.Run("name", TestRecursive)
+}
+
+func TestMethodSelection(t *testing.T) {
+	var h helperType
+
+	go h.help(t) // want "call to .+T.+SkipNow from a non-test goroutine"
+	t.Run("name", h.help)
+}
+
+type helperType struct{}
+
+func (h *helperType) help(t *testing.T) { t.SkipNow() }
+
+func TestIssue63799a(t *testing.T) {
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		t.Run("", func(t *testing.T) {
+			t.Fatal() // No warning. This is in a subtest.
+		})
+	}()
+	<-done
+}
+
+func TestIssue63799b(t *testing.T) {
+	// Simplified from go.dev/cl/538698
+
+	// nondet is some unspecified boolean placeholder.
+	var nondet func() bool
+
+	t.Run("nohup", func(t *testing.T) {
+		if nondet() {
+			t.Skip("ignored")
+		}
+
+		go t.Run("nohup-i", func(t *testing.T) {
+			t.Parallel()
+			if nondet() {
+				if nondet() {
+					t.Skip("go.dev/cl/538698 wanted to have skip here")
+				}
+
+				t.Error("ignored")
+			} else {
+				t.Log("ignored")
+			}
+		})
+	})
+}
+
+func TestIssue63849(t *testing.T) {
+	go func() {
+		helper(t) // False negative. We do not do an actual interprodecural reachability analysis.
+	}()
+	go helper(t) // want "call to .+T.+Skip from a non-test goroutine"
 }

go/analysis/passes/testinggoroutine/testdata/src/a/b.go

@@ -4,4 +4,8 @@
 
 package a
 
-func h() {}
+import "testing"
+
+func helper(t *testing.T) {
+	t.Skip()
+}