go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel

Add experimental new logic to the loopclosure analyzer that checks for
access to loop variables from parallel subtests. For now, this is gated
behind an internal variable so that we may experiment with it without
yet affecting cmd/vet.

Add an internal/loopclosure command that enables the experimental new
check.

Change-Id: Ibf7e388a58a2b3e712e66e095d06612cf3ea918c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/430916
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

go/analysis/passes/loopclosure/loopclosure.go

@@ -14,15 +14,20 @@ import (
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/go/types/typeutil"
+	"golang.org/x/tools/internal/analysisinternal"
 )
 
 const Doc = `check references to loop variables from within nested functions
 
-This analyzer checks for references to loop variables from within a
-function literal inside the loop body. It checks only instances where
-the function literal is called in a defer or go statement that is the
-last statement in the loop body, as otherwise we would need whole
-program analysis.
+This analyzer checks for references to loop variables from within a function
+literal inside the loop body. It checks for patterns where access to a loop
+variable is known to escape the current loop iteration:
+ 1. a call to go or defer at the end of the loop body
+ 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
+
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
 
 For example:
 
@@ -34,6 +39,10 @@ For example:
 
 See: https://golang.org/doc/go_faq.html#closures_and_goroutines`
 
+// TODO(rfindley): enable support for checking parallel subtests, pending
+// investigation, adding:
+// 3. a call testing.T.Run where the subtest body invokes t.Parallel()
+
 var Analyzer = &analysis.Analyzer{
 	Name:     "loopclosure",
 	Doc:      Doc,
@@ -79,52 +88,71 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			return
 		}
 
-		// Inspect a go or defer statement
-		// if it's the last one in the loop body.
-		// (We give up if there are following statements,
-		// because it's hard to prove go isn't followed by wait,
-		// or defer by return.)
-		if len(body.List) == 0 {
-			return
-		}
-		// The function invoked in the last return statement.
-		var fun ast.Expr
-		switch s := body.List[len(body.List)-1].(type) {
-		case *ast.GoStmt:
-			fun = s.Call.Fun
-		case *ast.DeferStmt:
-			fun = s.Call.Fun
-		case *ast.ExprStmt: // check for errgroup.Group.Go()
-			if call, ok := s.X.(*ast.CallExpr); ok {
-				fun = goInvokes(pass.TypesInfo, call)
-			}
-		}
-		lit, ok := fun.(*ast.FuncLit)
-		if !ok {
-			return
-		}
-		ast.Inspect(lit.Body, func(n ast.Node) bool {
-			id, ok := n.(*ast.Ident)
-			if !ok || id.Obj == nil {
-				return true
-			}
-			if pass.TypesInfo.Types[id].Type == nil {
-				// Not referring to a variable (e.g. struct field name)
-				return true
-			}
-			for _, v := range vars {
-				if v.Obj == id.Obj {
-					pass.ReportRangef(id, "loop variable %s captured by func literal",
-						id.Name)
+		// Inspect statements to find function literals that may be run outside of
+		// the current loop iteration.
+		//
+		// For go, defer, and errgroup.Group.Go, we ignore all but the last
+		// statement, because it's hard to prove go isn't followed by wait, or
+		// defer by return.
+		//
+		// We consider every t.Run statement in the loop body, because there is
+		// no such commonly used mechanism for synchronizing parallel subtests.
+		// It is of course theoretically possible to synchronize parallel subtests,
+		// though such a pattern is likely to be exceedingly rare as it would be
+		// fighting against the test runner.
+		lastStmt := len(body.List) - 1
+		for i, s := range body.List {
+			var fun ast.Expr // if non-nil, a function that escapes the loop iteration
+			switch s := s.(type) {
+			case *ast.GoStmt:
+				if i == lastStmt {
+					fun = s.Call.Fun
+				}
+
+			case *ast.DeferStmt:
+				if i == lastStmt {
+					fun = s.Call.Fun
+				}
+
+			case *ast.ExprStmt: // check for errgroup.Group.Go and testing.T.Run (with T.Parallel)
+				if call, ok := s.X.(*ast.CallExpr); ok {
+					if i == lastStmt {
+						fun = goInvoke(pass.TypesInfo, call)
+					}
+					if fun == nil && analysisinternal.LoopclosureParallelSubtests {
+						fun = parallelSubtest(pass.TypesInfo, call)
+					}
 				}
 			}
-			return true
-		})
+
+			lit, ok := fun.(*ast.FuncLit)
+			if !ok {
+				continue
+			}
+
+			ast.Inspect(lit.Body, func(n ast.Node) bool {
+				id, ok := n.(*ast.Ident)
+				if !ok || id.Obj == nil {
+					return true
+				}
+				if pass.TypesInfo.Types[id].Type == nil {
+					// Not referring to a variable (e.g. struct field name)
+					return true
+				}
+				for _, v := range vars {
+					if v.Obj == id.Obj {
+						pass.ReportRangef(id, "loop variable %s captured by func literal",
+							id.Name)
+					}
+				}
+				return true
+			})
+		}
 	})
 	return nil, nil
 }
 
