go/analysis/passes/loopclosure: enable analysis of parallel subtests

Remove the internal guard preventing the loopclosure analyzer from
checking parallel subtests.

Also, improve the accuracy of the parallel subtest check:
 - only consider statements after the final labeled statement in the
   subtest body
 - verify that the *testing.T value for which T.Parallel() is invoked
   matches the argument to the subtest literal

Fixes golang/go#55972

Change-Id: Ia2d9e08dfa88b5e31a9151872025272560d4b5e8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447256
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>

go/analysis/passes/loopclosure/loopclosure.go

@@ -14,7 +14,6 @@ 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
@@ -24,10 +23,11 @@ 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
+ 3. a call testing.T.Run where the subtest body invokes t.Parallel()
 
-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.
+In the case of (1) and (2), 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:
 
@@ -39,10 +39,6 @@ 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,
@@ -121,7 +117,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 					if i == lastStmt {
 						stmts = litStmts(goInvoke(pass.TypesInfo, call))
 					}
-					if stmts == nil && analysisinternal.LoopclosureParallelSubtests {
+					if stmts == nil {
 						stmts = parallelSubtest(pass.TypesInfo, call)
 					}
 				}
@@ -178,15 +174,19 @@ func goInvoke(info *types.Info, call *ast.CallExpr) ast.Expr {
 	return call.Args[0]
 }
 
-// parallelSubtest returns statements that would would be executed
-// asynchronously via the go test runner, as t.Run has been invoked with a
+// parallelSubtest returns statements that can be easily proven to execute
+// concurrently via the go test runner, as t.Run has been invoked with a
 // function literal that calls t.Parallel.
 //
 // In practice, users rely on the fact that statements before the call to
 // t.Parallel are synchronous. For example by declaring test := test inside the
 // function literal, but before the call to t.Parallel.
 //
-// Therefore, we only flag references that occur after the call to t.Parallel:
+// Therefore, we only flag references in statements that are obviously
+// dominated by a call to t.Parallel. As a simple heuristic, we only consider
+// statements following the final labeled statement in the function body, to
+// avoid scenarios where a jump would cause either the call to t.Parallel or
+// the problematic reference to be skipped.
 //
 //	import "testing"
 //
@@ -210,17 +210,81 @@ func parallelSubtest(info *types.Info, call *ast.CallExpr) []ast.Stmt {
 		return nil
 	}
 
