cmd/vet: bring in pass to catch invalid uses of testing.T in goroutines

Add "go/analysis/passes/testinggoroutine" from x/tools and vendor its source in.
This pass will catch misuses of:
* testing.T.Fail*
* testing.T.Fatal*
* testing.T.Skip*
inside goroutines explicitly started by the go keyword.

The pass was implemented in CL 212920.

While here, found 2 misuses in:
* database/sql/sql_test.go
* runtime/syscall_windows_test.go
and fixed them in CL 235527.

Fixes #5746

Change-Id: I1740ad3f1d677bb5d78dc5d8d66bac6ec287a2b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/235677
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>

2 files changed, 156 insertions, 0 deletions

diff --git a/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.go b/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.go
new file mode 100644
index 0000000000..d2b9a5640d
--- /dev/null
+++ b/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.go
@@ -0,0 +1,154 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package testinggoroutine
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const Doc = `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.
+This checker detects calls to these functions that occur within a goroutine
+started by the test. For example:
+
+func TestFoo(t *testing.T) {
+    go func() {
+        t.Fatal("oops") // error: (*T).Fatal called from non-test goroutine
+    }()
+}
+`
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "testinggoroutine",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+var forbidden = map[string]bool{
+	"FailNow": true,
+	"Fatal":   true,
+	"Fatalf":  true,
+	"Skip":    true,
+	"Skipf":   true,
+	"SkipNow": true,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	if !analysisutil.Imports(pass.Pkg, "testing") {
+		return nil, nil
+	}
+
+	// Filter out anything that isn't a function declaration.
+	onlyFuncs := []ast.Node{
+		(*ast.FuncDecl)(nil),
+	}
+
+	inspect.Nodes(onlyFuncs, func(node ast.Node, push bool) bool {
+		fnDecl, ok := node.(*ast.FuncDecl)
+		if !ok {
+			return false
+		}
+
+		if !hasBenchmarkOrTestParams(fnDecl) {
+			return false
+		}
+
+		// Now traverse the benchmark/test's body and check that none of the
+		// forbidden methods are invoked in the goroutines within the body.
+		ast.Inspect(fnDecl, func(n ast.Node) bool {
+			goStmt, ok := n.(*ast.GoStmt)
+			if !ok {
+				return true
+			}
+
+			checkGoStmt(pass, goStmt)
+
+			// No need to further traverse the GoStmt since right
+			// above we manually traversed it in the ast.Inspect(goStmt, ...)
+			return false
+		})
+
+		return false
+	})
+
+	return nil, nil
+}
+
+func hasBenchmarkOrTestParams(fnDecl *ast.FuncDecl) bool {
+	// Check that the function's arguments include "*testing.T" or "*testing.B".
+	params := fnDecl.Type.Params.List
+
+	for _, param := range params {
+		if _, ok := typeIsTestingDotTOrB(param.Type); ok {
+			return true
+		}
+	}
+
+	return false
+}
+
+func typeIsTestingDotTOrB(expr ast.Expr) (string, bool) {
+	starExpr, ok := expr.(*ast.StarExpr)
+	if !ok {
+		return "", false
+	}
+	selExpr, ok := starExpr.X.(*ast.SelectorExpr)
+	if !ok {
+		return "", false
+	}
+
+	varPkg := selExpr.X.(*ast.Ident)
+	if varPkg.Name != "testing" {
+		return "", false
+	}
+
+	varTypeName := selExpr.Sel.Name
+	ok = varTypeName == "B" || varTypeName == "T"
+	return varTypeName, ok
+}
+
+// checkGoStmt traverses the goroutine and checks for the
+// use of the forbidden *testing.(B, T) methods.
+func checkGoStmt(pass *analysis.Pass, goStmt *ast.GoStmt) {
+	// Otherwise examine the goroutine to check for the forbidden methods.
+	ast.Inspect(goStmt, func(n ast.Node) bool {
+		selExpr, ok := n.(*ast.SelectorExpr)
+		if !ok {
+			return true
+		}
+
+		_, bad := forbidden[selExpr.Sel.Name]
+		if !bad {
+			return true
+		}
+
+		// Now filter out false positives by the import-path/type.
+		ident, ok := selExpr.X.(*ast.Ident)
+		if !ok {
+			return true
+		}
+		if ident.Obj == nil || ident.Obj.Decl == nil {
+			return true
+		}
+		field, ok := ident.Obj.Decl.(*ast.Field)
+		if !ok {
+			return true
+		}
+		if typeName, ok := typeIsTestingDotTOrB(field.Type); ok {
+			pass.ReportRangef(selExpr, "call to (*%s).%s from a non-test goroutine", typeName, selExpr.Sel)
+		}
+		return true
+	})
+}
diff --git a/src/cmd/vet/main.go b/src/cmd/vet/main.go
index 6381de840c..bad3807039 100644
--- a/src/cmd/vet/main.go
+++ b/src/cmd/vet/main.go
@@ -24,6 +24,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/stdmethods"
 	"golang.org/x/tools/go/analysis/passes/stringintconv"
 	"golang.org/x/tools/go/analysis/passes/structtag"
+	"golang.org/x/tools/go/analysis/passes/testinggoroutine"
 	"golang.org/x/tools/go/analysis/passes/tests"
 	"golang.org/x/tools/go/analysis/passes/unmarshal"
 	"golang.org/x/tools/go/analysis/passes/unreachable"
@@ -55,6 +56,7 @@ func main() {
 		stringintconv.Analyzer,
 		structtag.Analyzer,
 		tests.Analyzer,
+		testinggoroutine.Analyzer,
 		unmarshal.Analyzer,
 		unreachable.Analyzer,
 		unsafeptr.Analyzer,