-// goInvokes returns a function expression that would be called asynchronously
+// goInvoke returns a function expression that would be called asynchronously
 // (but not awaited) in another goroutine as a consequence of the call.
 // For example, given the g.Go call below, it returns the function literal expression.
 //
@@ -133,33 +161,89 @@ func run(pass *analysis.Pass) (interface{}, error) {
 //	g.Go(func() error { ... })
 //
 // Currently only "golang.org/x/sync/errgroup.Group()" is considered.
-func goInvokes(info *types.Info, call *ast.CallExpr) ast.Expr {
-	f := typeutil.StaticCallee(info, call)
-	// Note: Currently only supports: golang.org/x/sync/errgroup.Go.
-	if f == nil || f.Name() != "Go" {
-		return nil
-	}
-	recv := f.Type().(*types.Signature).Recv()
-	if recv == nil {
-		return nil
-	}
-	rtype, ok := recv.Type().(*types.Pointer)
-	if !ok {
-		return nil
-	}
-	named, ok := rtype.Elem().(*types.Named)
-	if !ok {
-		return nil
-	}
-	if named.Obj().Name() != "Group" {
-		return nil
-	}
-	pkg := f.Pkg()
-	if pkg == nil {
-		return nil
-	}
-	if pkg.Path() != "golang.org/x/sync/errgroup" {
+func goInvoke(info *types.Info, call *ast.CallExpr) ast.Expr {
+	if !isMethodCall(info, call, "golang.org/x/sync/errgroup", "Group", "Go") {
 		return nil
 	}
 	return call.Args[0]
 }
+
+// parallelSubtest returns a function expression that would be called
+// asynchronously via the go test runner, as t.Run has been invoked with a
+// function literal that calls t.Parallel.
+//
+//		import "testing"
+//
+//		func TestFoo(t *testing.T) {
+//			tests := []int{0, 1, 2}
+//			for i, t := range tests {
+//			 	t.Run("subtest", func(t *testing.T) {
+//	 				t.Parallel()
+//			 		println(i, t)
+//			 	})
+//		 	}
+//		}
+func parallelSubtest(info *types.Info, call *ast.CallExpr) ast.Expr {
+	if !isMethodCall(info, call, "testing", "T", "Run") {
+		return nil
+	}
+
+	lit, ok := call.Args[1].(*ast.FuncLit)
+	if !ok {
+		return nil
+	}
+
+	for _, stmt := range lit.Body.List {
+		exprStmt, ok := stmt.(*ast.ExprStmt)
+		if !ok {
+			continue
+		}
+		if isMethodCall(info, exprStmt.X, "testing", "T", "Parallel") {
+			return lit
+		}
+	}
+
+	return nil
+}
+
+// isMethodCall reports whether expr is a method call of
+// <pkgPath>.<typeName>.<method>.
+func isMethodCall(info *types.Info, expr ast.Expr, pkgPath, typeName, method string) bool {
+	call, ok := expr.(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+
+	// Check that we are calling a method <method>
+	f := typeutil.StaticCallee(info, call)
+	if f == nil || f.Name() != method {
+		return false
+	}
+	recv := f.Type().(*types.Signature).Recv()
+	if recv == nil {
+		return false
+	}
+
+	// Check that the receiver is a <pkgPath>.<typeName> or
+	// *<pkgPath>.<typeName>.
+	rtype := recv.Type()
+	if ptr, ok := recv.Type().(*types.Pointer); ok {
+		rtype = ptr.Elem()
+	}
+	named, ok := rtype.(*types.Named)
+	if !ok {
+		return false
+	}
+	if named.Obj().Name() != typeName {
+		return false
+	}
+	pkg := f.Pkg()
+	if pkg == nil {
+		return false
+	}
+	if pkg.Path() != pkgPath {
+		return false
+	}
+
+	return true
+}

go/analysis/passes/loopclosure/loopclosure_test.go

@@ -5,9 +5,11 @@
 package loopclosure_test
 
 import (
-	"golang.org/x/tools/internal/typeparams"
 	"testing"
 
+	"golang.org/x/tools/internal/analysisinternal"
+	"golang.org/x/tools/internal/typeparams"
+
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/loopclosure"
 )
@@ -19,4 +21,12 @@ func Test(t *testing.T) {
 		tests = append(tests, "typeparams")
 	}
 	analysistest.Run(t, testdata, loopclosure.Analyzer, tests...)
+
+	// Enable checking of parallel subtests.
+	defer func(parallelSubtest bool) {
+		analysisinternal.LoopclosureParallelSubtests = parallelSubtest
+	}(analysisinternal.LoopclosureParallelSubtests)
+	analysisinternal.LoopclosureParallelSubtests = true
+
+	analysistest.Run(t, testdata, loopclosure.Analyzer, "subtests")
 }

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

@@ -6,7 +6,9 @@
 
 package testdata
 
-import "golang.org/x/sync/errgroup"
+import (
+	"golang.org/x/sync/errgroup"
+)
 
 func _() {
 	var s []int
@@ -91,9 +93,9 @@ func _() {
 	}
 }
 
-// Group is used to test that loopclosure does not match on any type named "Group".
-// The checker only matches on methods "(*...errgroup.Group).Go".
-type Group struct{};
+// Group is used to test that loopclosure only matches Group.Go when Group is
+// from the golang.org/x/sync/errgroup package.
+type Group struct{}
 
 func (g *Group) Go(func() error) {}
 

go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go

@@ -0,0 +1,99 @@
+// Copyright 2022 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.
+
+// This file contains tests that the loopclosure analyzer detects leaked
+// references via parallel subtests.
+
+package subtests
+
+import (
+	"testing"
+)
+
+// T is used to test that loopclosure only matches T.Run when T is from the
+// testing package.
+type T struct{}
+
+// Run should not match testing.T.Run. Note that the second argument is
+// intentionally a *testing.T, not a *T, so that we can check both
+// testing.T.Parallel inside a T.Run, and a T.Parallel inside a testing.T.Run.
+func (t *T) Run(string, func(*testing.T)) { // The second argument here is testing.T
+}
+
+func (t *T) Parallel() {}
+
+func _(t *testing.T) {
+	for i, test := range []int{1, 2, 3} {
+		// Check that parallel subtests are identified.
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			println(i)    // want "loop variable i captured by func literal"
+			println(test) // want "loop variable test captured by func literal"
+		})
+
+		// Check that serial tests are OK.
+		t.Run("", func(t *testing.T) {
+			println(i)
+			println(test)
+		})
+
+		// Check that the location of t.Parallel does not matter.
+		t.Run("", func(t *testing.T) {
+			println(i)    // want "loop variable i captured by func literal"
+			println(test) // want "loop variable test captured by func literal"
+			t.Parallel()
+		})
+
+		// Check uses in nested blocks.
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			{
+				println(i)    // want "loop variable i captured by func literal"
+				println(test) // want "loop variable test captured by func literal"
+			}
+		})
+
+		// Check that we catch uses in nested subtests.
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			t.Run("", func(t *testing.T) {
+				println(i)    // want "loop variable i captured by func literal"
+				println(test) // want "loop variable test captured by func literal"
+			})
+		})
+
+		// Check that there is no diagnostic if t is not a *testing.T.
+		t.Run("", func(_ *testing.T) {
+			t := &T{}
+			t.Parallel()
+			println(i)
+			println(test)
+		})
+	}
+}
+
+// Check that there is no diagnostic when loop variables are shadowed within
+// the loop body.
+func _(t *testing.T) {
+	for i, test := range []int{1, 2, 3} {
+		i := i
+		test := test
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			println(i)
+			println(test)
+		})
+	}
+}
+
+// Check that t.Run must be *testing.T.Run.
+func _(t *T) {
+	for i, test := range []int{1, 2, 3} {
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			println(i)
+			println(test)
+		})
+	}
+}

