Repository navigation
[linter-miner] Add loop-var-mutation-in-closure linter - #64056
github-actions[bot] wants to merge 3 commits into
Conversation
Reports loop variables captured in closures (goroutines, anonymous functions) without proper scoping, which can cause unintended mutations and data races. The linter detects patterns like: - Loop variables used in goroutines without shadowing or parameter passing - Loop variables used in deferred functions without proper scoping - Both for loops and range loops Safe patterns are recognized: - Variable shadowing: x := x - Parameter passing: func(x) - Local variable declarations that shadow the loop variable This linter found 15+ instances in the codebase that should be fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass found a few opportunities to simplify the analyzer implementation without changing behavior. Focus is on deleting incidental complexity (string token checks, nested control flow, repeated scans, and optional debug plumbing).
net: -34 lines possible.
Generated by ✂️ Ponytail Reviewer for #64056 · codex · gpt53codex · 6.08 AIC · ⌖ 3.28 AIC · ⊞ 12.5K
Comment /ponytail to run again
| func extractForLoopVarDefs(pass *analysis.Pass, stmt *ast.ForStmt) map[string]types.Object { | ||
| vars := make(map[string]types.Object) | ||
|
|
||
| if stmt.Init == nil { |
There was a problem hiding this comment.
L77: stdlib: assign.Tok.String() == ":=". assign.Tok == token.DEFINE, no string conversion.
| return shadowed | ||
| } | ||
|
|
||
| // Look for assignments that shadow loop variables: x := x or x := ... |
There was a problem hiding this comment.
L185: stdlib: stmt.Tok.String() != ":=". stmt.Tok != token.DEFINE, same check with fewer moving parts.
|
|
||
| // Look for assignments that shadow loop variables: x := x or x := ... | ||
| ast.Inspect(funcLit.Body, func(node ast.Node) bool { | ||
| switch stmt := node.(type) { |
There was a problem hiding this comment.
L187-211: shrink: deep nested if chain for x := x detection. Use guard-style continue and a small helper to cut indentation and branches.
| paramNames, shadowedVars map[string]struct{}) { | ||
|
|
||
| ast.Inspect(funcBody, func(node ast.Node) bool { | ||
| ident, ok := node.(*ast.Ident) |
There was a problem hiding this comment.
L237-273: shrink: O(vars) scan on every identifier. Build map[types.Object]string once and do one lookup per ident.
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var pkgLog = logger.New("linters:loopvarmutationinclosure") |
There was a problem hiding this comment.
L20: yagni: package-level logger used for one debug line. Drop logger wiring and rely on analyzer diagnostics only.
🏗️ ADR required — draft added for PR #64056I enforced the design-decision gate for this PR because it adds 536 lines in default business-logic directories ( Evidence reviewed
OutcomeI added a draft ADR at Inferred decision
Next actionPlease review and refine the ADR content, then keep it with the PR as the architectural record for this linter addition.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on a correctness-blocking false-positive issue plus a shadow-ordering false negative.
📋 Key Themes & Highlights
Key Themes
- Blocking: no Go-version gating. The bug this linter targets (loop variable capture in closures) was fixed by Go 1.22's per-iteration loop variable semantics. The stdlib's own
golang.org/x/tools/go/analysis/passes/loopclosureexplicitly skips files using Go 1.22+ (versions.Go1_22). This new linter has no such check. I ran it standalone againstpkg/cli/...in this repo (which declaresgo 1.26.8) and it reported 14 diagnostics — every one a false positive, since none of those closures can actually race under current Go semantics. This needs to be fixed before the linter is usable. - Shadow-ordering false negative. The safety check for
x := xshadowing doesn't consider source position — a use of the loop variable before the shadow assignment in the same closure is incorrectly treated as safe. Verified with a new test case thatanalysistestreports "no diagnostic was reported" for this pattern.
Positive Highlights
- ✅ Clean integration with existing linter infrastructure (
analyzerutil,astutil,filecheck,nolint) and registry pattern, consistent withlargefunc/deferinloop. - ✅ Good test coverage of the intended patterns (shadowing, parameter passing, nested loops, multi-var capture) — the gaps found are about scope (Go version) and edge-case ordering, not test hygiene generally.
See inline comments for details and a suggested regression test.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 65.7 AIC · ⌖ 14.7 AIC · ⊞ 10K
Comment /matt to run again
| func run(pass *analysis.Pass) (any, error) { | ||
| insp, err := astutil.Inspector(pass) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
[/tdd] Critical false-positive risk: this linter targets the pre-Go-1.22 loop variable capture bug, but does not check the analyzed file's Go language version. Since Go 1.22, loop variables are scoped per-iteration, so this exact pattern is no longer a bug in files that opt into go 1.22+ semantics — which includes this entire repo (go.mod declares go 1.26.8).
💡 Evidence & suggested fix
The stdlib's own equivalent analyzer explicitly guards against this:
// golang.org/x/tools/go/analysis/passes/loopclosure/loopclosure.go
cur.Filter(func(n ast.Node) bool {
// Only traverse the file if its goversion is strictly before go1.22.
return !analyzerutil.FileUsesGoVersion(pass, n, versions.Go1_22)
})Running this new linter standalone against pkg/cli/... in this very repo produces 14 diagnostics (e.g. audit_cache.go:78, fix_command.go:328, workflows.go:318) — all of which are false positives under Go 1.22+ semantics, since the loop variable is already per-iteration scoped and no shadow/param-passing is required.
Add a Go-version check (via pass.Pkg.GoVersion / golang.org/x/tools/internal/versions, or an equivalent helper in analyzerutil) and skip files/packages using Go 1.22+ before reporting. Without this, the linter will be effectively unusable on any modern Go codebase, including this one.
@copilot please address this.
| shadowedVars := extractFuncShadowedVars(pass, funcLit, loopVarDefs) | ||
|
|
||
| // Walk the function body to find uses of loop variables | ||
| if funcLit.Body != nil { |
There was a problem hiding this comment.
[/tdd] extractFuncShadowedVars/walkFuncBodyForLoopVarUses treat shadowing as file-position-independent: a use before the shadow assignment in source order (e.g. fmt.Println(i); i := i) is silently treated as safe even though it still races on the un-shadowed value. The test fixtures don't cover this ordering case.
💡 Suggested test
func BadUseBeforeShadow() {
for i := 0; i < 10; i++ {
go func() {
fmt.Println(i) // want `loop variable i captured in closure`
i := i
_ = i
}()
}
}I added this exact case locally and confirmed it currently produces no diagnostic (false negative) — analysistest fails with "no diagnostic was reported". Consider comparing ident.Pos() against the shadow assignment's position, or use per-use dataflow instead of a package-wide "is this var ever shadowed anywhere in the func" set.
@copilot please address this.
There was a problem hiding this comment.
Summary
This PR adds a new loopvarmutationinclosure linter, but its core detection logic does not account for Go 1.22+ per-iteration loop variable semantics. This repo's go.mod requires go 1.26.8, meaning loop variables are already scoped per-iteration and the closures flagged as "bad" (e.g., directly capturing i in a goroutine without i := i or parameter-passing) are safe, idiomatic code — not bugs.
golang.org/x/tools' own loopclosure analyzer explicitly skips files targeting Go 1.22+ (versions.Go1_22 / FileUsesGoVersion) for exactly this reason; this new analyzer has no equivalent check anywhere in run/checkLoopNode/checkFuncLitForCaptures.
Left one blocking inline comment detailing the issue, the verification steps taken, and the required fix (gate on file Go version like x/tools/loopclosure does).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 119.7 AIC · ⌖ 14.2 AIC · ⊞ 8.1K
| } | ||
|
|
||
| pkgLog.Printf("analyzing package %s", pass.Pkg.Path()) | ||
|
|
There was a problem hiding this comment.
This linter's entire premise is invalid for this codebase. Since Go 1.22, for and range loop variables are per-iteration by default (see golang.org/x/tools' own loopclosure analyzer, which explicitly skips files with go1.22+ via versions.Go1_22/FileUsesGoVersion). This repo's go.mod declares go 1.26.8.
I verified this locally:
for i := 0; i < 10; i++ {
go func() {
mu.Lock()
seen[i] = true // captures i directly, NO shadow/param-pass
mu.Unlock()
}()
}
// -> seen contains all values 0..9, no data race, no bugThis is exactly the "bad" pattern this linter reports (e.g. walkFuncBodyForLoopVarUses, line ~273-277), but it's actually correct, idiomatic Go 1.22+ code. Nothing in run, checkLoopNode, or checkFuncLitForCaptures inspects the Go language version of the file/module (no reference to x/tools/internal/versions or an equivalent check anywhere in this file).
As written, this analyzer will generate a large volume of false positives across the entire codebase (which the PR description itself confirms: "Found 15+ actual instances... requiring attention" — none of which are actual bugs under Go 1.22+ semantics) and will actively push contributors toward unnecessary, noisy shadowing/parameter-passing that adds no safety benefit.
Required fix: gate reporting on the file's Go language version the same way golang.org/x/tools/go/analysis/passes/loopclosure does — skip files whose effective Go version is >= 1.22 (or invert: only run for files below 1.22), using analyzerutil/x/tools/internal/versions-equivalent version detection. Without this, the linter should not be merged/enabled.
@copilot please address this.
Overview
This PR introduces a new custom Go linter called
loop-var-mutation-in-closurethat detects and flags a common Go pitfall: loop variables captured in closures without proper scoping.Problem This Linter Solves
When loop variables are captured directly in closures (goroutines, anonymous functions), they can be mutated before the closure executes, leading to unexpected behavior and data races.
Example of the problem:
Correct solutions:
Implementation Details
github.com/github/gh-aw/pkg/linters/loopvarmutationinclosurepkg/linters/loopvarmutationinclosure/loopvarmutationinclosure.go- Main analyzerpkg/linters/loopvarmutationinclosure/loopvarmutationinclosure_test.go- Testspkg/linters/loopvarmutationinclosure/testdata/src/loopvarmutationinclosure/loopvarmutationinclosure.go- Test fixturespkg/linters/registry.go- Registered the new analyzerDetection
The linter correctly identifies:
forandrangeloop variablesThe linter safely ignores:
Test Results
Sample findings from
pkg/cli:audit_cache.go:78-processedRuncaptured in closurefix_command.go:328-331-codemodcaptured in multiple placesworkflows.go:318-331-filecaptured in several goroutineslogs_multi.go:372-targetIDandtargetcapturedEvidence
This linter was derived from:
The pattern was identified as a high-signal, low-false-positive issue that:
Compliance
pkg/linters/largefunc/)(nolint/redacted):loopvarmutationinclosuredirectives