stringsconcatloop: detect x = x + y accumulator pattern alongside +=

docs/adr/49066-stringsconcatloop-extend-to-x-equals-x-plus-y.md

@@ -0,0 +1,52 @@
+# ADR-49066: Extend stringsconcatloop to Detect `x = x + y` Accumulator Pattern
+
+**Date**: 2026-07-30
+**Status**: Draft
+**Deciders**: pelikhan (copilot-swe-agent)
+
+---
+
+### Context
+
+The `stringsconcatloop` linter existed to flag string `+=` inside for/range loops, which allocates a new string every iteration and creates O(n²) total-bytes cost for cross-iteration accumulators. However, the matcher only checked `token.ADD_ASSIGN` (`x += y`), leaving the semantically identical `x = x + y` (`token.ASSIGN` + `BinaryExpr{Op: ADD}`) undetected.
+
+This gap was being deliberately exploited: PR #49033 explicitly rewrote 17+ `x += y` sites to `x = x + y` to bypass the linter, and the PR description stated this as its intent. A production site (`pkg/workflow/schedule_preprocessing.go:447`) already exhibited the exact pattern. If left unaddressed, `x = x + y` would become a documented, permanent workaround that other contributors could copy.
+
+A naive "flag all `lhs = lhs + rhs` in loops" fix would cause false positives: when `lhs` is the loop's own iteration variable (e.g., `for _, line := range lines { line = line + suffix }`), the variable is reset each iteration and is not a cross-iteration accumulator — no O(n²) risk exists.
+
+### Decision
+
+We will extend `stringsconcatloop` to also match `token.ASSIGN` statements where `Lhs[0]` is an identifier whose name appears as the left operand of a `BinaryExpr{Op: ADD}` on the right-hand side (direct self-referential form only: `x = x + rhs`). A loop-scope guard (`isLoopScopedIdent`) will be added to exclude variables that are declared by the enclosing loop itself — range `Key`/`Value` identifiers and `ForStmt` `:=` init variables — so only genuine cross-iteration accumulators are flagged. The existing `enclosingLoopPosition` helper is refactored to `enclosingLoop`, returning the `ast.Node` needed by the guard.
+
+### Alternatives Considered
+
+#### Alternative 1: Do Nothing (Accept `x = x + y` as a Bypass)
+
+Accept that `x = x + y` is a valid pattern and leave the linter unchanged. Developers wishing to avoid the `strings.Builder` requirement could use this form.
+
+Not chosen because the bypass was explicitly documented and being actively promoted: PR #49033 demonstrated it as a deliberate strategy. Accepting it would permanently widen the enforcement gap and legitimize O(n²) string accumulation patterns.
+
+#### Alternative 2: Flag All `x = x + y` Inside Loops Without a Loop-Scope Guard
+
+Implement the `token.ASSIGN` + `BinaryExpr` check without adding `isLoopScopedIdent`, relying on the existing `FuncLit`-boundary stop alone.
+
+Not chosen because `pkg/workflow/schedule_preprocessing.go:447` demonstrates a real false positive: `for _, line := range lines { line = line + ... }` where `line` is the range iteration variable reset each pass. Without the guard, valid single-iteration rebinds would be flagged, producing noise and eroding linter trust.
+
+### Consequences
+
+#### Positive
+- Both `x += y` and `x = x + y` forms of cross-iteration string accumulation are now detected; the documented bypass vector is closed.
+- The loop-scope guard prevents false positives on per-iteration variables, preserving the linter's signal-to-noise ratio.
+- Testdata explicitly covers the new true-positive forms (range loop, classic for loop) and all false-positive guard cases (range value var, range key var, func literal boundary, non-self-referential `accum = p + accum`, out-of-loop form).
+
+#### Negative
+- The linter implementation is more complex: `enclosingLoop` now returns three values instead of two, and a new `isLoopScopedIdent` helper must be maintained.
+- The scope guard only handles the direct single-step form `x = x + rhs`; chained forms such as `x = x + a + b` remain undetected by design, creating a narrower but still extant bypass for multi-operand expressions.
+
+#### Neutral
+- The `enclosingLoopPosition` function is renamed to `enclosingLoop` and its signature changes; callers (currently only `run()`) must be updated.
+- `doc.go` and `Analyzer.Doc` descriptions are updated to reflect both detection forms, keeping documentation in sync with implementation.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/doc.go

