feat(linters): add stringsconcatloop — detect string += concatenation inside loops - #47894
Conversation
…loops Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds the stringsconcatloop analyzer to detect inefficient string concatenation inside loops.
Changes:
- Implements and registers the analyzer.
- Adds analysistest coverage.
- Updates linter documentation and analyzer counts.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringsconcatloop/stringsconcatloop.go |
Implements loop-concatenation detection. |
pkg/linters/stringsconcatloop/stringsconcatloop_test.go |
Runs analyzer tests. |
pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go |
Provides positive and negative fixtures. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds documentation synchronization metadata. |
pkg/linters/README.md |
Documents the analyzer. |
pkg/linters/doc.go |
Updates package documentation and count. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/linters/stringsconcatloop/stringsconcatloop.go:89
- A
ForStmtalso encloses itsInitstatement, which runs only once. For valid code such asfor result += prefix; cond; { ... }, this branch reports quadratic loop concatenation even though the assignment is not repeated. Skip the current assignment when it is the loop'sInit(while continuing to inspect any outer loop); thePoststatement should remain reportable because it runs per iteration.
switch encl.Node().(type) {
case *ast.ForStmt, *ast.RangeStmt:
return true
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Medium
| pass.ReportRangef(assign, | ||
| "string concatenation with += inside a loop causes O(n²) allocations; use strings.Builder instead") |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
ADR for the decision to add a static analysis linter that detects O(n²) string += concatenation inside for/range loops.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (192 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Analysis Summarystringsconcatloop_test.go uses the canonical
spec_test.go modifications maintain the contract:
Test inflation: 16 test lines / 95 production lines = 0.17 (well under 2:1 threshold) Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two issues need fixing before merge.
### Findings
[high] nolint directive placement is broken for multi-statement loop bodies (line 60): The suppression check uses pos.Line-1 to match a directive on the enclosing for line, but pos is derived from assign.Pos() — the += line itself. Any loop body with statements before the += will silently ignore the //nolint:stringsconcatloop directive. Testdata only covers the degenerate one-statement case.
[medium] IIFE false negative (line 68): isInsideLoop stops at any *ast.FuncLit, including immediately-invoked closures. An IIFE called on every loop iteration allocates identically to direct +=, but is never flagged. The testdata comment "not flagged (new scope)" is misleading — the scope argument does not apply to performance. This should be documented as a known limitation at minimum.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 44.2 AIC · ⌖ 4.89 AIC · ⊞ 5.7K
Comment /review to run again
| continue | ||
| } | ||
| if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsconcatloop") { | ||
| continue |
There was a problem hiding this comment.
//nolint on the for line silently fails for multi-statement loop bodies: the directive is matched by checking pos.Line and pos.Line-1, but pos comes from assign.Pos() — the += line. If the += is not on the immediately following line, the nolint directive is ignored and the diagnostic fires.
💡 Detail
The testdata exercises only the single-statement body case:
for _, p := range parts { (nolint/redacted):stringsconcatloop
result += p // line N+1 — pos.Line-1 matches ✓
}But silently breaks when there are preceding statements:
for _, p := range parts { (nolint/redacted):stringsconcatloop
doSomething() // line N+1
result += p // line N+2 — pos.Line-1 = N+1, no match ✗ → diagnostic still fires
}Users placing //nolint:stringsconcatloop on the for line with any preceding statement in the body will find suppression ineffective with no diagnostic to explain why.
Fix options: (a) look up the nolint on the += line itself rather than the enclosing for line, (b) expand the lookup to cover the entire loop body range, or (c) document explicitly that the directive must be on the += assignment line. Add a testdata case covering a multi-statement body to catch regressions.
| } | ||
|
|
||
| if !isInsideLoop(cur) { | ||
| continue |
There was a problem hiding this comment.
IIFE closure false negative: acc += p inside an immediately-invoked func(){}() is still O(n2) but is never flagged: isInsideLoop returns false on the first *ast.FuncLit ancestor, treating all func literals as exempt. IIFEs are called inline on every loop iteration and share the same performance characteristic as direct +=.
💡 Detail
The testdata comment says "not flagged (new scope)" but the scope claim only applies to variable declarations — the performance problem is identical:
acc := ""
for _, p := range parts {
func() {
acc += p // still O(n2): called on every iteration, allocates a new string each time
}()
}The FuncLit exemption is intentional for non-IIFE cases (e.g. goroutines, callbacks passed elsewhere), but applying it unconditionally means the most trivially refactorable IIFE pattern is silently skipped.
At minimum, the Doc string and/or README should acknowledge this limitation so users aren't misled. Alternatively, detect IIFEs by checking whether the FuncLit's parent node is a CallExpr and the callee is the FuncLit itself — in that case, continue traversal instead of stopping.
There was a problem hiding this comment.
Review: stringsconcatloop linter
The implementation is well-structured and follows established patterns in the codebase. The cursor-based Enclosing traversal correctly handles the func-literal boundary exemption, nolint directive handling matches existing linters, and testdata coverage is thorough.
One existing comment (line 72 of the main Go file) flags an inaccuracy in the diagnostic message: O(n2) allocations conflates number-of-allocations (O(n)) with total bytes allocated (O(n2)). That wording should be corrected to avoid misleading consumers of the diagnostic.
No other blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34.9 AIC · ⌖ 4.52 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness concerns and one clarity gap.
📋 Key Themes & Highlights
Key Themes
- Nolint placement vs. report site mismatch — the suppression comment is on the
forline but the diagnostic fires on the inner+=; this may silently not suppress. - Dead guard —
len(assign.Lhs) == 0is unreachable fortoken.ADD_ASSIGN; remove to avoid misleading readers. - Undocumented design decision — func-literal exemption is a real false-negative trade-off that should be explained in code.
Positive Highlights
- ✅ Solid cursor-based AST traversal using the
inspector.CursorAPI consistently with the rest of the codebase - ✅ Excellent testdata coverage: range, classic for, named types, integers, Builder, nolint, func literals — all present
- ✅ Correct scope-aware
isInsideLoopthat stops at function literal boundaries - ✅ Full registration + documentation update with accurate count (57→58)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.2 AIC · ⌖ 4.84 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(assign.Pos(), false) | ||
| if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { |
There was a problem hiding this comment.
[/tdd] The nolint directive in the testdata is on the for line, but the diagnostic fires on the += assignment — these are different line numbers. Confirm nolint.HasDirectiveForLinter checks the reported node's line, not the loop header; if it only checks the loop line the suppression silently fails.
💡 Detail
In testdata/.../stringsconcatloop.go the //nolint:stringsconcatloop comment is on the for _, p := range parts { line, while pass.ReportRangef(assign, ...) fires on result += p which is on the next line. If the nolint lookup is line-exact, the suppression is broken.
Consider adding an explicit assertion (e.g., an analysistest-style // no want comment, or a separate sub-test that verifies the diagnostic count is zero) to catch this.
@copilot please address this.
| for cur := range root.Preorder((*ast.AssignStmt)(nil)) { | ||
| assign, ok := cur.Node().(*ast.AssignStmt) | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
[/codebase-design] len(assign.Lhs) == 0 is a dead guard — token.ADD_ASSIGN always has exactly one LHS operand and the Go parser will not produce a zero-length LHS for a compound-assignment. Remove it to avoid misleading future readers into thinking this case is reachable.
@copilot please address this.
| acc := "" | ||
| for _, p := range parts { | ||
| func() { | ||
| acc += p |
There was a problem hiding this comment.
[/tdd] The func-literal exemption is a silent false negative that deserves a code comment or doc note explaining the trade-off. acc += p inside the goroutine/closure does cause O(n2) allocations — it's just not flagged because the implementation stops at function literal boundaries. A reader of the testdata or the linter output might wonder why it's skipped.
💡 Suggestion
Add a comment in the testdata and/or the isInsideLoop function explaining why func-literal-enclosed concatenations are intentionally out of scope (e.g., the assignment may run zero or more times depending on call site, making the loop relationship non-obvious).
@copilot please address this.
|
@copilot please run the skill, confirm the branch is up to date, address unresolved review feedback, and rerun or retrigger checks as needed on the latest HEAD. Unresolved review feedback:
Run: https://github.com/github/gh-aw/actions/runs/30143146258
|
…ncatloop-linter Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…suppression Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed on latest HEAD
I reviewed prior run |
|
🎉 This pull request is included in a new release. Release: |
Adds a new
stringsconcatloopGo analysis linter that flagsstring +=insidefor/rangeloop bodies. Each iteration allocates a full string copy, yielding O(n²) memory; the idiomatic fix isstrings.Builder.Implementation
pkg/linters/stringsconcatloop/— analyzer walks*ast.AssignStmtnodes via cursor-based inspector, checkstoken.ADD_ASSIGNwithstringunderlying type on LHS, then verifies enclosure in afor/rangeloop without crossing afuncliteral boundary. Skips generated files; honours//nolint:stringsconcatloop.pkg/linters/registry.go— registered inAll()pkg/linters/doc.go,README.md,spec_test.go— doc surfaces updated (count 57→58)Run: https://github.com/github/gh-aw/actions/runs/30143146258