From: Emmanuel T Odeke Date: Fri, 29 May 2020 09:17:38 +0000 (-0700) Subject: cmd/vet: bring in pass to catch invalid uses of testing.T in goroutines X-Git-Tag: go1.16beta1~366 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=5a267c840ae16c1cc7352caa14da5f500d03d338;p=gostls13.git 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 TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor Trust: Emmanuel Odeke --- 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,