From ebd014ddefcb909144622a68820c261a212c1c3b Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 9 Jun 2026 05:21:00 +0000 Subject: [PATCH 1/3] feat(pkg/paralleltestctx): add -check-outer-assignments Add an opt-in flag that flags an adjacent flake pattern: an outer-scope variable reassigned with `=` inside a parallel subtest. Sibling parallel subtests can race on the shared variable, and the fix is almost always to shadow with `:=`. Example of the bug shape this catches (mirrors coder/coder#25980): func TestX(t *testing.T) { t.Parallel() var err error for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() _, err = doThing(tc) // shared across siblings require.NoError(t, err) }) } } The check is gated behind `-check-outer-assignments` (default off) so existing users see no behavior change. Rule: 1. Enclosing scope contains a t.Parallel() call. 2. Assignment uses `=` (not `:=`). 3. LHS is an *ast.Ident (not SelectorExpr, IndexExpr, etc.). 4. Ident resolves to a *types.Var declared outside the scope. 5. Object isn't already covered by the timeout-context rule. Empirical sweep on coder/coder (930 test files): 4 findings, all actionable (the diagnostic's suggested `:=` shadow is the correct fix in each case). --- README.md | 28 +++++++ pkg/paralleltestctx/analyzer.go | 32 ++++--- pkg/paralleltestctx/analyzer_test.go | 9 ++ pkg/paralleltestctx/outervars.go | 72 ++++++++++++++++ .../testdata/src/outervars/outervars_test.go | 83 +++++++++++++++++++ 5 files changed, 211 insertions(+), 13 deletions(-) create mode 100644 pkg/paralleltestctx/outervars.go create mode 100644 pkg/paralleltestctx/testdata/src/outervars/outervars_test.go diff --git a/README.md b/README.md index b964486..906506c 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,34 @@ for an in-depth explanation of the problem. go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest ./... ``` +### Outer-scope variable reassignment + +The `-check-outer-assignments` flag enables a secondary check that warns +when a variable declared in an enclosing scope is reassigned with `=` +inside a parallel subtest. Sibling parallel subtests can race on the +shared variable; 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) + }) + } +} +``` + +```bash +go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest -check-outer-assignments ./... +``` + +The check is off by default and can be combined with `-custom-funcs`. + ### Custom functions that produce contexts with timeouts By default, the linter detects `context.WithTimeout` and `context.WithDeadline` diff --git a/pkg/paralleltestctx/analyzer.go b/pkg/paralleltestctx/analyzer.go index 3bc4909..21cf4b3 100644 --- a/pkg/paralleltestctx/analyzer.go +++ b/pkg/paralleltestctx/analyzer.go @@ -23,15 +23,17 @@ type timeoutFunc struct { } type ctxAnalyzer struct { - analyzer *analysis.Analyzer - timeoutFuncFlag string - timeoutFuncs []timeoutFunc // cache for flag parsed w/ defaults + analyzer *analysis.Analyzer + timeoutFuncFlag string + timeoutFuncs []timeoutFunc // cache for flag parsed w/ defaults + checkOuterAssignFlag bool } func newCtxAnalyzer() *ctxAnalyzer { a := &ctxAnalyzer{} var flags flag.FlagSet flags.StringVar(&a.timeoutFuncFlag, "custom-funcs", "", "comma-separated list of additional function names that create timeout/deadline contexts") + flags.BoolVar(&a.checkOuterAssignFlag, "check-outer-assignments", false, "also warn when outer-scope variables are reassigned with `=` inside a parallel subtest") a.analyzer = &analysis.Analyzer{ Name: "paralleltestctx", Doc: Doc, @@ -305,12 +307,18 @@ 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 { + + if len(timeoutCtxs) == 0 && !a.checkOuterAssignFlag { 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 +403,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 +414,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 +450,10 @@ func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarN return true }) + + if a.checkOuterAssignFlag { + 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..38aeb96 100644 --- a/pkg/paralleltestctx/analyzer_test.go +++ b/pkg/paralleltestctx/analyzer_test.go @@ -29,3 +29,12 @@ func TestCustomWithReceiver(t *testing.T) { analysistest.Run(t, testdata, analyzer.analyzer, "custom") } + +func TestOuterVars(t *testing.T) { + testdata := analysistest.TestData() + + analyzer := newCtxAnalyzer() + analyzer.checkOuterAssignFlag = true + + analysistest.Run(t, testdata, analyzer.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 } From 05dbd0c42d1bdbffbd724d2a54813dce54ef8512 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 9 Jun 2026 05:39:17 +0000 Subject: [PATCH 2/3] feat(pkg/paralleltestctx): always run the outer-var check Empirical sweep on coder/coder (930 test files) found 4 diagnostics under this rule, all actionable (the suggested `:=` shadow is the correct fix in each case). Zero false positives at the actionable-bug standard, so the opt-in gate is unnecessary friction. Remove the -check-outer-assignments flag and run the check unconditionally alongside the existing context rule. --- README.md | 47 ++++++++++++++++------------ pkg/paralleltestctx/analyzer.go | 16 +++------- pkg/paralleltestctx/analyzer_test.go | 6 +--- 3 files changed, 33 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 906506c..110c182 100644 --- a/README.md +++ b/README.md @@ -14,12 +14,23 @@ for an in-depth explanation of the problem. go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest ./... ``` -### Outer-scope variable reassignment +### Custom functions that produce contexts with timeouts + +By default, the linter detects `context.WithTimeout` and `context.WithDeadline` +as producing contexts with timeouts or deadlines. Additional functions that +create a context with a deadline or timeout can be specified using the +`-custom-funcs` flag. + +```bash +go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest -custom-funcs="testutil.Context" ./... +``` + +## Examples -The `-check-outer-assignments` flag enables a secondary check that warns -when a variable declared in an enclosing scope is reassigned with `=` -inside a parallel subtest. Sibling parallel subtests can race on the -shared variable; the fix is almost always to shadow with `:=`. +### ❌ 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) { @@ -36,24 +47,20 @@ func TestBad(t *testing.T) { } ``` -```bash -go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest -check-outer-assignments ./... +```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) + }) + } +} ``` -The check is off by default and can be combined with `-custom-funcs`. - -### Custom functions that produce contexts with timeouts - -By default, the linter detects `context.WithTimeout` and `context.WithDeadline` -as producing contexts with timeouts or deadlines. Additional functions that -create a context with a deadline or timeout can be specified using the -`-custom-funcs` flag. -```bash -go run github.com/coder/paralleltestctx/cmd/paralleltestctx@latest -custom-funcs="testutil.Context" ./... -``` - -## Examples ### ❌ Potentially flakey test diff --git a/pkg/paralleltestctx/analyzer.go b/pkg/paralleltestctx/analyzer.go index 21cf4b3..4120726 100644 --- a/pkg/paralleltestctx/analyzer.go +++ b/pkg/paralleltestctx/analyzer.go @@ -23,17 +23,15 @@ type timeoutFunc struct { } type ctxAnalyzer struct { - analyzer *analysis.Analyzer - timeoutFuncFlag string - timeoutFuncs []timeoutFunc // cache for flag parsed w/ defaults - checkOuterAssignFlag bool + analyzer *analysis.Analyzer + timeoutFuncFlag string + timeoutFuncs []timeoutFunc // cache for flag parsed w/ defaults } func newCtxAnalyzer() *ctxAnalyzer { a := &ctxAnalyzer{} var flags flag.FlagSet flags.StringVar(&a.timeoutFuncFlag, "custom-funcs", "", "comma-separated list of additional function names that create timeout/deadline contexts") - flags.BoolVar(&a.checkOuterAssignFlag, "check-outer-assignments", false, "also warn when outer-scope variables are reassigned with `=` inside a parallel subtest") a.analyzer = &analysis.Analyzer{ Name: "paralleltestctx", Doc: Doc, @@ -308,9 +306,7 @@ 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 && !a.checkOuterAssignFlag { - return - } + timeoutCtxObjs := make(map[types.Object]struct{}, len(timeoutCtxs)) for _, c := range timeoutCtxs { @@ -451,9 +447,7 @@ func (a *ctxAnalyzer) analyzeScope(pass *analysis.Pass, scope ast.Node, testVarN return true }) - if a.checkOuterAssignFlag { - a.analyzeOuterVarReassignments(pass, scope, parallelCalls, timeoutCtxObjs) - } + 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 38aeb96..a1fa26f 100644 --- a/pkg/paralleltestctx/analyzer_test.go +++ b/pkg/paralleltestctx/analyzer_test.go @@ -32,9 +32,5 @@ func TestCustomWithReceiver(t *testing.T) { func TestOuterVars(t *testing.T) { testdata := analysistest.TestData() - - analyzer := newCtxAnalyzer() - analyzer.checkOuterAssignFlag = true - - analysistest.Run(t, testdata, analyzer.analyzer, "outervars") + analysistest.Run(t, testdata, Analyzer(), "outervars") } From 8202baf072cf8eb11679fe0e51dc596f50c39217 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 9 Jun 2026 05:43:07 +0000 Subject: [PATCH 3/3] fmt(pkg/paralleltestctx): drop stray blank lines --- pkg/paralleltestctx/analyzer.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/paralleltestctx/analyzer.go b/pkg/paralleltestctx/analyzer.go index 4120726..a64464d 100644 --- a/pkg/paralleltestctx/analyzer.go +++ b/pkg/paralleltestctx/analyzer.go @@ -306,8 +306,6 @@ func (a *ctxAnalyzer) analyzeTestFunction(pass *analysis.Pass, fd *ast.FuncDecl, // Collect all timeout contexts and their positions timeoutCtxs := a.collectTimeoutContexts(pass, fd, helpers) - - timeoutCtxObjs := make(map[types.Object]struct{}, len(timeoutCtxs)) for _, c := range timeoutCtxs { timeoutCtxObjs[c.obj] = struct{}{}