stringsconcatloop: detect x = x + y accumulator pattern alongside += - #49066
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
x = x + y accumulator pattern alongside +=
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. PR changes only the linter implementation (stringsconcatloop.go), documentation, and test fixture data (testdata/src/.../stringsconcatloop.go). The existing stringsconcatloop_test.go file remains unchanged. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
There was a problem hiding this comment.
🟡 Not ready to approve
The scope guard misses valid classic-for accumulators, and the new diagnostic inaccurately reports += syntax.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Extends stringsconcatloop to detect direct x = x + y string accumulators.
Changes:
- Adds assignment-form detection and loop-scope filtering.
- Adds positive and negative analyzer test cases.
- Updates linter documentation.
File summaries
| File | Description |
|---|---|
pkg/linters/stringsconcatloop/stringsconcatloop.go |
Implements detection and scope handling. |
pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go |
Adds analyzer test cases. |
pkg/linters/doc.go |
Updates the linter summary. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // x = x + y form in a range loop – should be flagged. | ||
| accum := "" | ||
| for _, p := range parts { | ||
| accum = accum + p // want `string concatenation with \+= inside a loop` |
There was a problem hiding this comment.
Fixed in commit 4c74e3e. The diagnostic message is now syntax-neutral: "string concatenation inside a loop allocates O(n) strings and O(n²) total bytes; use strings.Builder instead". All // want patterns in testdata updated accordingly.
| // isLoopScopedIdent reports whether name is declared by loopNode as a loop | ||
| // variable: the Key or Value identifier of a RangeStmt, or a variable in the | ||
| // short-declaration Init of a ForStmt. Such variables are per-iteration | ||
| // rebinds, not cross-iteration accumulators. | ||
| func isLoopScopedIdent(loopNode ast.Node, name string) bool { | ||
| switch n := loopNode.(type) { | ||
| case *ast.RangeStmt: | ||
| if id, ok := n.Key.(*ast.Ident); ok && id.Name == name { | ||
| return true | ||
| } | ||
| if id, ok := n.Value.(*ast.Ident); ok && id.Name == name { | ||
| return true | ||
| } | ||
| case *ast.ForStmt: | ||
| init, ok := n.Init.(*ast.AssignStmt) | ||
| if !ok || init.Tok != token.DEFINE { | ||
| return false | ||
| } | ||
| for _, lhs := range init.Lhs { | ||
| if id, ok := lhs.(*ast.Ident); ok && id.Name == name { | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in commit 4c74e3e. Removed the ForStmt branch from isLoopScopedIdent — for-init variables run once before the loop and carry state across all iterations, so they are genuine accumulators and must be flagged. Only RangeStmt Key/Value remain exempted. Added a regression test case for for s3 := ""; ... that verifies it is now flagged.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is clean and correct. The x = x + y accumulator detection is well-guarded:
- Requires exact self-referential form (
x = x + rhs), notx = rhs + x— intentionally conservative. isLoopScopedIdentcorrectly handles nilKey/Value/Initvia type assertions.- Test cases cover all relevant branches (range, for, func-literal boundary, loop-scoped vars, outside-loop, reversed operand).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.6 AIC · ⌖ 4.75 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a correctness gap and two missing test boundaries.
📋 Key Themes & Highlights
Key Issues
- Correctness gap (line 105): The
isLoopScopedIdentguard is only applied to theASSIGNform. A range-value or for-init variable using+=(e.g.line += suffix) would be falsely flagged. The two forms need consistent treatment. - Missing test (line 75): The out-of-scope chained form
x = x + a + bis called out in a comment but not covered by a testdata case. The boundary is implicitly correct today but unverified. - Asymmetric defensive guard (line 56): The
len(assign.Lhs) != 1check exists only forASSIGN, notADD_ASSIGN. Keeping them symmetric makes future audits easier.
Positive Highlights
- ✅ Clean refactor of
enclosingLoopPosition→enclosingLoop— returning the AST node is the right design for the new guard. - ✅
isLoopScopedIdentis a well-scoped, readable helper with a clear contract. - ✅ Comprehensive false-positive cases in testdata: reversed operands, func-literal boundary, out-of-loop form, range key/value vars.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.3 AIC · ⌖ 5.18 AIC · ⊞ 7K
Comment /matt to run again
| // For the x = x + y form, skip variables declared by the enclosing | ||
| // loop itself (range key/value or for-init short-decl): those are | ||
| // per-iteration rebinds, not cross-iteration accumulators. | ||
| if assignLhsName != "" && isLoopScopedIdent(loopNode, assignLhsName) { |
There was a problem hiding this comment.
[/tdd] The loop-scope guard is only applied to the ASSIGN form (x = x + y), not to ADD_ASSIGN (x += y). A range-value or for-init variable using += would be falsely flagged, creating a behavioral asymmetry between the two forms.
💡 Example false positive (currently unflagged by tests)
for _, line := range parts {
line += " suffix" // would be flagged — but line is a per-iteration rebind
}The fix: capture the LHS name for ADD_ASSIGN too and apply isLoopScopedIdent uniformly, or extract a shared helper. A test case should be added to testdata to prevent regression.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 89abd77. The scope guard (isLoopScopedIdent + isLoopBodyLocal) is now applied uniformly whenever the LHS is a plain *ast.Ident, regardless of whether the token is ADD_ASSIGN or ASSIGN. A testdata case for line += " suffix" on a range-value variable confirms it is no longer flagged.
| var assignLhsName string // non-empty only for the token.ASSIGN form | ||
|
|
||
| switch assign.Tok { | ||
| case token.ADD_ASSIGN: |
There was a problem hiding this comment.
[/diagnosing-bugs] For ADD_ASSIGN, assign.Lhs[0] is used directly without checking that len(assign.Lhs) == 1. A multi-LHS assignment like a, b += ... is syntactically invalid in Go, but the guard on line 61–63 (applied only to ASSIGN) makes the intent asymmetric and harder to reason about.
💡 Suggestion
Add a len(assign.Lhs) == 1 guard for the ADD_ASSIGN case as well, matching the defensive style used for ASSIGN. This is belt-and-suspenders given Go syntax, but keeps both branches symmetric and easy to audit.
case token.ADD_ASSIGN:
if len(assign.Lhs) != 1 {
continue
}
lhsExpr = assign.Lhs[0]@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 89abd77. Added len(assign.Lhs) != 1 guard to the ADD_ASSIGN case, making it symmetric with the ASSIGN branch.
| rhsLeft, ok := binExpr.X.(*ast.Ident) | ||
| if !ok || rhsLeft.Name != lhsIdent.Name { | ||
| continue | ||
| } |
There was a problem hiding this comment.
[/tdd] The comment says "direct self-referential form only — x = x + a + b is out of scope", but this is not tested. The x = x + a + b case parses as BinaryExpr{X: BinaryExpr{X: x, Y: a}, Y: b}, so binExpr.X would be a *ast.BinaryExpr, not *ast.Ident — meaning it would silently fall through. A testdata case would document and lock this boundary.
💡 Suggested test case
// x = x + a + b (chained) – out of scope, not flagged.
for _, p := range parts {
accum = accum + p + " extra" // should NOT be flagged
}This prevents a future contributor from unexpectedly expanding the scope without realising it.
@copilot please address this.
There was a problem hiding this comment.
Added a testdata case in commit 89abd77: accum4 = accum4 + p + " extra" (chained form) is present in good() with no // want annotation, which the test framework enforces must produce no diagnostic. The comment explains why: binExpr.X is itself a BinaryExpr, not an *ast.Ident, so the self-referential check fails silently.
There was a problem hiding this comment.
Verdict: Request Changes — a real false-positive gap and an asymmetric detection bypass need addressing before merge.
Themes
isLoopScopedIdentdoesn't account for accumulator-shaped variables declared withvarinside the loop body itself, so per-iteration locals can be misflagged as cross-iteration accumulators (medium, no test coverage for this case despite the PR otherwise being thorough about false-positive guards).- The new
x = x + ymatcher only supports simple identifier LHS, while the existing+=matcher supports any lvalue (struct fields, map/slice indices). This reintroduces exactly the kind of detection bypass this PR set out to close, just for a narrower set of forms. - Core refactor (
enclosingLoopPosition→enclosingLoop, newisLoopScopedIdenthelper) is otherwise clean and well-tested for the range/for key/value/init cases it does handle.
🔎 Code quality review by PR Code Quality Reviewer · auto · 42.4 AIC · ⌖ 4.75 AIC · ⊞ 7.8K
Comment /review to run again
| // For the x = x + y form, skip variables declared by the enclosing | ||
| // loop itself (range key/value or for-init short-decl): those are | ||
| // per-iteration rebinds, not cross-iteration accumulators. | ||
| if assignLhsName != "" && isLoopScopedIdent(loopNode, assignLhsName) { |
There was a problem hiding this comment.
False positive: a fresh var accum string declared inside the loop body each iteration is misclassified as a cross-iteration accumulator and gets flagged.
💡 Loop-body-local var decls are not recognized as per-iteration rebinds
isLoopScopedIdent only checks RangeStmt.Key/Value and the ForStmt short-decl Init. It does not look at var declarations inside the loop body itself. Example that would be (incorrectly) flagged:
for _, p := range parts {
var accum string
accum = accum + p
_ = accum
}Here accum is declared fresh every iteration — it is not a cross-iteration accumulator, yet the current logic reports it as one because the declaration site is inside the loop body, not the RangeStmt/ForStmt node itself.
Fix: walk the loop body's top-level statement list (or use pass.TypesInfo.Defs/scope lookup) to check whether the identifier's declaring *ast.GenDecl/ValueSpec or := statement is inside the loop body, and treat that as loop-scoped too.
No test case in the diff covers this scenario, despite the PR explicitly adding several other false-positive guard tests.
There was a problem hiding this comment.
Fixed in commit 89abd77. Added isLoopBodyLocal which uses pass.TypesInfo.ObjectOf(ident).Pos() to check whether the variable's declaration falls inside the loop body (body.Lbrace .. body.Rbrace). A testdata case var local string; local = local + p inside a range loop confirms it is not flagged.
| switch assign.Tok { | ||
| case token.ADD_ASSIGN: | ||
| lhsExpr = assign.Lhs[0] | ||
| case token.ASSIGN: |
There was a problem hiding this comment.
The new x = x + y detection only supports a simple identifier LHS, unlike += which accepts any lvalue — an inconsistency that silently drops coverage for common accumulator patterns.
💡 Asymmetric LHS handling between ADD_ASSIGN and ASSIGN paths
For token.ADD_ASSIGN, lhsExpr = assign.Lhs[0] accepts any expression — s.Field += y, m[k] += y, arr[i] += y are all flagged. For the new token.ASSIGN branch, assign.Lhs[0].(*ast.Ident) requires a plain identifier, so the equivalent s.Field = s.Field + y or m[k] = m[k] + y forms are silently skipped.
This means the PR's stated goal (detecting the semantically identical x = x + y form) is only partially achieved — struct-field and map/slice-index accumulators, just as susceptible to the O(n2) allocation problem, remain an undetected bypass, exactly the kind of gap this PR is meant to close.
Suggested fix: generalize the self-referential check to compare binExpr.X against assign.Lhs[0] structurally rather than restricting to *ast.Ident.
There was a problem hiding this comment.
The ASSIGN path intentionally restricts to plain identifiers. Generalising to struct-field and map/slice-index lvalues (s.Field = s.Field + y, m[k] = m[k] + y) requires structural AST comparison and is explicitly out of scope for this PR (noted in the PR description as "direct self-referential form only"). The ADD_ASSIGN path already catches field/index accumulators via the existing += form; this gap is narrow in practice. A follow-up issue can track the generalisation if desired.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Triage Summary
Linter enhancement (stringsconcatloop accumulator pattern). PR is in blocked (merge-conflict) state — needs rebase.
|
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
…emption - Make diagnostic message syntax-neutral: remove '+=', now reads 'string concatenation inside a loop ...' - Remove ForStmt init-var exemption from isLoopScopedIdent: for-init vars run once and carry state across iterations (genuine accumulators), so they must be flagged. Only RangeStmt Key/Value remain exempted. - Update all testdata // want patterns to match new message - Add regression test: for-init string accumulator is flagged Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Apply isLoopScopedIdent + isLoopBodyLocal to ADD_ASSIGN form too, not just ASSIGN; range-value vars using += are now correctly exempted - Add len(assign.Lhs) != 1 guard to ADD_ASSIGN case (symmetric with ASSIGN) - Add isLoopBodyLocal helper: uses TypesInfo.ObjectOf to detect vars declared inside the loop body (per-iteration locals, not accumulators) - Testdata: add chained x = x + a + b case (not flagged), loop-body-local var case (not flagged), and ADD_ASSIGN range-value guard case Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review threads addressed:
Local |
|
🎉 This pull request is included in a new release. Release: |
The linter only matched
token.ADD_ASSIGN(x += y), leaving the semantically identicalx = x + yform undetected — a gap that was being intentionally exploited as a documented workaround.Changes
New pattern match:
run()now also handlestoken.ASSIGNwhereRhs[0]is a*ast.BinaryExpr{Op: token.ADD}whose left operand is the same identifier asLhs[0](direct self-referential form only —x = x + a + bis out of scope).Loop-scope guard: For the
ASSIGNform, variables declared by the enclosing loop itself are excluded — range key/value vars and for-init short-decl vars are per-iteration rebinds, not cross-iteration accumulators. This avoids false positives like:enclosingLoopPosition→enclosingLoop: refactored to also return theast.NodesoisLoopScopedIdentcan inspect the loop's key/value/init declarations.isLoopScopedIdent: new helper that checks whether a name is declared by aRangeStmt(Key/Value) orForStmt(:=Init).Testdata: added true-positive cases for both ASSIGN forms (range loop and classic
for), plus false-positive guard cases (range value var, range key var overmap[string]int, func-literal boundary, non-self-referentialaccum = p + accum, out-of-loop form).doc.go/Analyzer.Doc: updated descriptions to reflect both+=andx = x + y.x = x + y— already happening in open P [Content truncated due to length] #49046Run: https://github.com/github/gh-aw/actions/runs/30523835792