-	for i, stmt := range lit.Body.List {
+	// Capture the *testing.T object for the first argument to the function
+	// literal.
+	if len(lit.Type.Params.List[0].Names) == 0 {
+		return nil
+	}
+
+	tObj := info.Defs[lit.Type.Params.List[0].Names[0]]
+	if tObj == nil {
+		return nil
+	}
+
+	// Match statements that occur after a call to t.Parallel following the final
+	// labeled statement in the function body.
+	//
+	// We iterate over lit.Body.List to have a simple, fast and "frequent enough"
+	// dominance relationship for t.Parallel(): lit.Body.List[i] dominates
+	// lit.Body.List[j] for i < j unless there is a jump.
+	var stmts []ast.Stmt
+	afterParallel := false
+	for _, stmt := range lit.Body.List {
+		stmt, labeled := unlabel(stmt)
+		if labeled {
+			// Reset: naively we don't know if a jump could have caused the
+			// previously considered statements to be skipped.
+			stmts = nil
+			afterParallel = false
+		}
+
+		if afterParallel {
+			stmts = append(stmts, stmt)
+			continue
+		}
+
+		// Check if stmt is a call to t.Parallel(), for the correct t.
 		exprStmt, ok := stmt.(*ast.ExprStmt)
 		if !ok {
 			continue
 		}
-		if isMethodCall(info, exprStmt.X, "testing", "T", "Parallel") {
-			return lit.Body.List[i+1:]
+		expr := exprStmt.X
+		if isMethodCall(info, expr, "testing", "T", "Parallel") {
+			call, _ := expr.(*ast.CallExpr)
+			if call == nil {
+				continue
+			}
+			x, _ := call.Fun.(*ast.SelectorExpr)
+			if x == nil {
+				continue
+			}
+			id, _ := x.X.(*ast.Ident)
+			if id == nil {
+				continue
+			}
+			if info.Uses[id] == tObj {
+				afterParallel = true
+			}
 		}
 	}
 
-	return nil
+	return stmts
+}
+
+// unlabel returns the inner statement for the possibly labeled statement stmt,
+// stripping any (possibly nested) *ast.LabeledStmt wrapper.
+//
+// The second result reports whether stmt was an *ast.LabeledStmt.
+func unlabel(stmt ast.Stmt) (ast.Stmt, bool) {
+	labeled := false
+	for {
+		labelStmt, ok := stmt.(*ast.LabeledStmt)
+		if !ok {
+			return stmt, labeled
+		}
+		labeled = true
+		stmt = labelStmt.Stmt
+	}
 }
 
 // isMethodCall reports whether expr is a method call of

go/analysis/passes/loopclosure/loopclosure_test.go

@@ -9,23 +9,14 @@ import (
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/loopclosure"
-	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/typeparams"
 )
 
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
-	tests := []string{"a", "golang.org/..."}
+	tests := []string{"a", "golang.org/...", "subtests"}
 	if typeparams.Enabled {
 		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/subtests/subtest.go

@@ -47,6 +47,14 @@ func _(t *testing.T) {
 			println(test) // want "loop variable test captured by func literal"
 		})
 
+		// Check that *testing.T value matters.
+		t.Run("", func(t *testing.T) {
+			var x testing.T
+			x.Parallel()
+			println(i)
+			println(test)
+		})
+
 		// Check that shadowing the loop variables within the test literal is OK if
 		// it occurs before t.Parallel().
 		t.Run("", func(t *testing.T) {
@@ -92,6 +100,46 @@ func _(t *testing.T) {
 			println(i)
 			println(test)
 		})
+
+		// Check that there is no diagnostic when a jump to a label may have caused
+		// the call to t.Parallel to have been skipped.
+		t.Run("", func(t *testing.T) {
+			if true {
+				goto Test
+			}
+			t.Parallel()
+		Test:
+			println(i)
+			println(test)
+		})
+
+		// Check that there is no diagnostic when a jump to a label may have caused
+		// the loop variable reference to be skipped, but there is a diagnostic
+		// when both the call to t.Parallel and the loop variable reference occur
+		// after the final label in the block.
+		t.Run("", func(t *testing.T) {
+			if true {
+				goto Test
+			}
+			t.Parallel()
+			println(i) // maybe OK
+		Test:
+			t.Parallel()
+			println(test) // want "loop variable test captured by func literal"
+		})
+
+		// Check that multiple labels are handled.
+		t.Run("", func(t *testing.T) {
+			if true {
+				goto Test1
+			} else {
+				goto Test2
+			}
+		Test1:
+		Test2:
+			t.Parallel()
+			println(test) // want "loop variable test captured by func literal"
+		})
 	}
 }
 
@@ -119,3 +167,32 @@ func _(t *T) {
 		})
 	}
 }
+
+// Check that the top-level must be parallel in order to cause a diagnostic.
+//
+// From https://pkg.go.dev/testing:
+//
+//	"Run does not return until parallel subtests have completed, providing a
+//	way to clean up after a group of parallel tests"
+func _(t *testing.T) {
+	for _, test := range []int{1, 2, 3} {
+		// In this subtest, a/b must complete before the synchronous subtest "a"
+		// completes, so the reference to test does not escape the current loop
+		// iteration.
+		t.Run("a", func(s *testing.T) {
+			s.Run("b", func(u *testing.T) {
+				u.Parallel()
+				println(test)
+			})
+		})
+
+		// In this subtest, c executes concurrently, so the reference to test may
+		// escape the current loop iteration.
+		t.Run("c", func(s *testing.T) {
+			s.Parallel()
+			s.Run("d", func(u *testing.T) {
+				println(test) // want "loop variable test captured by func literal"
+			})
+		})
+	}
+}

gopls/doc/analyzers.md

@@ -223,10 +223,11 @@ 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
+ 3. a call testing.T.Run where the subtest body invokes t.Parallel()
 
-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.
+In the case of (1) and (2), 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:
 

internal/analysisinternal/analysis.go

@@ -18,10 +18,6 @@ import (
 // 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)