diff --git a/docs/adr/49066-stringsconcatloop-extend-to-x-equals-x-plus-y.md b/docs/adr/49066-stringsconcatloop-extend-to-x-equals-x-plus-y.md new file mode 100644 index 00000000000..621f6fc1c0e --- /dev/null +++ b/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.* diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 4194977f888..33576a41762 100644 --- a/pkg/linters/doc.go +++ b/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) diff --git a/pkg/linters/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/stringsconcatloop.go index 5c605484641..0a73117a00d 100644 --- a/pkg/linters/stringsconcatloop/stringsconcatloop.go +++ b/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,21 +97,34 @@ 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), @@ -87,10 +132,53 @@ func enclosingLoopPosition(pass *analysis.Pass, cur inspector.Cursor) (token.Pos ) { 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 } diff --git a/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go index 9d52b42550d..96b512f9159 100644 --- a/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go +++ b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go @@ -8,14 +8,14 @@ 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 @@ -23,9 +23,30 @@ func bad() { 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() {