@@ -49,7 +49,7 @@
 //   - strconvparseignorederror — flags strconv parsing calls where the error is discarded with _
 //   - stringbytesroundtrip — reports redundant string/[]byte round-trip conversions such as string([]byte(s)) or []byte(string(b)) that produce a wasteful intermediate copy
 //   - stringreplaceminusone — flags strings.Replace calls with n=-1 that should use strings.ReplaceAll
-//   - stringsconcatloop — flags string += concatenation inside for/range loops that should use strings.Builder
+//   - stringsconcatloop — flags string += or x = x + y concatenation inside for/range loops that should use strings.Builder
 //   - stringscountcontains — reports strings.Count(s, sub) comparisons with 0 or 1 (e.g. > 0, >= 1, == 0, != 0, < 1, <= 0) and their yoda-order variants that should use strings.Contains(s, sub) or !strings.Contains(s, sub)
 //   - stringsindexcontains — flags strings.Index(s, substr) comparisons that should use strings.Contains
 //   - stringsindexhasprefix — reports strings.Index(s, sub) comparisons with 0 (== 0 and != 0) and their yoda-order variants that should use strings.HasPrefix(s, sub) or !strings.HasPrefix(s, sub)

pkg/linters/stringsconcatloop/stringsconcatloop.go

@@ -1,7 +1,7 @@
 // Package stringsconcatloop implements a Go analysis linter that flags
