[linter-miner] Add loop-index-address-taken linter - #63475
github-actions[bot] wants to merge 2 commits into
Conversation
This linter detects a common concurrency bug where loop index variables
are captured by address in goroutines or deferred functions without
proper value capture. This is dangerous because the loop variable may
be reassigned before the goroutine/deferred function executes.
For example:
for i := range items {
go func() {
fmt.Println(i) // BUG: i may have changed
}()
}
Should be:
for i := range items {
go func(i int) {
fmt.Println(i) // OK: captured by value
}(i)
}
The linter correctly handles:
- Parameters captured by value (no error)
- Variables without address-of operator (no error)
- Value variables in nested scopes (no error)
- nolint directives (respects suppressions)
- Generated files (skipped)
Tests verify positive cases (bugs flagged) and negative cases
(correct patterns not flagged).
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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The analyzer reports safe Go 1.22+ loops, misses several genuine address escapes, and breaks the registry documentation test.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds a custom Go analyzer intended to detect loop-index addresses escaping through goroutines or deferred calls.
Changes:
- Implements and registers
loopindexaddresstaken. - Adds analysistest coverage and fixtures.
| File | Description |
|---|---|
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/loopindexaddresstaken/loopindexaddresstaken.go |
Implements detection logic. |
pkg/linters/loopindexaddresstaken/loopindexaddresstaken_test.go |
Runs analyzer fixtures. |
pkg/linters/loopindexaddresstaken/testdata/src/loopindexaddresstaken/loopindexaddresstaken.go |
Defines positive and negative cases. |
| ioutildeprecated.Analyzer, | ||
| httpstatuscode.Analyzer, | ||
| largefunc.Analyzer, | ||
| loopindexaddresstaken.Analyzer, |
| // It does NOT flag: | ||
| // - Capturing the loop variable by value: go func(i int) { ... }(i) | ||
| // - Just using the variable (not its address): go func() { ... fmt.Println(i) ... }() | ||
| // - Breaking out of the loop after starting the goroutine (single iteration) |
🏗️ ADR Required — draft added for PR #63475I found no existing ADR in the PR body or current Evidence reviewed
Inferred architectural decisionThis PR introduces a repository-wide static-analysis rule to enforce a specific concurrency-safety policy: detect taking the address of Action takenI added a draft ADR to the PR branch:
Next action for the authorPlease review and refine the draft ADR, especially the scope boundaries and trade-offs, and keep it with the PR as the design record for this linter addition.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
This linter is not safe to merge yet: it encodes pre-Go-1.22 loop-variable semantics, matches identifiers by name instead of object identity, and skips nested closures that can still capture the same loop variable.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45 AIC · ⌖ 7.09 AIC · ⊞ 20.3K
Comment /review to run again
|
|
||
| ast.Inspect(body, func(n ast.Node) bool { | ||
| // Skip nested function literals — their scope is separate | ||
| if _, ok := n.(*ast.FuncLit); ok && n != body { |
There was a problem hiding this comment.
Skipping every nested function literal here hides real captures of the outer loop variable instead of creating a new safe scope.
💡 Why this blocks the linter
A nested closure still closes over the outer i unless it shadows it, so this becomes a false negative:
var i int
for i = range items {
go func() {
inner := func() { fmt.Println(&i) }
inner()
}()
}The new fixture currently labels that pattern as GoodNestedFuncLit, which bakes the false negative into the test suite. Please keep descending and use object identity to decide whether the inner &i refers to the loop variable.
|
|
||
| // Check if the operand is the loop variable | ||
| operandIdent, ok := unary.X.(*ast.Ident) | ||
| if !ok || operandIdent.Name != loopVar.Name { |
There was a problem hiding this comment.
Comparing identifiers by name here will misreport any shadowed local i inside the closure as if it were the range variable.
💡 Why this blocks the linter
This check should use object identity, not Ident.Name, because the safe code below is a different variable but currently matches:
var i int
for i = range items {
go func() {
i := 0
fmt.Println(&i)
}()
}Other analyzers in this tree already use pass.TypesInfo to compare referenced objects; this one should do the same both here and in hasLoopVarAsParam.
| } | ||
|
|
||
| // Get the loop variables | ||
| indexVar := getLoopVar(rangeStmt.Key) |
There was a problem hiding this comment.
This rule is going to flag correct Go 1.22+ code because it treats for i := range loop vars as reused across iterations even though this module targets Go 1.26.
💡 Why this blocks the linter
Since Go 1.22, range variables introduced with := are fresh per iteration, so the new fixtures are not bugs anymore. The still-broken form is the predeclared-variable case:
var i int
for i = range items {
go func() { fmt.Println(&i) }()
}Please gate this analyzer on the loop form or language version instead of all RangeStmts, and add fixtures for the = case.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs and /tdd — requesting changes primarily because the core premise of the linter appears invalidated by this repository's own Go version.
📋 Key Themes & Highlights
Key Themes
- Premise mismatch (blocking):
go.modpinsgo 1.26.8. Since Go 1.22,for-range loop variables are scoped per-iteration, so the exact address-of-loop-var-in-goroutine/defer pattern this linter flags is no longer a data race in this codebase (or any module targeting Go 1.22+). I reproduced this locally: the flagged pattern produced correct per-iteration values, not the classic "all goroutines see last value" bug. Worth confirming whether the linter should be scoped to pre-1.22 modules, reframed as a style nit, or dropped. - Formatting:
gofmt -lfails on bothpkg/linters/registry.go(import not alphabetically sorted) andloopindexaddresstaken.go(trailing whitespace) —make fmtneeds to be run before merge.
Positive Highlights
- ✅ Clean, well-documented package with a clear doc comment explaining intent and false-positive avoidance.
- ✅ Good test fixture coverage of positive/negative cases (value-capture params, nested func literals, classic
forloops correctly excluded). - ✅ Correct handling of nolint directives and generated-file skipping via shared
analyzerutil/nolint/filecheckhelpers, consistent with sibling linters.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 59.3 AIC · ⌖ 14.8 AIC · ⊞ 10.1K
Comment /matt to run again
| // - Just using the variable (not its address): go func() { ... fmt.Println(i) ... }() | ||
| // - Breaking out of the loop after starting the goroutine (single iteration) | ||
| package loopindexaddresstaken | ||
|
|
There was a problem hiding this comment.
[/grill-with-docs] Since this repo targets go 1.26.8 (go.mod), Go 1.22+ per-iteration loop variable semantics mean for i := range items { go func(){ &i }() } no longer shares an address across iterations — I verified this by running the exact pattern (each goroutine got a distinct value, not the last-iteration value). The bug this linter targets doesn't actually occur in code built with this module's Go version.
💡 Reproduction
// go.mod: go 1.22 (or later)
for i := range items {
go func() {
results[i] = *(&i) // each goroutine sees its own i, NOT the buggy shared value
}()
}Running this prints values in a permutation of [0 1 2], not repeated last-index values — confirming there's no bug to catch here for code on Go 1.22+.
Consider either: (a) scoping the analyzer to only fire when the analyzed module's go directive is < 1.22, or (b) reframing the docs/rationale to note this is now purely a style/clarity nit (taking &i is unusual but not a data race under 1.22+ semantics), or (c) dropping the linter if it no longer detects a real bug in this codebase.
@copilot please address this.
| "github.com/github/gh-aw/pkg/linters/ioutildeprecated" | ||
| "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" | ||
| "github.com/github/gh-aw/pkg/linters/largefunc" | ||
| "github.com/github/gh-aw/pkg/linters/loopindexaddresstaken" |
There was a problem hiding this comment.
[/tdd] gofmt/goimports fails on this line — the new import isn't alphabetically sorted (loopindexaddresstaken is placed before lenstringsplit/lenstringzero, breaking the existing l* ordering). Run make fmt (per repo conventions) before merging; CI formatting checks will likely fail otherwise.
💡 Verified locally
$ gofmt -l pkg/linters/registry.go
pkg/linters/registry.go
Correct order should be: largefunc, lenstringsplit, lenstringzero, logfatallibrary, loopindexaddresstaken, manualmutexunlock, ... (same fix needed for the allAnalyzers slice entry).
@copilot please address this.
|
|
||
| // Get the loop variables | ||
| indexVar := getLoopVar(rangeStmt.Key) | ||
|
|
There was a problem hiding this comment.
[/tdd] Trailing whitespace on this blank line fails gofmt -l (confirmed locally). Run make fmt to clean this up alongside the import-order fix in registry.go.
@copilot please address this.
There was a problem hiding this comment.
Impeccable review — loop-index-address-taken linter
Applied harden (edge-case/false-positive robustness) and audit (code quality) review modes, since this PR is a bug-detection tool where false positives directly undermine its value.
| # | Issue | Severity |
|---|---|---|
| 1 | Matches loop var by identifier name only, not by object identity/scope — produces a false positive when the name is shadowed inside the goroutine/defer body (verified locally: appended a ShadowedFalsePositive test case and reproduced the incorrect diagnostic) |
Blocking |
| 2 | New import/analyzer entries in registry.go are not alphabetically ordered (gofmt -d confirms) |
Non-blocking |
Recommend using pass.TypesInfo.Uses/ObjectOf to compare identifier objects rather than string names for issue #1, since that's the root cause of the false positive.
Verification steps
go test ./pkg/linters/loopindexaddresstaken/... # passes as-is
# appended a shadowed-variable case to testdata, reran -> reproduced false positive
gofmt -d pkg/linters/registry.go # confirms import ordering diff
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 71.5 AIC · ⌖ 13.4 AIC · ⊞ 8.1K
|
|
||
| // Check if the operand is the loop variable | ||
| operandIdent, ok := unary.X.(*ast.Ident) | ||
| if !ok || operandIdent.Name != loopVar.Name { |
There was a problem hiding this comment.
False positive: matches by identifier name, not by declaration/scope.
operandIdent.Name != loopVar.Name only compares the identifier's textual name. If the loop variable's name is shadowed by a local redeclaration inside the goroutine/defer body (e.g. i := 42 inside the closure), &i there refers to the local variable, not the loop variable — but the linter still flags it.
Reproduced locally by appending this case to the testdata fixture and running go test ./pkg/linters/loopindexaddresstaken/...:
func ShadowedFalsePositive(items []string) {
for i, v := range items {
fmt.Println(i)
go func() {
i := 42
fmt.Println(&i, v) // false positive: this `i` is the local, not the loop var
}()
}
}This produces unexpected diagnostic: taking the address of loop variable i in a goroutine... even though the code is correct.
Suggest resolving identity via pass.TypesInfo.Uses[operandIdent]/ObjectOf (or checking the *ast.Object/scope) and comparing against the loop variable's actual types.Object/*ast.Object, not just the name string, to avoid false positives on shadowed variables.
@copilot please address this.
| "github.com/github/gh-aw/pkg/linters/ioutildeprecated" | ||
| "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" | ||
| "github.com/github/gh-aw/pkg/linters/largefunc" | ||
| "github.com/github/gh-aw/pkg/linters/loopindexaddresstaken" |
There was a problem hiding this comment.
Import not alphabetically ordered — breaks gofmt/goimports grouping convention used throughout this file.
loopindexaddresstaken (starts with loop...) is inserted before lenstringsplit/lenstringzero (len...), which sort earlier alphabetically. gofmt -d on this file confirms:
- "github.com/github/gh-aw/pkg/linters/loopindexaddresstaken"
"github.com/github/gh-aw/pkg/linters/lenstringsplit"
"github.com/github/gh-aw/pkg/linters/lenstringzero"
"github.com/github/gh-aw/pkg/linters/logfatallibrary"
+ "github.com/github/gh-aw/pkg/linters/loopindexaddresstaken"
Move the import to after logfatallibrary (before manualmutexunlock) to restore alphabetical order. The corresponding entry in the allAnalyzers slice below (line ~120) has the same ordering issue — it's placed before logfatallibrary.Analyzer instead of after.
@copilot please address this.
|
@copilot Please address the outstanding review feedback and continue with the
|
Summary
This PR introduces a new linter that detects a common concurrency bug where loop index variables are captured by address in goroutines or deferred functions without proper value capture.
Problem
Loop variables in for-range loops are reused across iterations. When the address of a loop variable is used in a goroutine or deferred function, all captures share the same address, causing the captured value to change unexpectedly. This is a subtle but dangerous bug.
Example of the bug:
Correct pattern:
Implementation Details
pkg/linters/loopindexaddresstaken/loopindexaddresstaken&) applied to loop index variablesWhat the linter detects
go func() { &i }defer func() { &i }What the linter correctly ignores
go func(i int) { ... }(i)go func() { fmt.Println(i) }Testing
go test ./pkg/linters/loopindexaddresstaken/...pkg/linters/registry.goEvidence from mining
This linter addresses a pattern that recurs in real-world Go code and is difficult to catch in code review. It provides high signal-to-noise (few false positives) and is not covered by standard golangci-lint rules.