From ef69bd04c24655871e6911b9c2417f86a54ac71a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:20:45 +0000 Subject: [PATCH 1/4] Initial plan From 20852098b421247b2b32ea9333818c92a4aa78c0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:33:50 +0000 Subject: [PATCH 2/4] feat(linters): add stringsconcatloop analyzer to detect string += in loops Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/README.md | 2 + pkg/linters/doc.go | 3 +- pkg/linters/registry.go | 2 + pkg/linters/spec_test.go | 6 +- .../stringsconcatloop/stringsconcatloop.go | 95 +++++++++++++++++++ .../stringsconcatloop_test.go | 16 ++++ .../stringsconcatloop/stringsconcatloop.go | 71 ++++++++++++++ 7 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 pkg/linters/stringsconcatloop/stringsconcatloop.go create mode 100644 pkg/linters/stringsconcatloop/stringsconcatloop_test.go create mode 100644 pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 399e83c5aef..469046fbdac 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -52,6 +52,7 @@ This package currently provides custom Go analyzers in the following subpackages - `strconvparseignorederror` — reports `strconv` parsing calls (`Atoi`, `ParseInt`, etc.) where the error return is discarded with `_`. - `stringbytesroundtrip` — reports redundant `string([]byte(s))` or `[]byte(string(b))` round-trip conversions that produce a wasteful intermediate copy. - `stringreplaceminusone` — reports `strings.Replace` calls whose `n` argument is `-1`, which should use the more readable `strings.ReplaceAll`. +- `stringsconcatloop` — reports `string +=` concatenation inside `for`/`range` loop bodies, which allocates a new string copy on every iteration (O(n²) memory); use `strings.Builder` instead. - `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)` instead. - `stringsindexcontains` — reports `strings.Index(s, substr)` comparisons with `-1` or `0` (e.g. `!= -1`, `>= 0`, `> -1`, `== -1`, `< 0`, `<= -1`) and their yoda-order variants that should use `strings.Contains(s, substr)` or `!strings.Contains(s, substr)` instead. - `stringsjoinone` — reports `strings.Join([]string{s}, sep)` calls with a single-element slice literal where the separator is never used and the call is equivalent to just `s`. @@ -117,6 +118,7 @@ This package currently provides custom Go analyzers in the following subpackages | `strconvparseignorederror` | Custom `go/analysis` analyzer that flags `strconv` parsing calls where the error return is discarded with `_` | | `stringbytesroundtrip` | Custom `go/analysis` analyzer that flags redundant `string([]byte(s))` or `[]byte(string(b))` round-trip conversions that produce a wasteful intermediate copy | | `stringreplaceminusone` | Custom `go/analysis` analyzer that flags `strings.Replace` calls with `n=-1` that should use `strings.ReplaceAll` | +| `stringsconcatloop` | Custom `go/analysis` analyzer that flags `string +=` concatenation inside `for`/`range` loops that should use `strings.Builder` | | `stringscountcontains` | Custom `go/analysis` analyzer that flags `strings.Count(s, sub)` comparisons with `0` or `1` that should use `strings.Contains` or `!strings.Contains` | | `stringsindexcontains` | Custom `go/analysis` analyzer that flags `strings.Index(s, substr)` comparisons with `-1` or `0` that should use `strings.Contains` or `!strings.Contains` | | `stringsjoinone` | Custom `go/analysis` analyzer that flags `strings.Join([]string{s}, sep)` calls with a single-element slice literal where the separator is unused and the call is equivalent to just `s` | diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index e2282310622..742cb71c361 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 57 active analyzers: +// All 58 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -48,6 +48,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 // - 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 // - stringsjoinone — flags strings.Join([]string{s}, sep) calls with a single-element slice literal where the separator is unused and the call is equivalent to just s diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index b4013b0b207..3485022bac8 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -49,6 +49,7 @@ import ( "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/stringbytesroundtrip" "github.com/github/gh-aw/pkg/linters/stringreplaceminusone" + "github.com/github/gh-aw/pkg/linters/stringsconcatloop" "github.com/github/gh-aw/pkg/linters/stringscountcontains" "github.com/github/gh-aw/pkg/linters/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/stringsjoinone" @@ -112,6 +113,7 @@ func All() []*analysis.Analyzer { strconvparseignorederror.Analyzer, stringbytesroundtrip.Analyzer, stringreplaceminusone.Analyzer, + stringsconcatloop.Analyzer, stringsindexcontains.Analyzer, stringsjoinone.Analyzer, stringscountcontains.Analyzer, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 969d9ea23d6..0afe1d41cdb 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -57,6 +57,7 @@ import ( "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" "github.com/github/gh-aw/pkg/linters/stringbytesroundtrip" "github.com/github/gh-aw/pkg/linters/stringreplaceminusone" + "github.com/github/gh-aw/pkg/linters/stringsconcatloop" "github.com/github/gh-aw/pkg/linters/stringscountcontains" "github.com/github/gh-aw/pkg/linters/stringsindexcontains" "github.com/github/gh-aw/pkg/linters/stringsjoinone" @@ -83,7 +84,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 57 analyzers +// "Public API > Subpackages" table. The README documents 58 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -94,7 +95,7 @@ type docAnalyzer struct { // hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, -// strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringscountcontains, stringsindexcontains, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, +// strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, // tolowerequalfold, trimleftright, uncheckedtypeassertion, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ @@ -144,6 +145,7 @@ func documentedAnalyzers() []docAnalyzer { {"strconvparseignorederror", strconvparseignorederror.Analyzer}, {"stringbytesroundtrip", stringbytesroundtrip.Analyzer}, {"stringreplaceminusone", stringreplaceminusone.Analyzer}, + {"stringsconcatloop", stringsconcatloop.Analyzer}, {"stringscountcontains", stringscountcontains.Analyzer}, {"stringsindexcontains", stringsindexcontains.Analyzer}, {"stringsjoinone", stringsjoinone.Analyzer}, diff --git a/pkg/linters/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/stringsconcatloop.go new file mode 100644 index 00000000000..0abaa020770 --- /dev/null +++ b/pkg/linters/stringsconcatloop/stringsconcatloop.go @@ -0,0 +1,95 @@ +// Package stringsconcatloop implements a Go analysis linter that flags +// string += concatenation inside for/range loop bodies, which allocates a +// new string copy on every iteration (O(n²) memory). 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 + } + if len(assign.Lhs) == 0 { + continue + } + + pos := pass.Fset.PositionFor(assign.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + continue + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsconcatloop") { + continue + } + + if !astutil.IsStringType(pass, assign.Lhs[0]) { + continue + } + + if !isInsideLoop(cur) { + continue + } + + pass.ReportRangef(assign, + "string concatenation with += inside a loop causes O(n²) allocations; use strings.Builder instead") + } + + return nil, nil +} + +// isInsideLoop reports whether cur (an AssignStmt) is enclosed within a +// for or range loop body, without crossing a function literal boundary. +// Assignments inside func literals are exempt because they form a new scope. +func isInsideLoop(cur inspector.Cursor) 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 true + case *ast.FuncLit: + return false + } + } + return false +} diff --git a/pkg/linters/stringsconcatloop/stringsconcatloop_test.go b/pkg/linters/stringsconcatloop/stringsconcatloop_test.go new file mode 100644 index 00000000000..3ed531916e7 --- /dev/null +++ b/pkg/linters/stringsconcatloop/stringsconcatloop_test.go @@ -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") +} diff --git a/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go new file mode 100644 index 00000000000..e16465f060c --- /dev/null +++ b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go @@ -0,0 +1,71 @@ +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 (new scope). + acc := "" + for _, p := range parts { + func() { + acc += p + }() + } + _ = acc +} + +func nolintDirective() { + parts := []string{"a", "b", "c"} + + result := "" + for _, p := range parts { //nolint:stringsconcatloop + result += p + } + _ = result +} From c6b0a513f482bdafc2f94616f4febefb02eca133 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:53:41 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-47894 for stringsconcatloop linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR for the decision to add a static analysis linter that detects O(n²) string += concatenation inside for/range loops. --- .../adr/47894-add-stringsconcatloop-linter.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 docs/adr/47894-add-stringsconcatloop-linter.md diff --git a/docs/adr/47894-add-stringsconcatloop-linter.md b/docs/adr/47894-add-stringsconcatloop-linter.md new file mode 100644 index 00000000000..749455b7d91 --- /dev/null +++ b/docs/adr/47894-add-stringsconcatloop-linter.md @@ -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.* From 484345480ec161c50f3fface4edc28f0fadd19af Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 04:20:20 +0000 Subject: [PATCH 4/4] fix(stringsconcatloop): clarify cost wording and improve nolint loop suppression Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../stringsconcatloop/stringsconcatloop.go | 35 ++++++++++--------- .../stringsconcatloop/stringsconcatloop.go | 10 +++++- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/pkg/linters/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/stringsconcatloop.go index 0abaa020770..5c605484641 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 copy on every iteration (O(n²) memory). The idiomatic fix is -// to use strings.Builder. +// 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 ( @@ -48,37 +48,38 @@ func run(pass *analysis.Pass) (any, error) { if assign.Tok != token.ADD_ASSIGN { continue } - if len(assign.Lhs) == 0 { - continue - } pos := pass.Fset.PositionFor(assign.Pos(), false) if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { continue } + + loopPos, inLoop := enclosingLoopPosition(pass, cur) + if !inLoop { + continue + } if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsconcatloop") { continue } - - if !astutil.IsStringType(pass, assign.Lhs[0]) { + if nolint.HasDirectiveForLinter(loopPos, noLintIndex, "stringsconcatloop") { continue } - if !isInsideLoop(cur) { + if !astutil.IsStringType(pass, assign.Lhs[0]) { continue } pass.ReportRangef(assign, - "string concatenation with += inside a loop causes O(n²) allocations; use strings.Builder instead") + "string concatenation with += inside a loop allocates O(n) strings and O(n²) total bytes; use strings.Builder instead") } return nil, nil } -// isInsideLoop reports whether cur (an AssignStmt) is enclosed within a -// for or range loop body, without crossing a function literal boundary. -// Assignments inside func literals are exempt because they form a new scope. -func isInsideLoop(cur inspector.Cursor) bool { +// 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), @@ -86,10 +87,10 @@ func isInsideLoop(cur inspector.Cursor) bool { ) { switch encl.Node().(type) { case *ast.ForStmt, *ast.RangeStmt: - return true + return pass.Fset.PositionFor(encl.Node().Pos(), false), true case *ast.FuncLit: - return false + return token.Position{}, false } } - return false + return token.Position{}, false } diff --git a/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go index e16465f060c..9d52b42550d 100644 --- a/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go +++ b/pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go @@ -50,7 +50,8 @@ func good() { } _ = n - // String += inside a func literal inside a loop – not flagged (new scope). + // String += inside a func literal inside a loop – not flagged. The linter + // intentionally stops at func literal boundaries. acc := "" for _, p := range parts { func() { @@ -68,4 +69,11 @@ func nolintDirective() { result += p } _ = result + + result2 := "" + for _, p := range parts { //nolint:stringsconcatloop + _ = strings.TrimSpace(p) + result2 += p + } + _ = result2 }