feat(linters): add stringsconcatloop — detect string += concatenation inside loops #47894
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # ADR-47894: Add stringsconcatloop Linter to Detect O(n²) String Concatenation in Loops | ||
|
|
||
| **Date**: 2026-07-25 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| Go's `string` type is immutable. Every `string +=` inside a loop allocates a brand-new string and copies the previous content, yielding O(n²) time and memory as the loop count grows. The idiomatic fix — `strings.Builder` — avoids re-allocation by accumulating bytes and materialising the final string once. The `gh-aw` repository maintains a collection of custom `go/analysis` analyzers to enforce code-quality conventions at lint time; adding a dedicated analyzer for this pattern ensures the issue is caught before code merges rather than discovered in production profiling. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will add a new `stringsconcatloop` analyzer under `pkg/linters/stringsconcatloop/` that walks `*ast.AssignStmt` nodes with `token.ADD_ASSIGN`, verifies the LHS has a `string` underlying type via the type-checker, and reports a diagnostic when the assignment is enclosed within a `for` or `range` loop body — stopping at `func` literal boundaries to avoid false positives on closures. The analyzer honours `//nolint:stringsconcatloop` directives and skips generated files via the existing `filecheck` and `nolint` infrastructure already used by other linters in the collection. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Rely on Runtime Profiling and Benchmarks | ||
|
|
||
| Profile-guided discovery (pprof, benchmarks) would catch hot O(n²) concat paths only after the code is merged and exercised. This defers detection to a later, more expensive phase of the development cycle and misses cold-path code that is rarely profiled. Static analysis at lint time prevents the pattern from entering the codebase at all. | ||
|
|
||
| #### Alternative 2: Adopt a Third-Party Linter (gocritic / staticcheck) | ||
|
|
||
| `gocritic` (rule `appendAssign`) and `staticcheck` include heuristics that overlap with this pattern, but neither covers the full set of cases the repository cares about (e.g., named string types, cursor-based AST traversal for accurate loop ancestry). Importing a third-party tool would also add a binary dependency and version-management burden, whereas a custom analyzer integrates cleanly with the existing `go/analysis` harness, `nolint` index, and `filecheck` generated-file detection already shared across all 58 analyzers in this collection. | ||
|
|
||
| #### Alternative 3: Extend an Existing String-Manipulation Linter | ||
|
|
||
| Folding this check into `stringbytesroundtrip` or another existing `string*` analyzer was considered but rejected: each analyzer in the collection has a single, clearly scoped responsibility, and conflating unrelated patterns makes the diagnostic messages ambiguous and the test surface harder to reason about. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - String O(n²) concat patterns are caught at `golangci-lint` / CI run time, before merging. | ||
| - The implementation reuses the shared `filecheck`, `nolint`, and `astutil.IsStringType` infrastructure, keeping the new analyzer consistent with the rest of the linter collection. | ||
| - Named-string-type aliases (e.g., `type myString string`) are also flagged, closing a gap that a purely syntax-level check would miss. | ||
|
|
||
| #### Negative | ||
| - Short loops (two or three iterations) incur no meaningful allocation overhead; the analyzer will flag these and require either a `//nolint` suppression or a `strings.Builder` rewrite that is arguably less readable in that context. | ||
| - Developers unfamiliar with the linter collection must learn one more rule and its suppression mechanism. | ||
|
|
||
| #### Neutral | ||
| - The linter count increments from 57 to 58; `doc.go`, `registry.go`, `spec_test.go`, and `README.md` all receive corresponding mechanical updates. | ||
| - The `//nolint:stringsconcatloop` escape hatch is available for the rare case where `+=` is intentional and the allocation cost is acceptable. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| // 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. | ||
| package stringsconcatloop | ||
|
|
||
| import ( | ||
| "go/ast" | ||
| "go/token" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/astutil" | ||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| "github.com/github/gh-aw/pkg/linters/internal/nolint" | ||
| ) | ||
|
|
||
| // 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", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsconcatloop", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| root, err := astutil.Root(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| noLintIndex, err := nolint.Index(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| generatedFiles, err := filecheck.Index(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for cur := range root.Preorder((*ast.AssignStmt)(nil)) { | ||
| assign, ok := cur.Node().(*ast.AssignStmt) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if assign.Tok != token.ADD_ASSIGN { | ||
| continue | ||
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(assign.Pos(), false) | ||
| if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The 💡 DetailIn Consider adding an explicit assertion (e.g., an @copilot please address this. |
||
| continue | ||
| } | ||
|
|
||
| loopPos, inLoop := enclosingLoopPosition(pass, cur) | ||
| if !inLoop { | ||
| continue | ||
| } | ||
| if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsconcatloop") { | ||
| continue | ||
|
Contributor
There was a problem hiding this comment.
💡 DetailThe 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 Fix options: (a) look up the nolint on the |
||
| } | ||
| if nolint.HasDirectiveForLinter(loopPos, noLintIndex, "stringsconcatloop") { | ||
| continue | ||
| } | ||
|
|
||
| if !astutil.IsStringType(pass, assign.Lhs[0]) { | ||
| continue | ||
|
Contributor
There was a problem hiding this comment. IIFE closure false negative: 💡 DetailThe 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 At minimum, the |
||
| } | ||
|
|
||
| pass.ReportRangef(assign, | ||
| "string concatenation with += 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) { | ||
| 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 | ||
| case *ast.FuncLit: | ||
| return token.Position{}, false | ||
| } | ||
| } | ||
| return token.Position{}, false | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package stringsconcatloop_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/stringsconcatloop" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, stringsconcatloop.Analyzer, "stringsconcatloop") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package stringsconcatloop | ||
|
|
||
| import "strings" | ||
|
|
||
| func bad() { | ||
| parts := []string{"a", "b", "c"} | ||
|
|
||
| // Basic range loop – should be flagged. | ||
| result := "" | ||
| for _, p := range parts { | ||
| result += p // want `string concatenation with \+= 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 | ||
|
|
||
| // 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 | ||
| } | ||
|
|
||
| func good() { | ||
| parts := []string{"a", "b", "c"} | ||
|
|
||
| // Using strings.Builder – not flagged. | ||
| var sb strings.Builder | ||
| for _, p := range parts { | ||
| sb.WriteString(p) | ||
| } | ||
| _ = sb.String() | ||
|
|
||
| // += outside any loop – not flagged. | ||
| result := "prefix" | ||
| result += "suffix" | ||
| _ = result | ||
|
|
||
| // Integer += inside a loop – not flagged. | ||
| n := 0 | ||
| for i := range parts { | ||
| n += i | ||
| } | ||
| _ = n | ||
|
|
||
| // String += inside a func literal inside a loop – not flagged. The linter | ||
| // intentionally stops at func literal boundaries. | ||
| acc := "" | ||
| for _, p := range parts { | ||
| func() { | ||
| acc += p | ||
|
Contributor
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. 💡 SuggestionAdd a comment in the testdata and/or the @copilot please address this. |
||
| }() | ||
| } | ||
| _ = acc | ||
| } | ||
|
|
||
| func nolintDirective() { | ||
| parts := []string{"a", "b", "c"} | ||
|
|
||
| result := "" | ||
| for _, p := range parts { //nolint:stringsconcatloop | ||
| result += p | ||
| } | ||
| _ = result | ||
|
|
||
| result2 := "" | ||
| for _, p := range parts { //nolint:stringsconcatloop | ||
| _ = strings.TrimSpace(p) | ||
| result2 += p | ||
| } | ||
| _ = result2 | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
len(assign.Lhs) == 0is a dead guard —token.ADD_ASSIGNalways 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.