gopls/doc/analyzers.md

@@ -218,11 +218,15 @@ inferred from function arguments, or from other type arguments:
 
 check references to loop variables from within nested functions
 
-This analyzer checks for references to loop variables from within a
-function literal inside the loop body. It checks only instances where
-the function literal is called in a defer or go statement that is the
-last statement in the loop body, as otherwise we would need whole
-program analysis.
+This analyzer checks for references to loop variables from within a function
+literal inside the loop body. It checks for patterns where access to a loop
+variable is known to escape the current loop iteration:
+ 1. a call to go or defer at the end of the loop body
+ 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
+
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
 
 For example:
 

gopls/internal/lsp/source/api_json.go

@@ -300,7 +300,7 @@ var GeneratedAPIJSON = &APIJSON{
 						},
 						{
 							Name:    "\"loopclosure\"",
-							Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+							Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
 							Default: "true",
 						},
 						{
@@ -926,7 +926,7 @@ var GeneratedAPIJSON = &APIJSON{
 		},
 		{
 			Name:    "loopclosure",
-			Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+			Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
 			Default: true,
 		},
 		{

internal/analysisinternal/analysis.go

@@ -14,9 +14,14 @@ import (
 	"strconv"
 )
 
-// Flag to gate diagnostics for fuzz tests in 1.18.
+// DiagnoseFuzzTests controls whether the 'tests' analyzer diagnoses fuzz tests
+// in Go 1.18+.
 var DiagnoseFuzzTests bool = false
 
+// LoopclosureParallelSubtests controls whether the 'loopclosure' analyzer
+// diagnoses loop variables references in parallel subtests.
+var LoopclosureParallelSubtests = false
+
 var (
 	GetTypeErrors func(p interface{}) []types.Error
 	SetTypeErrors func(p interface{}, errors []types.Error)

internal/loopclosure/main.go

@@ -0,0 +1,22 @@
+// Copyright 2022 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.
+
+// The loopclosure command applies the golang.org/x/tools/go/analysis/passes/loopclosure
+// analysis to the specified packages of Go source code. It enables
+// experimental checking of parallel subtests.
+//
+// TODO: Once the parallel subtest experiment is complete, this can be made
+// public at go/analysis/passes/loopclosure/cmd, or deleted.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/passes/loopclosure"
+	"golang.org/x/tools/go/analysis/singlechecker"
+	"golang.org/x/tools/internal/analysisinternal"
+)
+
+func main() {
+	analysisinternal.LoopclosureParallelSubtests = true
+	singlechecker.Main(loopclosure.Analyzer)
+}