-// string += concatenation inside for/range loop bodies, which allocates a new
-// string on every iteration and can lead to O(n²) total allocated bytes. The
-// idiomatic fix is to use strings.Builder.
+// string concatenation inside for/range loop bodies using += or the equivalent
+// x = x + y form, which allocates a new string on every iteration and can lead
+// to O(n²) total allocated bytes. The idiomatic fix is to use strings.Builder.
 package stringsconcatloop
 
 import (
@@ -20,7 +20,7 @@ import (
 // Analyzer is the string-concat-in-loop analysis pass.
 var Analyzer = &analysis.Analyzer{
 	Name:     "stringsconcatloop",
-	Doc:      "reports string += concatenation inside for/range loops that should use strings.Builder",
+	Doc:      "reports string concatenation (+=  or x = x + y) inside for/range loops that should use strings.Builder",
 	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsconcatloop",
 	Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer},
 	Run:      run,
@@ -45,7 +45,39 @@ func run(pass *analysis.Pass) (any, error) {
 		if !ok {
 			continue
 		}
-		if assign.Tok != token.ADD_ASSIGN {
+
+		// Match both `x += y` (ADD_ASSIGN) and `x = x + y` (ASSIGN with a
+		// self-referential binary addition on a plain identifier).
+		var lhsExpr ast.Expr
+
+		switch assign.Tok {
+		case token.ADD_ASSIGN:
+			if len(assign.Lhs) != 1 {
+				continue
+			}
+			lhsExpr = assign.Lhs[0]
+		case token.ASSIGN:
+			if len(assign.Lhs) != 1 || len(assign.Rhs) != 1 {
+				continue
+			}
+			lhsIdent, ok := assign.Lhs[0].(*ast.Ident)
+			if !ok {
+				continue
+			}
+			binExpr, ok := assign.Rhs[0].(*ast.BinaryExpr)
+			if !ok || binExpr.Op != token.ADD {
+				continue
+			}
+			// Only match the direct self-referential form: x = x + rhs,
+			// where x is the same identifier on both sides (not chained
+			// forms like x = x + a + b which parse with a BinaryExpr on
+			// the left of the outer add, not an Ident).
+			rhsLeft, ok := binExpr.X.(*ast.Ident)
+			if !ok || rhsLeft.Name != lhsIdent.Name {
+				continue
+			}
+			lhsExpr = lhsIdent
+		default:
 			continue
 		}
 
@@ -54,7 +86,7 @@ func run(pass *analysis.Pass) (any, error) {
 			continue
 		}
 
-		loopPos, inLoop := enclosingLoopPosition(pass, cur)
+		loopPos, loopNode, inLoop := enclosingLoop(pass, cur)
 		if !inLoop {
 			continue
 		}
@@ -65,32 +97,88 @@ func run(pass *analysis.Pass) (any, error) {
 			continue
 		}
 
-		if !astutil.IsStringType(pass, assign.Lhs[0]) {
+		if !astutil.IsStringType(pass, lhsExpr) {
 			continue
 		}
 
+		// Skip variables that are per-iteration rather than cross-iteration
+		// accumulators. These checks apply only when the LHS is a plain
+		// identifier (the ADD_ASSIGN form also accepts field/index lvalues,
+		// but those can only be tested via the ASSIGN form which requires Ident).
+		if lhsIdent, ok := lhsExpr.(*ast.Ident); ok {
+			if isLoopScopedIdent(loopNode, lhsIdent.Name) {
+				continue
+			}
+			if isLoopBodyLocal(pass, loopNode, lhsIdent) {
+				continue
+			}
+		}
+
 		pass.ReportRangef(assign,
-			"string concatenation with += inside a loop allocates O(n) strings and O(n²) total bytes; use strings.Builder instead")
+			"string concatenation inside a loop allocates O(n) strings and O(n²) total bytes; use strings.Builder instead")
 	}
 
 	return nil, nil
 }
 
-// enclosingLoopPosition returns the nearest enclosing for/range statement
-// position for cur (an AssignStmt), without crossing a function literal
-// boundary. Assignments inside func literals are intentionally exempt.
-func enclosingLoopPosition(pass *analysis.Pass, cur inspector.Cursor) (token.Position, bool) {
+// enclosingLoop returns the nearest enclosing for/range statement, its source
+// position, and true for cur (an AssignStmt), without crossing a function
+// literal boundary. Assignments inside func literals are intentionally exempt.
+func enclosingLoop(pass *analysis.Pass, cur inspector.Cursor) (token.Position, ast.Node, bool) {
 	for encl := range cur.Enclosing(
 		(*ast.ForStmt)(nil),
 		(*ast.RangeStmt)(nil),
 		(*ast.FuncLit)(nil),
 	) {
 		switch encl.Node().(type) {
 		case *ast.ForStmt, *ast.RangeStmt:
-			return pass.Fset.PositionFor(encl.Node().Pos(), false), true
+			return pass.Fset.PositionFor(encl.Node().Pos(), false), encl.Node(), true
 		case *ast.FuncLit:
-			return token.Position{}, false
+			return token.Position{}, nil, false
 		}
 	}
-	return token.Position{}, false
+	return token.Position{}, nil, false
+}
+
+// isLoopScopedIdent reports whether name is declared by loopNode as a loop
+// variable: the Key or Value identifier of a RangeStmt. Such variables are
+// per-iteration rebinds, not cross-iteration accumulators.
+//
+// Note: ForStmt init variables (e.g. for s := ""; ...) are intentionally NOT
+// exempted — the init clause runs only once, so the variable carries state
+// across all iterations and is a genuine accumulator.
+func isLoopScopedIdent(loopNode ast.Node, name string) bool {
+	n, ok := loopNode.(*ast.RangeStmt)
+	if !ok {
+		return false
+	}
+	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
+	}
+	return false
+}
+
+// isLoopBodyLocal reports whether ident is declared inside the loop body
+// (rather than before the loop). Such variables are freshly created on every
+// iteration and are therefore not cross-iteration accumulators.
+func isLoopBodyLocal(pass *analysis.Pass, loopNode ast.Node, ident *ast.Ident) bool {
+	obj := pass.TypesInfo.ObjectOf(ident)
+	if obj == nil {
+		return false
+	}
+	var body *ast.BlockStmt
+	switch n := loopNode.(type) {
+	case *ast.ForStmt:
+		body = n.Body
+	case *ast.RangeStmt:
+		body = n.Body
+	}
+	if body == nil {
+		return false
+	}
+	pos := obj.Pos()
+	return pos >= body.Lbrace && pos < body.Rbrace
 }

pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go

@@ -8,24 +8,45 @@ func bad() {
 	// Basic range loop – should be flagged.
 	result := ""
 	for _, p := range parts {
-		result += p // want `string concatenation with \+= inside a loop`
+		result += p // want `string concatenation inside a loop`
 	}
 	_ = result
 
 	// Classic for loop – should be flagged.
 	s := ""
 	for i := 0; i < len(parts); i++ {
-		s += parts[i] // want `string concatenation with \+= inside a loop`
+		s += parts[i] // want `string concatenation inside a loop`
 	}
 	_ = s
 
 	// Named string type – should also be flagged.
 	type myString string
 	var ms myString
 	for _, p := range parts {
-		ms += myString(p) // want `string concatenation with \+= inside a loop`
+		ms += myString(p) // want `string concatenation inside a loop`
 	}
 	_ = ms
+
+	// x = x + y form in a range loop – should be flagged.
+	accum := ""
+	for _, p := range parts {
+		accum = accum + p // want `string concatenation inside a loop`
+	}
+	_ = accum
+
+	// x = x + y form in a classic for loop – should be flagged.
+	s2 := ""
+	for i := 0; i < len(parts); i++ {
+		s2 = s2 + parts[i] // want `string concatenation inside a loop`
+	}
+	_ = s2
+
+	// for-init string accumulator – the init clause runs once, so the variable
+	// carries state across all iterations and is a genuine accumulator.
+	for s3 := ""; len(s3) < 10; {
+		s3 = s3 + "x" // want `string concatenation inside a loop`
+		_ = s3
+	}
 }
 
 func good() {
@@ -59,6 +80,66 @@ func good() {
 		}()
 	}
 	_ = acc
+
+	// x = x + y outside any loop – not flagged.
+	outside := "prefix"
+	outside = outside + "suffix"
+	_ = outside
+
+	// x = y + x (left operand is not the LHS) – not flagged.
+	accum2 := ""
+	for _, p := range parts {
+		accum2 = p + accum2
+	}
+	_ = accum2
+
+	// Range value variable reassigned per iteration – not a cross-iteration
+	// accumulator, so not flagged.
+	for _, line := range parts {
+		line = line + " suffix"
+		_ = line
+	}
+
+	// Range key variable over a string-keyed map – not a cross-iteration
+	// accumulator, so not flagged.
+	m := map[string]int{"a": 1, "b": 2}
+	for k := range m {
+		k = k + "_x"
+		_ = k
+	}
+
+	// x = x + y inside a func literal inside a loop – not flagged. The linter
+	// intentionally stops at func literal boundaries.
+	accum3 := ""
+	for _, p := range parts {
+		func() {
+			accum3 = accum3 + p
+		}()
+	}
+	_ = accum3
+
+	// x = x + a + b (chained addition) – binExpr.X is itself a BinaryExpr,
+	// not an Ident, so the self-referential check fails and it is not flagged.
+	accum4 := ""
+	for _, p := range parts {
+		accum4 = accum4 + p + " extra"
+	}
+	_ = accum4
+
+	// Variable declared inside the loop body is a per-iteration local, not a
+	// cross-iteration accumulator – not flagged.
+	for _, p := range parts {
+		var local string
+		local = local + p
+		_ = local
+	}
+
+	// Range value variable reassigned via += – not a cross-iteration
+	// accumulator, so not flagged.
+	for _, line := range parts {
+		line += " suffix"
+		_ = line
+	}
 }
 
 func nolintDirective() {