diff --git a/README.md b/README.md index b964486..110c182 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,41 @@ go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest -custom-funcs ## Examples +### ❌ Outer-scope variable reassigned in a parallel subtest + +Sibling parallel subtests can race on a variable declared in an enclosing +scope. The fix is almost always to shadow with `:=`. + +```go +func TestBad(t *testing.T) { + t.Parallel() + var err error + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // WARNING: `err` is shared across parallel subtests. + _, err = doThing(tc) + require.NoError(t, err) + }) + } +} +``` + +```go +func TestGood(t *testing.T) { + t.Parallel() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := doThing(tc) // shadowed + require.NoError(t, err) + }) + } +} +``` + + + ### ❌ Potentially flakey test ```go diff --git a/pkg/paralleltestctx/analyzer.go b/pkg/paralleltestctx/analyzer.go index 3bc4909..a64464d 100644 --- a/pkg/paralleltestctx/analyzer.go +++ b/pkg/paralleltestctx/analyzer.go @@ -305,12 +305,14 @@ func (a *ctxAnalyzer) analyzeTestFunction(pass *analysis.Pass, fd *ast.FuncDecl, // Collect all timeout contexts and their positions timeoutCtxs := a.collectTimeoutContexts(pass, fd, helpers) - if len(timeoutCtxs) == 0 { - return + + timeoutCtxObjs := make(map[types.Object]struct{}, len(timeoutCtxs)) + for _, c := range timeoutCtxs { + timeoutCtxObjs[c.obj] = struct{}{} } // Find all t.Parallel() calls and check for context usage after them - a.checkContextUsageAfterParallel(pass, fd, testVarName, timeoutCtxs) + a.analyzeScope(pass, fd.Body, testVarName, timeoutCtxs, timeoutCtxObjs) } func isTestFunction(fd *ast.FuncDecl) bool { @@ -395,14 +397,8 @@ func (a *ctxAnalyzer) collectTimeoutContexts(pass *analysis.Pass, fd *ast.FuncDe return contexts } -// checkContextUsageAfterParallel walks through the AST and reports timeout context usage after t.Parallel() calls -func (a *ctxAnalyzer) checkContextUsageAfterParallel(pass *analysis.Pass, fd *ast.FuncDecl, testVarName string, timeoutCtxs []timeoutContext) { - // Analyze each function scope (main test and subtests) separately - a.analyzeScope(pass, fd.Body, testVarName, timeoutCtxs) -} - // analyzeScope analyzes a specific scope (test function or subtest) for the pattern -func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarName string, timeoutCtxs []timeoutContext) { +func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarName string, timeoutCtxs []timeoutContext, timeoutCtxObjs map[types.Object]struct{}) { var parallelCalls []ast.Node reportedNodes := make(map[ast.Node]bool) // Track nodes we've already reported @@ -412,7 +408,7 @@ func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarN // Analyze the subtest scope separately with its own test variable subtestVarName := a.getSubtestParamName(fl) if subtestVarName != "" { - a.analyzeScope(pass, fl, subtestVarName, timeoutCtxs) + a.analyzeScope(pass, fl, subtestVarName, timeoutCtxs, timeoutCtxObjs) } return false // Don't continue into this scope } @@ -448,6 +444,8 @@ func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarN return true }) + + a.analyzeOuterVarReassignments(pass, scope, parallelCalls, timeoutCtxObjs) } // checkContextViolation checks if a context identifier violates the t.Parallel usage rules diff --git a/pkg/paralleltestctx/analyzer_test.go b/pkg/paralleltestctx/analyzer_test.go index 3d1c6b4..a1fa26f 100644 --- a/pkg/paralleltestctx/analyzer_test.go +++ b/pkg/paralleltestctx/analyzer_test.go @@ -29,3 +29,8 @@ func TestCustomWithReceiver(t *testing.T) { analysistest.Run(t, testdata, analyzer.analyzer, "custom") } + +func TestOuterVars(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, Analyzer(), "outervars") +} diff --git a/pkg/paralleltestctx/outervars.go b/pkg/paralleltestctx/outervars.go new file mode 100644 index 0000000..415da20 --- /dev/null +++ b/pkg/paralleltestctx/outervars.go @@ -0,0 +1,72 @@ +package paralleltestctx + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" +) + +// analyzeOuterVarReassignments flags an outer-scope variable that is +// reassigned with `=` inside a parallel subtest scope. Sibling parallel +// subtests can race on the shared variable, and the fix is almost always +// to shadow with `:=`. +// +// Constraints (deliberately tight to keep the FP rate low): +// +// 1. The enclosing scope must contain a t.Parallel() call. +// 2. The assignment must use `=`, not `:=`. +// 3. The LHS must be an *ast.Ident (not a SelectorExpr, IndexExpr, etc.). +// 4. The Ident must resolve to a *types.Var declared OUTSIDE this scope. +// 5. The object must not already be tracked as a timeout context — the +// existing context-specific diagnostic covers that case. +func (a *ctxAnalyzer) analyzeOuterVarReassignments( + pass *analysis.Pass, + scope ast.Node, + parallelCalls []ast.Node, + timeoutCtxObjs map[types.Object]struct{}, +) { + if len(parallelCalls) == 0 { + return + } + scopeStart := scope.Pos() + scopeEnd := scope.End() + + ast.Inspect(scope, func(n ast.Node) bool { + if fl, ok := n.(*ast.FuncLit); ok && ast.Node(fl) != scope { + // Nested subtest body has its own scope; analyzed separately. + return false + } + as, ok := n.(*ast.AssignStmt) + if !ok || as.Tok != token.ASSIGN { + return true + } + for _, lhs := range as.Lhs { + id, ok := lhs.(*ast.Ident) + if !ok || id.Name == "_" { + continue + } + obj := pass.TypesInfo.Uses[id] + if obj == nil { + continue + } + v, ok := obj.(*types.Var) + if !ok { + continue + } + if v.Pos() >= scopeStart && v.Pos() <= scopeEnd { + continue + } + if _, isCtx := timeoutCtxObjs[obj]; isCtx { + continue + } + pass.Reportf( + as.Pos(), + "outer-scope variable %s reassigned inside a parallel subtest; did you mean to shadow with := ?", + id.Name, + ) + } + return true + }) +} diff --git a/pkg/paralleltestctx/testdata/src/outervars/outervars_test.go b/pkg/paralleltestctx/testdata/src/outervars/outervars_test.go new file mode 100644 index 0000000..f465fbb --- /dev/null +++ b/pkg/paralleltestctx/testdata/src/outervars/outervars_test.go @@ -0,0 +1,83 @@ +package outervars + +import ( + "errors" + "testing" +) + +// Bug: outer `err` reassigned (=) inside parallel subtest. +// Mirrors https://github.com/coder/coder/pull/25980. +func TestShadowedErrWarn(t *testing.T) { + t.Parallel() + cases := []struct{ name string }{{"a"}, {"b"}} + var err error + _ = err + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err = doThing(tc.name) // want "outer-scope variable err reassigned" + if err == nil { + t.Fatal("expected error") + } + }) + } +} + +// OK: `:=` shadows. +func TestShadowedErrOK(t *testing.T) { + t.Parallel() + t.Run("sub", func(t *testing.T) { + t.Parallel() + _, err := doThing("a") + _ = err + }) +} + +// OK: subtest is not parallel. +func TestNonParallelSubtestOK(t *testing.T) { + var err error + _ = err + t.Run("sub", func(t *testing.T) { + _, err = doThing("a") + _ = err + }) +} + +// OK: variable is declared inside the subtest body. +func TestInnerVarOK(t *testing.T) { + t.Parallel() + t.Run("sub", func(t *testing.T) { + t.Parallel() + var err error + _, err = doThing("a") + _ = err + }) +} + +// OK: LHS is a SelectorExpr, not an Ident. +func TestStructFieldOK(t *testing.T) { + t.Parallel() + state := &struct{ err error }{} + t.Run("sub", func(t *testing.T) { + t.Parallel() + state.err = errors.New("x") + _ = state.err + }) +} + +// Multi-LHS: both outer vars get flagged. +func TestMultiLHSWarn(t *testing.T) { + t.Parallel() + var got int + var err error + _, _ = got, err + t.Run("sub", func(t *testing.T) { + t.Parallel() + got, err = doInt() // want "outer-scope variable got reassigned" "outer-scope variable err reassigned" + _ = got + _ = err + }) +} + +func doThing(s string) (string, error) { return s, errors.New("x") } +func doInt() (int, error) { return 1, nil }