stringsconcatloop: ADD_ASSIGN-only matcher lets contributors bypass the lint rule with `x = x + y` — already happening in open P [Content truncated due to length]

[github-actions]

Summary

stringsconcatloop (pkg/linters/stringsconcatloop/stringsconcatloop.go:48) only flags token.ADD_ASSIGN (x += y). It does not match the semantically identical token.ASSIGN form (x = x + y), even though both allocate a new string per iteration and carry the same O(n2) total-bytes risk when x is a cross-iteration accumulator.

This is not just a theoretical gap: open PR #49033 (fixing lint-monster report #49029) explicitly rewrites 17+ single-conditional += sites to x = x + y specifically to dodge this linter — the PR description says so directly: "replaced x += y with x = x + y — avoids the linter flag without adding Builder overhead." If merged as-is, every one of those sites becomes permanently invisible to stringsconcatloop, and the pattern is now a documented workaround other contributors can copy.

Evidence

• Matcher: pkg/linters/stringsconcatloop/stringsconcatloop.go:48 — if assign.Tok != token.ADD_ASSIGN { continue }, with no companion check for token.ASSIGN + *ast.BinaryExpr{Op: token.ADD}.
• PR fix(lint): replace string += in loops with strings.Builder; map[string]bool set → struct{} #49033 body, "single-conditional appends" section: converts if f.Value == currentFreq { label += " (current)" } to label = label + " (current)" across checkout_config_parser.go, audit_report_render.go, bootstrap_config.go, claude_logs.go, copilot_engine_execution.go, dependabot.go, expression_nodes.go, strict_mode_permissions_validation.go, threat_detection_external.go, add_interactive_schedule.go, audit_cross_run_render.go, compile_stats.go, experiments_analyze_statistics.go, fetch.go, imports.go, outcomes_command.go, view_command.go (18 files changed per PR stats, still open/unmerged as of this run).
• A prod site already matching this exact syntactic shape exists today: pkg/workflow/schedule_preprocessing.go:447 — line = line + " # Friendly format: " + friendly inside for _, line := range lines { ... }.

Why a naive fix needs a scope guard

The schedule_preprocessing.go:447 site above shows the naive fix ("flag any lhs = lhs + rhs inside a loop") would misfire: line is the RangeStmt's own iteration variable — it is reset to a fresh value at the top of every iteration, so the concatenation is a single one-shot append within that iteration, not a cross-iteration accumulator. It is not an O(n2) risk despite matching the syntactic shape.

The correct discriminator: only flag lhs = lhs + rhs when lhs is not the Key/Value identifier of the enclosing RangeStmt (or the Init variable of the enclosing ForStmt) — i.e., when the accumulator is declared outside the loop and therefore genuinely carries state across iterations. This mirrors the existing enclosingLoopPosition FuncLit-boundary walk already in this file (stringsconcatloop.go:82-96); it just needs one more exclusion branch for loop-scoped declarations.

Recommendation

1. In run(), also match *ast.AssignStmt with Tok == token.ASSIGN, Lhs[0] an *ast.Ident, and Rhs[0] a *ast.BinaryExpr{Op: token.ADD} whose left operand's identifier matches Lhs[0].
2. Add the loop-scoped-declaration exclusion described above so for _, line := range lines { line = line + x }-style single-iteration rebinds are not flagged.
3. Add a golden testdata case for both the new true-positive (sb := ""; for { sb = sb + x }) and the false-positive guard (the range-variable-rebind case), following the existing deferinloop/ctxbackground FuncLit-boundary-test convention.
4. Flag this to whoever reviews fix(lint): replace string += in loops with strings.Builder; map[string]bool set → struct{} #49033 before merge — the PR's own stated intent ("avoids the linter flag") should be treated as a signal to fix the linter, not accept the workaround as the destination pattern.

Validation checklist

• stringsconcatloop matches token.ASSIGN + self-referential BinaryExpr addition
• Range/for loop-scoped rebind (non-accumulator) case verified NOT flagged
• New golden testdata added under pkg/linters/stringsconcatloop/testdata/
• pkg/linters/doc.go / README / spec_test descriptions updated if matcher scope description changes
• Re-scan pkg/workflow/schedule_preprocessing.go:447 confirms no false positive
• Re-scan the 18 files touched by PR fix(lint): replace string += in loops with strings.Builder; map[string]bool set → struct{} #49033 once merged to confirm no missed true accumulators

Effort: small — single-file matcher extension + testdata, no autofix/SuggestedFix involved (this linter has none).

Generated by 🤖 Sergo - Serena Go Expert · age00 · 230.9 AIC · ⌖ 6.33 AIC · ⊞ 6K · ◷

• expires on Aug 5, 2026, 8:57 PM UTC-08:00