From b5f8202f292ab3a8472ea8de4e6ee084550b1dc1 Mon Sep 17 00:00:00 2001 From: Peyton Spencer Date: Thu, 9 Jul 2026 14:42:14 -0400 Subject: [PATCH 1/5] Add review pattern lint analyzers --- go.work | 2 + stringbuilderlint/analyzer.go | 171 +++++++++++ stringbuilderlint/analyzer_test.go | 11 + stringbuilderlint/doc.go | 3 + stringbuilderlint/go.mod | 12 + stringbuilderlint/go.sum | 7 + stringbuilderlint/testdata/src/a/a.go | 22 ++ .../testdata/src/fileignore/fileignore.go | 6 + uncheckederrlint/analyzer.go | 272 ++++++++++++++++++ uncheckederrlint/analyzer_test.go | 11 + uncheckederrlint/doc.go | 3 + uncheckederrlint/go.mod | 12 + uncheckederrlint/go.sum | 7 + uncheckederrlint/testdata/src/a/a.go | 65 +++++ .../testdata/src/fileignore/fileignore.go | 9 + .../golang.org/x/sync/errgroup/errgroup.go | 7 + 16 files changed, 620 insertions(+) create mode 100644 stringbuilderlint/analyzer.go create mode 100644 stringbuilderlint/analyzer_test.go create mode 100644 stringbuilderlint/doc.go create mode 100644 stringbuilderlint/go.mod create mode 100644 stringbuilderlint/go.sum create mode 100644 stringbuilderlint/testdata/src/a/a.go create mode 100644 stringbuilderlint/testdata/src/fileignore/fileignore.go create mode 100644 uncheckederrlint/analyzer.go create mode 100644 uncheckederrlint/analyzer_test.go create mode 100644 uncheckederrlint/doc.go create mode 100644 uncheckederrlint/go.mod create mode 100644 uncheckederrlint/go.sum create mode 100644 uncheckederrlint/testdata/src/a/a.go create mode 100644 uncheckederrlint/testdata/src/fileignore/fileignore.go create mode 100644 uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go diff --git a/go.work b/go.work index a3ed35b..b5d63aa 100644 --- a/go.work +++ b/go.work @@ -17,7 +17,9 @@ use ( ./shutdown ./slicer ./sqlconv + ./stringbuilderlint ./structify ./strutils ./tasker + ./uncheckederrlint ) diff --git a/stringbuilderlint/analyzer.go b/stringbuilderlint/analyzer.go new file mode 100644 index 0000000..2656977 --- /dev/null +++ b/stringbuilderlint/analyzer.go @@ -0,0 +1,171 @@ +package stringbuilderlint + +import ( + "go/ast" + "go/constant" + "go/token" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" +) + +const analyzerName = "stringbuilderlint" + +// Analyzer reports string concatenation with more than two parts. +var Analyzer = &analysis.Analyzer{ + Name: analyzerName, + Doc: "check for string concatenation with more than two parts that should use strings.Builder", + URL: "https://pkg.go.dev/github.com/omniaura/go-kit/stringbuilderlint", + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + for _, file := range pass.Files { + if generated(file) { + continue + } + + parents := parentMap(file) + ignores := newSuppressions(pass, file) + ast.Inspect(file, func(node ast.Node) bool { + expr, ok := node.(*ast.BinaryExpr) + if !ok || expr.Op != token.ADD { + return true + } + if !isString(pass.TypesInfo.Types[expr].Type) { + return true + } + if isStringConcatParent(pass, parents, expr) { + return true + } + if pass.TypesInfo.Types[expr].Value != nil && pass.TypesInfo.Types[expr].Value.Kind() == constant.String { + return true + } + parts := countStringConcatParts(pass, expr) + if parts <= 2 { + return true + } + if ignores.ignored(expr.Pos()) { + return true + } + + pass.Reportf(expr.Pos(), "string concatenation with %d parts should use strings.Builder", parts) + return true + }) + } + + return nil, nil +} + +func parentMap(root ast.Node) map[ast.Node]ast.Node { + parents := make(map[ast.Node]ast.Node) + var stack []ast.Node + ast.Inspect(root, func(node ast.Node) bool { + if node == nil { + stack = stack[:len(stack)-1] + return true + } + if len(stack) > 0 { + parents[node] = stack[len(stack)-1] + } + stack = append(stack, node) + return true + }) + return parents +} + +func isStringConcatParent(pass *analysis.Pass, parents map[ast.Node]ast.Node, node ast.Node) bool { + parent := parents[node] + if paren, ok := parent.(*ast.ParenExpr); ok { + parent = parents[paren] + } + expr, ok := parent.(*ast.BinaryExpr) + return ok && expr.Op == token.ADD && isString(pass.TypesInfo.Types[expr].Type) +} + +func countStringConcatParts(pass *analysis.Pass, expr ast.Expr) int { + if paren, ok := expr.(*ast.ParenExpr); ok { + return countStringConcatParts(pass, paren.X) + } + binary, ok := expr.(*ast.BinaryExpr) + if !ok || binary.Op != token.ADD || !isString(pass.TypesInfo.Types[binary].Type) { + return 1 + } + return countStringConcatParts(pass, binary.X) + countStringConcatParts(pass, binary.Y) +} + +func isString(t types.Type) bool { + if t == nil { + return false + } + basic, ok := t.Underlying().(*types.Basic) + return ok && basic.Kind() == types.String +} + +func generated(file *ast.File) bool { + return ast.IsGenerated(file) +} + +type suppressions struct { + pass *analysis.Pass + line map[int]bool + file bool +} + +func newSuppressions(pass *analysis.Pass, file *ast.File) suppressions { + s := suppressions{ + pass: pass, + line: make(map[int]bool), + } + for _, group := range file.Comments { + for _, comment := range group.List { + kind, ok := directiveApplies(comment.Text) + if !ok { + continue + } + switch kind { + case "lint:ignore": + s.line[pass.Fset.PositionFor(comment.End(), false).Line+1] = true + case "lint:file-ignore": + s.file = true + } + } + } + return s +} + +func (s suppressions) ignored(pos token.Pos) bool { + return s.file || s.line[s.pass.Fset.PositionFor(pos, false).Line] +} + +func directiveApplies(raw string) (string, bool) { + fields := strings.Fields(commentText(raw)) + if len(fields) < 3 { + return "", false + } + kind := fields[0] + if kind != "lint:ignore" && kind != "lint:file-ignore" { + return "", false + } + for analyzer := range strings.SplitSeq(fields[1], ",") { + analyzer = strings.TrimSpace(analyzer) + if analyzer == analyzerName || analyzer == "all" || analyzer == "*" { + return kind, true + } + } + return "", false +} + +func commentText(raw string) string { + switch { + case strings.HasPrefix(raw, "//"): + return strings.TrimSpace(strings.TrimPrefix(raw, "//")) + case strings.HasPrefix(raw, "/*") && strings.HasSuffix(raw, "*/"): + raw = strings.TrimPrefix(raw, "/*") + raw = strings.TrimSuffix(raw, "*/") + return strings.TrimSpace(raw) + default: + return strings.TrimSpace(raw) + } +} diff --git a/stringbuilderlint/analyzer_test.go b/stringbuilderlint/analyzer_test.go new file mode 100644 index 0000000..f1d64fd --- /dev/null +++ b/stringbuilderlint/analyzer_test.go @@ -0,0 +1,11 @@ +package stringbuilderlint + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer, "a", "fileignore") +} diff --git a/stringbuilderlint/doc.go b/stringbuilderlint/doc.go new file mode 100644 index 0000000..6fb0cf8 --- /dev/null +++ b/stringbuilderlint/doc.go @@ -0,0 +1,3 @@ +// Package stringbuilderlint reports non-trivial string concatenation that +// should use strings.Builder. +package stringbuilderlint diff --git a/stringbuilderlint/go.mod b/stringbuilderlint/go.mod new file mode 100644 index 0000000..0f65536 --- /dev/null +++ b/stringbuilderlint/go.mod @@ -0,0 +1,12 @@ +module github.com/omniaura/go-kit/stringbuilderlint + +go 1.25.5 + +toolchain go1.26.1 + +require golang.org/x/tools v0.44.0 + +require ( + golang.org/x/mod v0.35.0 // indirect + golang.org/x/sync v0.21.0 // indirect +) diff --git a/stringbuilderlint/go.sum b/stringbuilderlint/go.sum new file mode 100644 index 0000000..fb61b4f --- /dev/null +++ b/stringbuilderlint/go.sum @@ -0,0 +1,7 @@ +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.35.0 h1:Ww1D637e6Pg+Zb2KrWfHQUnH2dQRLBQyAtpr/haaJeM= +golang.org/x/mod v0.35.0/go.mod h1:+GwiRhIInF8wPm+4AoT6L0FA1QWAad3OMdTRx4tFYlU= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= +golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= diff --git a/stringbuilderlint/testdata/src/a/a.go b/stringbuilderlint/testdata/src/a/a.go new file mode 100644 index 0000000..056ee91 --- /dev/null +++ b/stringbuilderlint/testdata/src/a/a.go @@ -0,0 +1,22 @@ +package a + +func twoParts(a, b string) string { + return a + b +} + +func threeParts(a, b, c string) string { + return a + ":" + b + ":" + c // want "string concatenation with 5 parts should use strings.Builder" +} + +func nested(a, b, c string) string { + return (a + b) + c // want "string concatenation with 3 parts should use strings.Builder" +} + +func constantOnly() string { + return "a" + "b" + "c" +} + +func ignored(a, b, c string) string { + //lint:ignore stringbuilderlint fixture verifies line suppression + return a + b + c +} diff --git a/stringbuilderlint/testdata/src/fileignore/fileignore.go b/stringbuilderlint/testdata/src/fileignore/fileignore.go new file mode 100644 index 0000000..86452c9 --- /dev/null +++ b/stringbuilderlint/testdata/src/fileignore/fileignore.go @@ -0,0 +1,6 @@ +//lint:file-ignore stringbuilderlint fixture verifies file suppression +package fileignore + +func ignoredFile(a, b, c string) string { + return a + b + c +} diff --git a/uncheckederrlint/analyzer.go b/uncheckederrlint/analyzer.go new file mode 100644 index 0000000..2b2350b --- /dev/null +++ b/uncheckederrlint/analyzer.go @@ -0,0 +1,272 @@ +package uncheckederrlint + +import ( + "go/ast" + "go/token" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" +) + +const analyzerName = "uncheckederrlint" + +// Analyzer reports discarded errors from JSON decoders, errgroup.Wait, and common write paths. +var Analyzer = &analysis.Analyzer{ + Name: analyzerName, + Doc: "check for discarded errors that should be handled or logged", + URL: "https://pkg.go.dev/github.com/omniaura/go-kit/uncheckederrlint", + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + for _, file := range pass.Files { + if generated(file) { + continue + } + + ignores := newSuppressions(pass, file) + ast.Inspect(file, func(node ast.Node) bool { + switch node := node.(type) { + case *ast.AssignStmt: + checkAssignment(pass, ignores, node) + case *ast.ExprStmt: + if call, ok := node.X.(*ast.CallExpr); ok { + reportDiscardedCall(pass, ignores, call) + } + } + return true + }) + } + + return nil, nil +} + +func checkAssignment(pass *analysis.Pass, ignores suppressions, stmt *ast.AssignStmt) { + if len(stmt.Rhs) != 1 { + return + } + call, ok := stmt.Rhs[0].(*ast.CallExpr) + if !ok || !hasBlankErrorResult(pass, stmt.Lhs, call) { + return + } + reportDiscardedCall(pass, ignores, call) +} + +func reportDiscardedCall(pass *analysis.Pass, ignores suppressions, call *ast.CallExpr) { + message, ok := uncheckedMessage(pass, call) + if !ok || ignores.ignored(call.Pos()) { + return + } + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + Message: message, + }) +} + +func hasBlankErrorResult(pass *analysis.Pass, lhs []ast.Expr, call *ast.CallExpr) bool { + resultType := pass.TypesInfo.Types[call].Type + if len(lhs) == 1 && isBlank(lhs[0]) && isErrorType(resultType) { + return true + } + + results := resultTuple(resultType) + if results == nil { + return false + } + + if results.Len() == len(lhs) { + for i, target := range lhs { + if isBlank(target) && isErrorType(results.At(i).Type()) { + return true + } + } + return false + } + + return len(lhs) == 1 && isBlank(lhs[0]) && results.Len() == 1 && isErrorType(results.At(0).Type()) +} + +func resultTuple(t types.Type) *types.Tuple { + if sig, ok := t.(*types.Signature); ok { + return sig.Results() + } + tuple, _ := t.(*types.Tuple) + return tuple +} + +func isBlank(expr ast.Expr) bool { + ident, ok := expr.(*ast.Ident) + return ok && ident.Name == "_" +} + +func uncheckedMessage(pass *analysis.Pass, call *ast.CallExpr) (string, bool) { + switch { + case isJSONCodecCall(pass, call): + return "discarded JSON encode/decode error should be handled or logged", true + case isErrgroupWaitCall(pass, call): + return "discarded errgroup.Wait error should be handled or use sync.WaitGroup when goroutines cannot fail", true + case isWritePathCall(pass, call): + return "discarded write-path error should be handled or logged", true + default: + return "", false + } +} + +func isJSONCodecCall(pass *analysis.Pass, call *ast.CallExpr) bool { + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if objectPackagePath(pass.TypesInfo.Uses[selector.Sel]) == "encoding/json" { + switch selector.Sel.Name { + case "Marshal", "MarshalIndent", "Unmarshal": + return true + } + } + switch selector.Sel.Name { + case "Decode", "Encode": + return objectPackagePath(selectedObject(pass, selector)) == "encoding/json" + default: + return false + } +} + +func isErrgroupWaitCall(pass *analysis.Pass, call *ast.CallExpr) bool { + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != "Wait" { + return false + } + return namedPackagePath(deref(pass.TypesInfo.Types[selector.X].Type)) == "golang.org/x/sync/errgroup" +} + +func isWritePathCall(pass *analysis.Pass, call *ast.CallExpr) bool { + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !hasErrorResult(pass.TypesInfo.Types[call].Type) { + return false + } + name := selector.Sel.Name + for _, prefix := range []string{"Claim", "Create", "Delete", "Grant", "Insert", "Revert", "Save", "Trigger", "Update", "Upsert"} { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} + +func selectedObject(pass *analysis.Pass, selector *ast.SelectorExpr) types.Object { + if selection := pass.TypesInfo.Selections[selector]; selection != nil { + return selection.Obj() + } + return pass.TypesInfo.Uses[selector.Sel] +} + +func objectPackagePath(obj types.Object) string { + if obj == nil || obj.Pkg() == nil { + return "" + } + return obj.Pkg().Path() +} + +func namedPackagePath(t types.Type) string { + named, ok := t.(*types.Named) + if !ok || named.Obj() == nil || named.Obj().Pkg() == nil { + return "" + } + return named.Obj().Pkg().Path() +} + +func deref(t types.Type) types.Type { + if ptr, ok := t.(*types.Pointer); ok { + return ptr.Elem() + } + return t +} + +func hasErrorResult(t types.Type) bool { + if isErrorType(t) { + return true + } + results := resultTuple(t) + if results == nil { + return false + } + for i := 0; i < results.Len(); i++ { + if isErrorType(results.At(i).Type()) { + return true + } + } + return false +} + +func isErrorType(t types.Type) bool { + named, ok := t.(*types.Named) + return ok && named.Obj() != nil && named.Obj().Name() == "error" && named.Obj().Pkg() == nil +} + +func generated(file *ast.File) bool { + return ast.IsGenerated(file) +} + +type suppressions struct { + pass *analysis.Pass + line map[int]bool + file bool +} + +func newSuppressions(pass *analysis.Pass, file *ast.File) suppressions { + s := suppressions{ + pass: pass, + line: make(map[int]bool), + } + for _, group := range file.Comments { + for _, comment := range group.List { + kind, ok := directiveApplies(comment.Text) + if !ok { + continue + } + switch kind { + case "lint:ignore": + s.line[pass.Fset.PositionFor(comment.End(), false).Line+1] = true + case "lint:file-ignore": + s.file = true + } + } + } + return s +} + +func (s suppressions) ignored(pos token.Pos) bool { + return s.file || s.line[s.pass.Fset.PositionFor(pos, false).Line] +} + +func directiveApplies(raw string) (string, bool) { + fields := strings.Fields(commentText(raw)) + if len(fields) < 3 { + return "", false + } + kind := fields[0] + if kind != "lint:ignore" && kind != "lint:file-ignore" { + return "", false + } + for analyzer := range strings.SplitSeq(fields[1], ",") { + analyzer = strings.TrimSpace(analyzer) + if analyzer == analyzerName || analyzer == "all" || analyzer == "*" { + return kind, true + } + } + return "", false +} + +func commentText(raw string) string { + switch { + case strings.HasPrefix(raw, "//"): + return strings.TrimSpace(strings.TrimPrefix(raw, "//")) + case strings.HasPrefix(raw, "/*") && strings.HasSuffix(raw, "*/"): + raw = strings.TrimPrefix(raw, "/*") + raw = strings.TrimSuffix(raw, "*/") + return strings.TrimSpace(raw) + default: + return strings.TrimSpace(raw) + } +} diff --git a/uncheckederrlint/analyzer_test.go b/uncheckederrlint/analyzer_test.go new file mode 100644 index 0000000..52af6b5 --- /dev/null +++ b/uncheckederrlint/analyzer_test.go @@ -0,0 +1,11 @@ +package uncheckederrlint + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer, "a", "fileignore") +} diff --git a/uncheckederrlint/doc.go b/uncheckederrlint/doc.go new file mode 100644 index 0000000..151ef63 --- /dev/null +++ b/uncheckederrlint/doc.go @@ -0,0 +1,3 @@ +// Package uncheckederrlint reports discarded errors that should usually be +// handled or logged. +package uncheckederrlint diff --git a/uncheckederrlint/go.mod b/uncheckederrlint/go.mod new file mode 100644 index 0000000..3fa7fb4 --- /dev/null +++ b/uncheckederrlint/go.mod @@ -0,0 +1,12 @@ +module github.com/omniaura/go-kit/uncheckederrlint + +go 1.25.5 + +toolchain go1.26.1 + +require golang.org/x/tools v0.44.0 + +require ( + golang.org/x/mod v0.35.0 // indirect + golang.org/x/sync v0.21.0 // indirect +) diff --git a/uncheckederrlint/go.sum b/uncheckederrlint/go.sum new file mode 100644 index 0000000..fb61b4f --- /dev/null +++ b/uncheckederrlint/go.sum @@ -0,0 +1,7 @@ +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.35.0 h1:Ww1D637e6Pg+Zb2KrWfHQUnH2dQRLBQyAtpr/haaJeM= +golang.org/x/mod v0.35.0/go.mod h1:+GwiRhIInF8wPm+4AoT6L0FA1QWAad3OMdTRx4tFYlU= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= +golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= diff --git a/uncheckederrlint/testdata/src/a/a.go b/uncheckederrlint/testdata/src/a/a.go new file mode 100644 index 0000000..530bb11 --- /dev/null +++ b/uncheckederrlint/testdata/src/a/a.go @@ -0,0 +1,65 @@ +package a + +import ( + "encoding/json" + "strings" + + "golang.org/x/sync/errgroup" +) + +type store struct{} + +func (store) CreateThing() (int, error) { + return 0, nil +} + +func (store) ReadThing() (int, error) { + return 0, nil +} + +func ignoredUnmarshal(data []byte) { + var value map[string]any + _ = json.Unmarshal(data, &value) // want "discarded JSON encode/decode error should be handled or logged" +} + +func ignoredMarshal(value map[string]any) { + _, _ = json.Marshal(value) // want "discarded JSON encode/decode error should be handled or logged" +} + +func ignoredDecode(raw string) { + var value map[string]any + _ = json.NewDecoder(strings.NewReader(raw)).Decode(&value) // want "discarded JSON encode/decode error should be handled or logged" +} + +func ignoredEncode(value map[string]any) { + var out strings.Builder + _ = json.NewEncoder(&out).Encode(value) // want "discarded JSON encode/decode error should be handled or logged" +} + +func ignoredErrgroupWait() { + var group errgroup.Group + _ = group.Wait() // want "discarded errgroup.Wait error should be handled or use sync.WaitGroup when goroutines cannot fail" +} + +func ignoredWritePath(s store) { + _, _ = s.CreateThing() // want "discarded write-path error should be handled or logged" +} + +func handled(data []byte, s store) error { + var value map[string]any + if err := json.Unmarshal(data, &value); err != nil { + return err + } + _, err := s.CreateThing() + return err +} + +func readOnlyIgnored(s store) { + _, _ = s.ReadThing() +} + +func ignoredWithComment(data []byte) { + var value map[string]any + //lint:ignore uncheckederrlint fixture verifies line suppression + _ = json.Unmarshal(data, &value) +} diff --git a/uncheckederrlint/testdata/src/fileignore/fileignore.go b/uncheckederrlint/testdata/src/fileignore/fileignore.go new file mode 100644 index 0000000..b8a2921 --- /dev/null +++ b/uncheckederrlint/testdata/src/fileignore/fileignore.go @@ -0,0 +1,9 @@ +//lint:file-ignore uncheckederrlint fixture verifies file suppression +package fileignore + +import "encoding/json" + +func ignoredFile(data []byte) { + var value map[string]any + _ = json.Unmarshal(data, &value) +} diff --git a/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go b/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go new file mode 100644 index 0000000..27fa4c2 --- /dev/null +++ b/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go @@ -0,0 +1,7 @@ +package errgroup + +type Group struct{} + +func (*Group) Wait() error { + return nil +} From 1dc5ea46ec92781c2a4307b9c17f9f52354a83c9 Mon Sep 17 00:00:00 2001 From: Peyton Spencer Date: Thu, 9 Jul 2026 15:59:39 -0400 Subject: [PATCH 2/5] Cover naked JSON codec calls --- uncheckederrlint/testdata/src/a/a.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/uncheckederrlint/testdata/src/a/a.go b/uncheckederrlint/testdata/src/a/a.go index 530bb11..f99a99f 100644 --- a/uncheckederrlint/testdata/src/a/a.go +++ b/uncheckederrlint/testdata/src/a/a.go @@ -22,10 +22,19 @@ func ignoredUnmarshal(data []byte) { _ = json.Unmarshal(data, &value) // want "discarded JSON encode/decode error should be handled or logged" } +func nakedUnmarshal(data []byte) { + var value map[string]any + json.Unmarshal(data, &value) // want "discarded JSON encode/decode error should be handled or logged" +} + func ignoredMarshal(value map[string]any) { _, _ = json.Marshal(value) // want "discarded JSON encode/decode error should be handled or logged" } +func nakedMarshal(value map[string]any) { + json.Marshal(value) // want "discarded JSON encode/decode error should be handled or logged" +} + func ignoredDecode(raw string) { var value map[string]any _ = json.NewDecoder(strings.NewReader(raw)).Decode(&value) // want "discarded JSON encode/decode error should be handled or logged" From 7e865d99722a3332d2abaa708b42a433fd448c68 Mon Sep 17 00:00:00 2001 From: Peyton Spencer Date: Thu, 9 Jul 2026 16:25:52 -0400 Subject: [PATCH 3/5] Add string builder fixes and benchmarks --- go.work | 1 - lookuptablelint/benchmark_test.go | 136 ++++ stringbuilderlint/analyzer.go | 692 +++++++++++++++++- stringbuilderlint/analyzer_test.go | 2 +- stringbuilderlint/benchmark_test.go | 86 +++ stringbuilderlint/doc.go | 4 +- stringbuilderlint/testdata/src/a/a.go.golden | 48 ++ .../testdata/src/sprintf/sprintf.go | 23 + .../testdata/src/sprintf/sprintf.go.golden | 48 ++ uncheckederrlint/analyzer.go | 272 ------- uncheckederrlint/analyzer_test.go | 11 - uncheckederrlint/doc.go | 3 - uncheckederrlint/go.mod | 12 - uncheckederrlint/go.sum | 7 - uncheckederrlint/testdata/src/a/a.go | 74 -- .../testdata/src/fileignore/fileignore.go | 9 - .../golang.org/x/sync/errgroup/errgroup.go | 7 - 17 files changed, 1002 insertions(+), 433 deletions(-) create mode 100644 lookuptablelint/benchmark_test.go create mode 100644 stringbuilderlint/benchmark_test.go create mode 100644 stringbuilderlint/testdata/src/a/a.go.golden create mode 100644 stringbuilderlint/testdata/src/sprintf/sprintf.go create mode 100644 stringbuilderlint/testdata/src/sprintf/sprintf.go.golden delete mode 100644 uncheckederrlint/analyzer.go delete mode 100644 uncheckederrlint/analyzer_test.go delete mode 100644 uncheckederrlint/doc.go delete mode 100644 uncheckederrlint/go.mod delete mode 100644 uncheckederrlint/go.sum delete mode 100644 uncheckederrlint/testdata/src/a/a.go delete mode 100644 uncheckederrlint/testdata/src/fileignore/fileignore.go delete mode 100644 uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go diff --git a/go.work b/go.work index b5d63aa..9bf6e5f 100644 --- a/go.work +++ b/go.work @@ -21,5 +21,4 @@ use ( ./structify ./strutils ./tasker - ./uncheckederrlint ) diff --git a/lookuptablelint/benchmark_test.go b/lookuptablelint/benchmark_test.go new file mode 100644 index 0000000..64e6fb7 --- /dev/null +++ b/lookuptablelint/benchmark_test.go @@ -0,0 +1,136 @@ +package lookuptablelint + +import "testing" + +var benchmarkBoolSink bool + +var lookupInputs = [...]string{ + "a", "b", "c", "d", "e", "f", "g", "h", + "i", "j", "k", "l", "m", "n", "o", "p", +} + +var lookupMap2 = map[string]struct{}{ + "a": {}, + "b": {}, +} + +var lookupMap4 = map[string]struct{}{ + "a": {}, + "b": {}, + "c": {}, + "d": {}, +} + +var lookupMap8 = map[string]struct{}{ + "a": {}, + "b": {}, + "c": {}, + "d": {}, + "e": {}, + "f": {}, + "g": {}, + "h": {}, +} + +var lookupMap16 = map[string]struct{}{ + "a": {}, + "b": {}, + "c": {}, + "d": {}, + "e": {}, + "f": {}, + "g": {}, + "h": {}, + "i": {}, + "j": {}, + "k": {}, + "l": {}, + "m": {}, + "n": {}, + "o": {}, + "p": {}, +} + +func lookupSwitch2(value string) bool { + switch value { + case "a", "b": + return true + default: + return false + } +} + +func lookupSwitch4(value string) bool { + switch value { + case "a", "b", "c", "d": + return true + default: + return false + } +} + +func lookupSwitch8(value string) bool { + switch value { + case "a", "b", "c", "d", "e", "f", "g", "h": + return true + default: + return false + } +} + +func lookupSwitch16(value string) bool { + switch value { + case "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p": + return true + default: + return false + } +} + +func BenchmarkMapLookup2(b *testing.B) { + for i := 0; b.Loop(); i++ { + _, benchmarkBoolSink = lookupMap2[lookupInputs[i&15]] + } +} + +func BenchmarkSwitchLookup2(b *testing.B) { + for i := 0; b.Loop(); i++ { + benchmarkBoolSink = lookupSwitch2(lookupInputs[i&15]) + } +} + +func BenchmarkMapLookup4(b *testing.B) { + for i := 0; b.Loop(); i++ { + _, benchmarkBoolSink = lookupMap4[lookupInputs[i&15]] + } +} + +func BenchmarkSwitchLookup4(b *testing.B) { + for i := 0; b.Loop(); i++ { + benchmarkBoolSink = lookupSwitch4(lookupInputs[i&15]) + } +} + +func BenchmarkMapLookup8(b *testing.B) { + for i := 0; b.Loop(); i++ { + _, benchmarkBoolSink = lookupMap8[lookupInputs[i&15]] + } +} + +func BenchmarkSwitchLookup8(b *testing.B) { + for i := 0; b.Loop(); i++ { + benchmarkBoolSink = lookupSwitch8(lookupInputs[i&15]) + } +} + +func BenchmarkMapLookup16(b *testing.B) { + for i := 0; b.Loop(); i++ { + _, benchmarkBoolSink = lookupMap16[lookupInputs[i&15]] + } +} + +func BenchmarkSwitchLookup16(b *testing.B) { + for i := 0; b.Loop(); i++ { + benchmarkBoolSink = lookupSwitch16(lookupInputs[i&15]) + } +} diff --git a/stringbuilderlint/analyzer.go b/stringbuilderlint/analyzer.go index 2656977..68c3edf 100644 --- a/stringbuilderlint/analyzer.go +++ b/stringbuilderlint/analyzer.go @@ -1,61 +1,557 @@ package stringbuilderlint import ( + "bytes" + "fmt" "go/ast" "go/constant" + "go/format" "go/token" "go/types" + "path" + "strconv" "strings" "golang.org/x/tools/go/analysis" ) -const analyzerName = "stringbuilderlint" +const ( + analyzerName = "stringbuilderlint" + stringsPath = "strings" + strconvPath = "strconv" +) -// Analyzer reports string concatenation with more than two parts. +// Analyzer reports string concatenation and primitive fmt.Sprintf calls that +// should use strings.Builder. var Analyzer = &analysis.Analyzer{ Name: analyzerName, - Doc: "check for string concatenation with more than two parts that should use strings.Builder", + Doc: "check for string concatenation and primitive fmt.Sprintf calls that should use strings.Builder", URL: "https://pkg.go.dev/github.com/omniaura/go-kit/stringbuilderlint", Run: run, } +type reporter struct { + pass *analysis.Pass + file *ast.File + parents map[ast.Node]ast.Node + ignores suppressions +} + func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { if generated(file) { continue } - parents := parentMap(file) - ignores := newSuppressions(pass, file) + r := reporter{ + pass: pass, + file: file, + parents: parentMap(file), + ignores: newSuppressions(pass, file), + } ast.Inspect(file, func(node ast.Node) bool { - expr, ok := node.(*ast.BinaryExpr) - if !ok || expr.Op != token.ADD { - return true + switch node := node.(type) { + case *ast.BinaryExpr: + r.checkConcat(node) + case *ast.CallExpr: + r.checkSprintf(node) } - if !isString(pass.TypesInfo.Types[expr].Type) { - return true - } - if isStringConcatParent(pass, parents, expr) { - return true - } - if pass.TypesInfo.Types[expr].Value != nil && pass.TypesInfo.Types[expr].Value.Kind() == constant.String { - return true + return true + }) + } + + return nil, nil +} + +func (r reporter) checkConcat(expr *ast.BinaryExpr) { + if expr.Op != token.ADD { + return + } + if !isString(r.pass.TypesInfo.Types[expr].Type) { + return + } + if isStringConcatParent(r.pass, r.parents, expr) { + return + } + if r.pass.TypesInfo.Types[expr].Value != nil && r.pass.TypesInfo.Types[expr].Value.Kind() == constant.String { + return + } + parts := stringConcatParts(r.pass, expr) + if len(parts) <= 2 { + return + } + if r.ignores.ignored(expr.Pos()) { + return + } + + pieces := make([]builderPiece, 0, len(parts)) + for _, part := range parts { + if literal, ok := stringLiteralPiece(r.pass, part); ok { + pieces = append(pieces, literal) + continue + } + text, ok := renderNode(r.pass.Fset, part) + if !ok { + return + } + pieces = append(pieces, builderPiece{ + expr: part, + exprText: text, + conversion: conversion{kind: convertString}, + }) + } + + fix, ok := r.builderFix(expr.Pos(), expr.End(), pieces, "Replace with strings.Builder") + diag := analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: fmt.Sprintf("string concatenation with %d parts should use strings.Builder", len(parts)), + } + if ok { + diag.SuggestedFixes = []analysis.SuggestedFix{fix} + } + r.pass.Report(diag) +} + +func (r reporter) checkSprintf(call *ast.CallExpr) { + if !isFmtSprintfCall(r.pass, call) || r.ignores.ignored(call.Pos()) { + return + } + pieces, ok := r.sprintfPieces(call) + if !ok { + return + } + fix, ok := r.builderFix(call.Pos(), call.End(), pieces, "Replace with strings.Builder") + diag := analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "fmt.Sprintf with primitive arguments should use strings.Builder and strconv", + } + if ok { + diag.SuggestedFixes = []analysis.SuggestedFix{fix} + } + r.pass.Report(diag) +} + +type builderPiece struct { + literal string + literalLen int + expr ast.Expr + exprText string + conversion conversion + tempName string +} + +func stringLiteralPiece(pass *analysis.Pass, expr ast.Expr) (builderPiece, bool) { + tv := pass.TypesInfo.Types[expr] + if tv.Value == nil || tv.Value.Kind() != constant.String { + return builderPiece{}, false + } + value := constant.StringVal(tv.Value) + return builderPiece{ + literal: strconv.Quote(value), + literalLen: len(value), + }, true +} + +type conversionKind int + +const ( + convertString conversionKind = iota + 1 + convertBool + convertSigned + convertUnsigned + convertFloat +) + +type conversion struct { + kind conversionKind + base int + verb byte + precision int + bitSize int + upper bool +} + +func (r reporter) sprintfPieces(call *ast.CallExpr) ([]builderPiece, bool) { + if len(call.Args) == 0 { + return nil, false + } + formatValue := r.pass.TypesInfo.Types[call.Args[0]].Value + if formatValue == nil || formatValue.Kind() != constant.String { + return nil, false + } + segments, ok := parseFormat(constant.StringVal(formatValue)) + if !ok { + return nil, false + } + + argIndex := 1 + pieces := make([]builderPiece, 0, len(segments)) + for _, segment := range segments { + if segment.literal != "" { + pieces = append(pieces, builderPiece{ + literal: strconv.Quote(segment.literal), + literalLen: len(segment.literal), + }) + continue + } + if argIndex >= len(call.Args) { + return nil, false + } + expr := call.Args[argIndex] + argIndex++ + conv, ok := primitiveConversion(r.pass.TypesInfo.Types[expr].Type, segment) + if !ok { + return nil, false + } + text, ok := renderNode(r.pass.Fset, expr) + if !ok { + return nil, false + } + pieces = append(pieces, builderPiece{ + expr: expr, + exprText: text, + conversion: conv, + }) + } + if argIndex != len(call.Args) { + return nil, false + } + return pieces, true +} + +type formatSegment struct { + literal string + verb byte + precision int +} + +func parseFormat(format string) ([]formatSegment, bool) { + var segments []formatSegment + var literal strings.Builder + for i := 0; i < len(format); i++ { + if format[i] != '%' { + literal.WriteByte(format[i]) + continue + } + if i+1 >= len(format) { + return nil, false + } + if format[i+1] == '%' { + literal.WriteByte('%') + i++ + continue + } + if literal.Len() > 0 { + segments = append(segments, formatSegment{literal: literal.String()}) + literal.Reset() + } + + i++ + if strings.ContainsRune("#0+- ", rune(format[i])) { + return nil, false + } + if format[i] >= '0' && format[i] <= '9' { + return nil, false + } + precision := -2 + if format[i] == '.' { + i++ + if i >= len(format) || format[i] < '0' || format[i] > '9' { + return nil, false } - parts := countStringConcatParts(pass, expr) - if parts <= 2 { - return true + precision = 0 + for i < len(format) && format[i] >= '0' && format[i] <= '9' { + precision = precision*10 + int(format[i]-'0') + i++ } - if ignores.ignored(expr.Pos()) { - return true + if i >= len(format) { + return nil, false } + } + if format[i] == '[' || format[i] == '*' { + return nil, false + } + segments = append(segments, formatSegment{ + verb: format[i], + precision: precision, + }) + } + if literal.Len() > 0 { + segments = append(segments, formatSegment{literal: literal.String()}) + } + return segments, true +} + +func primitiveConversion(t types.Type, segment formatSegment) (conversion, bool) { + basic, ok := t.Underlying().(*types.Basic) + if !ok { + return conversion{}, false + } + kind := basic.Kind() + switch segment.verb { + case 's': + return conversion{kind: convertString}, kind == types.String + case 't': + return conversion{kind: convertBool}, kind == types.Bool + case 'v': + switch { + case kind == types.String: + return conversion{kind: convertString}, true + case kind == types.Bool: + return conversion{kind: convertBool}, true + case signedInteger(kind): + return conversion{kind: convertSigned, base: 10}, true + case unsignedInteger(kind): + return conversion{kind: convertUnsigned, base: 10}, true + case floatKind(kind): + return conversion{kind: convertFloat, verb: 'g', precision: -1, bitSize: floatBitSize(kind)}, true + default: + return conversion{}, false + } + case 'd': + return integerConversion(kind, 10, false) + case 'b': + return integerConversion(kind, 2, false) + case 'o': + return integerConversion(kind, 8, false) + case 'x': + return integerConversion(kind, 16, false) + case 'X': + return integerConversion(kind, 16, true) + case 'e', 'E', 'f', 'F': + if !floatKind(kind) { + return conversion{}, false + } + verb := segment.verb + if verb == 'F' { + verb = 'f' + } + precision := segment.precision + if precision == -2 { + precision = 6 + } + return conversion{kind: convertFloat, verb: verb, precision: precision, bitSize: floatBitSize(kind)}, true + case 'g', 'G': + if !floatKind(kind) { + return conversion{}, false + } + precision := segment.precision + if precision == -2 { + precision = -1 + } + return conversion{kind: convertFloat, verb: segment.verb, precision: precision, bitSize: floatBitSize(kind)}, true + default: + return conversion{}, false + } +} + +func integerConversion(kind types.BasicKind, base int, upper bool) (conversion, bool) { + switch { + case signedInteger(kind): + return conversion{kind: convertSigned, base: base, upper: upper}, true + case unsignedInteger(kind): + return conversion{kind: convertUnsigned, base: base, upper: upper}, true + default: + return conversion{}, false + } +} + +func signedInteger(kind types.BasicKind) bool { + switch kind { + case types.Int, types.Int8, types.Int16, types.Int32, types.Int64: + return true + default: + return false + } +} + +func unsignedInteger(kind types.BasicKind) bool { + switch kind { + case types.Uint, types.Uint8, types.Uint16, types.Uint32, types.Uint64, types.Uintptr: + return true + default: + return false + } +} + +func floatKind(kind types.BasicKind) bool { + return kind == types.Float32 || kind == types.Float64 +} - pass.Reportf(expr.Pos(), "string concatenation with %d parts should use strings.Builder", parts) +func floatBitSize(kind types.BasicKind) int { + if kind == types.Float32 { + return 32 + } + return 64 +} + +func (r reporter) builderFix(start, end token.Pos, pieces []builderPiece, message string) (analysis.SuggestedFix, bool) { + needsStrconv := false + for _, piece := range pieces { + switch piece.conversion.kind { + case convertBool, convertSigned, convertUnsigned, convertFloat: + needsStrconv = true + } + } + + imports := []importNeed{{pkgPath: stringsPath, defaultName: "strings"}} + if needsStrconv { + imports = append(imports, importNeed{pkgPath: strconvPath, defaultName: "strconv"}) + } + qualifiers, importEdits, ok := r.importsFor(imports) + if !ok { + return analysis.SuggestedFix{}, false + } + + used := namesInPieces(pieces) + for _, qualifier := range qualifiers { + used[qualifier] = true + } + builderName := uniqueName(used, "sb") + used[builderName] = true + for i := range pieces { + if pieces[i].expr == nil { + continue + } + name := uniqueName(used, fmt.Sprintf("builderPart%d", i)) + used[name] = true + pieces[i].tempName = name + } + + replacement, ok := renderBuilderReplacement(pieces, builderName, qualifiers[stringsPath], qualifiers[strconvPath]) + if !ok { + return analysis.SuggestedFix{}, false + } + edits := append([]analysis.TextEdit{}, importEdits...) + edits = append(edits, analysis.TextEdit{Pos: start, End: end, NewText: []byte(replacement)}) + return analysis.SuggestedFix{Message: message, TextEdits: edits}, true +} + +func renderBuilderReplacement(pieces []builderPiece, builderName, stringsQualifier, strconvQualifier string) (string, bool) { + var buf strings.Builder + buf.WriteString("func() string {\n") + for _, piece := range pieces { + if piece.expr == nil { + continue + } + converted, ok := convertedExpr(piece, stringsQualifier, strconvQualifier) + if !ok { + return "", false + } + fmt.Fprintf(&buf, "\t%s := %s\n", piece.tempName, converted) + } + fmt.Fprintf(&buf, "\tvar %s %s.Builder\n", builderName, stringsQualifier) + growParts := growTerms(pieces) + if len(growParts) > 0 { + fmt.Fprintf(&buf, "\t%s.Grow(%s)\n", builderName, strings.Join(growParts, " + ")) + } + for _, piece := range pieces { + if piece.literal != "" { + fmt.Fprintf(&buf, "\t%s.WriteString(%s)\n", builderName, piece.literal) + continue + } + fmt.Fprintf(&buf, "\t%s.WriteString(%s)\n", builderName, piece.tempName) + } + fmt.Fprintf(&buf, "\treturn %s.String()\n", builderName) + buf.WriteString("}()") + + formatted, err := format.Source([]byte("package p\n\nvar _ = " + buf.String())) + if err != nil { + return buf.String(), true + } + const prefix = "package p\n\nvar _ = " + return strings.TrimSpace(strings.TrimPrefix(string(formatted), prefix)), true +} + +func convertedExpr(piece builderPiece, stringsQualifier, strconvQualifier string) (string, bool) { + switch piece.conversion.kind { + case convertString: + return piece.exprText, true + case convertBool: + return fmt.Sprintf("%s.FormatBool(%s)", strconvQualifier, piece.exprText), true + case convertSigned: + text := fmt.Sprintf("%s.FormatInt(int64(%s), %d)", strconvQualifier, piece.exprText, piece.conversion.base) + if piece.conversion.upper { + text = fmt.Sprintf("%s.ToUpper(%s)", stringsQualifier, text) + } + return text, true + case convertUnsigned: + text := fmt.Sprintf("%s.FormatUint(uint64(%s), %d)", strconvQualifier, piece.exprText, piece.conversion.base) + if piece.conversion.upper { + text = fmt.Sprintf("%s.ToUpper(%s)", stringsQualifier, text) + } + return text, true + case convertFloat: + return fmt.Sprintf("%s.FormatFloat(float64(%s), %q, %d, %d)", strconvQualifier, piece.exprText, piece.conversion.verb, piece.conversion.precision, piece.conversion.bitSize), true + default: + return "", false + } +} + +func growTerms(pieces []builderPiece) []string { + var terms []string + literalBytes := 0 + flushLiteral := func() { + if literalBytes == 0 { + return + } + terms = append(terms, strconv.Itoa(literalBytes)) + literalBytes = 0 + } + for _, piece := range pieces { + if piece.literal != "" { + literalBytes += piece.literalLen + continue + } + flushLiteral() + terms = append(terms, "len("+piece.tempName+")") + } + flushLiteral() + return terms +} + +func namesInPieces(pieces []builderPiece) map[string]bool { + used := make(map[string]bool) + for _, piece := range pieces { + if piece.expr == nil { + continue + } + ast.Inspect(piece.expr, func(node ast.Node) bool { + ident, ok := node.(*ast.Ident) + if ok { + used[ident.Name] = true + } return true }) } + return used +} - return nil, nil +func uniqueName(used map[string]bool, base string) string { + if !used[base] { + return base + } + for i := 1; ; i++ { + name := fmt.Sprintf("%s%d", base, i) + if !used[name] { + return name + } + } +} + +func stringConcatParts(pass *analysis.Pass, expr ast.Expr) []ast.Expr { + if paren, ok := expr.(*ast.ParenExpr); ok { + return stringConcatParts(pass, paren.X) + } + binary, ok := expr.(*ast.BinaryExpr) + if !ok || binary.Op != token.ADD || !isString(pass.TypesInfo.Types[binary].Type) { + return []ast.Expr{expr} + } + parts := stringConcatParts(pass, binary.X) + parts = append(parts, stringConcatParts(pass, binary.Y)...) + return parts } func parentMap(root ast.Node) map[ast.Node]ast.Node { @@ -84,17 +580,6 @@ func isStringConcatParent(pass *analysis.Pass, parents map[ast.Node]ast.Node, no return ok && expr.Op == token.ADD && isString(pass.TypesInfo.Types[expr].Type) } -func countStringConcatParts(pass *analysis.Pass, expr ast.Expr) int { - if paren, ok := expr.(*ast.ParenExpr); ok { - return countStringConcatParts(pass, paren.X) - } - binary, ok := expr.(*ast.BinaryExpr) - if !ok || binary.Op != token.ADD || !isString(pass.TypesInfo.Types[binary].Type) { - return 1 - } - return countStringConcatParts(pass, binary.X) + countStringConcatParts(pass, binary.Y) -} - func isString(t types.Type) bool { if t == nil { return false @@ -103,6 +588,145 @@ func isString(t types.Type) bool { return ok && basic.Kind() == types.String } +func isFmtSprintfCall(pass *analysis.Pass, call *ast.CallExpr) bool { + selector, ok := call.Fun.(*ast.SelectorExpr) + return ok && selector.Sel.Name == "Sprintf" && objectPackagePath(pass.TypesInfo.Uses[selector.Sel]) == "fmt" +} + +func objectPackagePath(obj types.Object) string { + if obj == nil || obj.Pkg() == nil { + return "" + } + return obj.Pkg().Path() +} + +func renderNode(fset *token.FileSet, node any) (string, bool) { + var buf bytes.Buffer + if err := format.Node(&buf, fset, node); err != nil { + return "", false + } + return buf.String(), true +} + +type importNeed struct { + pkgPath string + defaultName string +} + +func (r reporter) importsFor(needs []importNeed) (map[string]string, []analysis.TextEdit, bool) { + qualifiers := make(map[string]string, len(needs)) + var missing []importNeed + used := fileNames(r.file) + for _, need := range needs { + if qualifier, ok := importedQualifier(r.file, need.pkgPath); ok { + if qualifier == "" { + return nil, nil, false + } + qualifiers[need.pkgPath] = qualifier + used[qualifier] = true + continue + } + qualifier := uniqueName(used, need.defaultName) + used[qualifier] = true + qualifiers[need.pkgPath] = qualifier + missing = append(missing, importNeed{pkgPath: need.pkgPath, defaultName: qualifier}) + } + if len(missing) == 0 { + return qualifiers, nil, true + } + edits, ok := addImportEdits(r.pass.Fset, r.file, missing) + return qualifiers, edits, ok +} + +func importedQualifier(file *ast.File, pkgPath string) (string, bool) { + for _, spec := range file.Imports { + if strings.Trim(spec.Path.Value, `"`) != pkgPath { + continue + } + if spec.Name == nil { + return path.Base(pkgPath), true + } + if spec.Name.Name == "_" || spec.Name.Name == "." { + return "", true + } + return spec.Name.Name, true + } + return "", false +} + +func fileNames(file *ast.File) map[string]bool { + used := make(map[string]bool) + ast.Inspect(file, func(node ast.Node) bool { + ident, ok := node.(*ast.Ident) + if ok { + used[ident.Name] = true + } + return true + }) + for _, spec := range file.Imports { + if spec.Name != nil { + used[spec.Name.Name] = true + continue + } + used[path.Base(strings.Trim(spec.Path.Value, `"`))] = true + } + return used +} + +func addImportEdits(fset *token.FileSet, file *ast.File, missing []importNeed) ([]analysis.TextEdit, bool) { + specs := make([]string, 0, len(missing)) + for _, need := range missing { + specs = append(specs, importSpecText(need.defaultName, path.Base(need.pkgPath), need.pkgPath)) + } + importDecl := firstImportDecl(file) + if importDecl == nil { + return []analysis.TextEdit{{ + Pos: file.Name.End(), + End: file.Name.End(), + NewText: []byte("\n\nimport (\n\t" + strings.Join(specs, "\n\t") + "\n)\n"), + }}, true + } + + if importDecl.Lparen.IsValid() { + return []analysis.TextEdit{{ + Pos: importDecl.Rparen, + End: importDecl.Rparen, + NewText: []byte("\n\t" + strings.Join(specs, "\n\t")), + }}, true + } + + if len(importDecl.Specs) != 1 { + return nil, false + } + existing, ok := renderNode(fset, importDecl.Specs[0]) + if !ok { + return nil, false + } + allSpecs := append([]string{existing}, specs...) + return []analysis.TextEdit{{ + Pos: importDecl.Pos(), + End: importDecl.End(), + NewText: []byte("import (\n\t" + strings.Join(allSpecs, "\n\t") + "\n)"), + }}, true +} + +func importSpecText(qualifier, defaultName, pkgPath string) string { + if qualifier != defaultName { + return fmt.Sprintf("%s %q", qualifier, pkgPath) + } + return fmt.Sprintf("%q", pkgPath) +} + +func firstImportDecl(file *ast.File) *ast.GenDecl { + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if ok && gen.Tok == token.IMPORT { + return gen + } + } + return nil +} + func generated(file *ast.File) bool { return ast.IsGenerated(file) } diff --git a/stringbuilderlint/analyzer_test.go b/stringbuilderlint/analyzer_test.go index f1d64fd..c2a4a77 100644 --- a/stringbuilderlint/analyzer_test.go +++ b/stringbuilderlint/analyzer_test.go @@ -7,5 +7,5 @@ import ( ) func TestAnalyzer(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), Analyzer, "a", "fileignore") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), Analyzer, "a", "fileignore", "sprintf") } diff --git a/stringbuilderlint/benchmark_test.go b/stringbuilderlint/benchmark_test.go new file mode 100644 index 0000000..c9cdfc5 --- /dev/null +++ b/stringbuilderlint/benchmark_test.go @@ -0,0 +1,86 @@ +package stringbuilderlint + +import ( + "fmt" + "strconv" + "strings" + "testing" +) + +var benchmarkStringSink string + +var ( + benchA = "alpha" + benchB = "bravo" + benchC = "charlie" + benchD = "delta" + benchE = "echo" + benchCount = 12345 + benchEnabled = true +) + +func BenchmarkConcat3(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + } +} + +func BenchmarkBuilderConcat3(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + benchmarkStringSink = sb.String() + } +} + +func BenchmarkConcat5(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + benchD + benchE + } +} + +func BenchmarkBuilderConcat5(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + part3 := benchD + part4 := benchE + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2) + len(part3) + len(part4)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + sb.WriteString(part3) + sb.WriteString(part4) + benchmarkStringSink = sb.String() + } +} + +func BenchmarkSprintfPrimitive(b *testing.B) { + for b.Loop() { + benchmarkStringSink = fmt.Sprintf("%s/%d/%t", benchA, benchCount, benchEnabled) + } +} + +func BenchmarkBuilderSprintfPrimitive(b *testing.B) { + for b.Loop() { + part0 := benchA + part2 := strconv.FormatInt(int64(benchCount), 10) + part4 := strconv.FormatBool(benchEnabled) + var sb strings.Builder + sb.Grow(len(part0) + 1 + len(part2) + 1 + len(part4)) + sb.WriteString(part0) + sb.WriteString("/") + sb.WriteString(part2) + sb.WriteString("/") + sb.WriteString(part4) + benchmarkStringSink = sb.String() + } +} diff --git a/stringbuilderlint/doc.go b/stringbuilderlint/doc.go index 6fb0cf8..129d32d 100644 --- a/stringbuilderlint/doc.go +++ b/stringbuilderlint/doc.go @@ -1,3 +1,3 @@ -// Package stringbuilderlint reports non-trivial string concatenation that -// should use strings.Builder. +// Package stringbuilderlint reports non-trivial string concatenation and +// primitive fmt.Sprintf calls that should use strings.Builder. package stringbuilderlint diff --git a/stringbuilderlint/testdata/src/a/a.go.golden b/stringbuilderlint/testdata/src/a/a.go.golden new file mode 100644 index 0000000..69e6b2c --- /dev/null +++ b/stringbuilderlint/testdata/src/a/a.go.golden @@ -0,0 +1,48 @@ +package a + +import ( + "strings" +) + +func twoParts(a, b string) string { + return a + b +} + +func threeParts(a, b, c string) string { + return func() string { + builderPart0 := a + builderPart2 := b + builderPart4 := c + var sb strings.Builder + sb.Grow(len(builderPart0) + 1 + len(builderPart2) + 1 + len(builderPart4)) + sb.WriteString(builderPart0) + sb.WriteString(":") + sb.WriteString(builderPart2) + sb.WriteString(":") + sb.WriteString(builderPart4) + return sb.String() + }() // want "string concatenation with 5 parts should use strings.Builder" +} + +func nested(a, b, c string) string { + return func() string { + builderPart0 := a + builderPart1 := b + builderPart2 := c + var sb strings.Builder + sb.Grow(len(builderPart0) + len(builderPart1) + len(builderPart2)) + sb.WriteString(builderPart0) + sb.WriteString(builderPart1) + sb.WriteString(builderPart2) + return sb.String() + }() // want "string concatenation with 3 parts should use strings.Builder" +} + +func constantOnly() string { + return "a" + "b" + "c" +} + +func ignored(a, b, c string) string { + //lint:ignore stringbuilderlint fixture verifies line suppression + return a + b + c +} diff --git a/stringbuilderlint/testdata/src/sprintf/sprintf.go b/stringbuilderlint/testdata/src/sprintf/sprintf.go new file mode 100644 index 0000000..06c86af --- /dev/null +++ b/stringbuilderlint/testdata/src/sprintf/sprintf.go @@ -0,0 +1,23 @@ +package sprintf + +import "fmt" + +type payload struct { + Name string +} + +func primitive(name string, count int, enabled bool) string { + return fmt.Sprintf("%s/%d/%t", name, count, enabled) // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func floatPrimitive(name string, ratio float64) string { + return fmt.Sprintf("%s=%.2f", name, ratio) // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func unsupportedStruct(value payload) string { + return fmt.Sprintf("%v", value) +} + +func unsupportedWidth(name string) string { + return fmt.Sprintf("%10s", name) +} diff --git a/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden b/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden new file mode 100644 index 0000000..e97df35 --- /dev/null +++ b/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden @@ -0,0 +1,48 @@ +package sprintf + +import ( + "fmt" + "strconv" + "strings" +) + +type payload struct { + Name string +} + +func primitive(name string, count int, enabled bool) string { + return func() string { + builderPart0 := name + builderPart2 := strconv.FormatInt(int64(count), 10) + builderPart4 := strconv.FormatBool(enabled) + var sb strings.Builder + sb.Grow(len(builderPart0) + 1 + len(builderPart2) + 1 + len(builderPart4)) + sb.WriteString(builderPart0) + sb.WriteString("/") + sb.WriteString(builderPart2) + sb.WriteString("/") + sb.WriteString(builderPart4) + return sb.String() + }() // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func floatPrimitive(name string, ratio float64) string { + return func() string { + builderPart0 := name + builderPart2 := strconv.FormatFloat(float64(ratio), 'f', 2, 64) + var sb strings.Builder + sb.Grow(len(builderPart0) + 1 + len(builderPart2)) + sb.WriteString(builderPart0) + sb.WriteString("=") + sb.WriteString(builderPart2) + return sb.String() + }() // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func unsupportedStruct(value payload) string { + return fmt.Sprintf("%v", value) +} + +func unsupportedWidth(name string) string { + return fmt.Sprintf("%10s", name) +} diff --git a/uncheckederrlint/analyzer.go b/uncheckederrlint/analyzer.go deleted file mode 100644 index 2b2350b..0000000 --- a/uncheckederrlint/analyzer.go +++ /dev/null @@ -1,272 +0,0 @@ -package uncheckederrlint - -import ( - "go/ast" - "go/token" - "go/types" - "strings" - - "golang.org/x/tools/go/analysis" -) - -const analyzerName = "uncheckederrlint" - -// Analyzer reports discarded errors from JSON decoders, errgroup.Wait, and common write paths. -var Analyzer = &analysis.Analyzer{ - Name: analyzerName, - Doc: "check for discarded errors that should be handled or logged", - URL: "https://pkg.go.dev/github.com/omniaura/go-kit/uncheckederrlint", - Run: run, -} - -func run(pass *analysis.Pass) (any, error) { - for _, file := range pass.Files { - if generated(file) { - continue - } - - ignores := newSuppressions(pass, file) - ast.Inspect(file, func(node ast.Node) bool { - switch node := node.(type) { - case *ast.AssignStmt: - checkAssignment(pass, ignores, node) - case *ast.ExprStmt: - if call, ok := node.X.(*ast.CallExpr); ok { - reportDiscardedCall(pass, ignores, call) - } - } - return true - }) - } - - return nil, nil -} - -func checkAssignment(pass *analysis.Pass, ignores suppressions, stmt *ast.AssignStmt) { - if len(stmt.Rhs) != 1 { - return - } - call, ok := stmt.Rhs[0].(*ast.CallExpr) - if !ok || !hasBlankErrorResult(pass, stmt.Lhs, call) { - return - } - reportDiscardedCall(pass, ignores, call) -} - -func reportDiscardedCall(pass *analysis.Pass, ignores suppressions, call *ast.CallExpr) { - message, ok := uncheckedMessage(pass, call) - if !ok || ignores.ignored(call.Pos()) { - return - } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - Message: message, - }) -} - -func hasBlankErrorResult(pass *analysis.Pass, lhs []ast.Expr, call *ast.CallExpr) bool { - resultType := pass.TypesInfo.Types[call].Type - if len(lhs) == 1 && isBlank(lhs[0]) && isErrorType(resultType) { - return true - } - - results := resultTuple(resultType) - if results == nil { - return false - } - - if results.Len() == len(lhs) { - for i, target := range lhs { - if isBlank(target) && isErrorType(results.At(i).Type()) { - return true - } - } - return false - } - - return len(lhs) == 1 && isBlank(lhs[0]) && results.Len() == 1 && isErrorType(results.At(0).Type()) -} - -func resultTuple(t types.Type) *types.Tuple { - if sig, ok := t.(*types.Signature); ok { - return sig.Results() - } - tuple, _ := t.(*types.Tuple) - return tuple -} - -func isBlank(expr ast.Expr) bool { - ident, ok := expr.(*ast.Ident) - return ok && ident.Name == "_" -} - -func uncheckedMessage(pass *analysis.Pass, call *ast.CallExpr) (string, bool) { - switch { - case isJSONCodecCall(pass, call): - return "discarded JSON encode/decode error should be handled or logged", true - case isErrgroupWaitCall(pass, call): - return "discarded errgroup.Wait error should be handled or use sync.WaitGroup when goroutines cannot fail", true - case isWritePathCall(pass, call): - return "discarded write-path error should be handled or logged", true - default: - return "", false - } -} - -func isJSONCodecCall(pass *analysis.Pass, call *ast.CallExpr) bool { - selector, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return false - } - if objectPackagePath(pass.TypesInfo.Uses[selector.Sel]) == "encoding/json" { - switch selector.Sel.Name { - case "Marshal", "MarshalIndent", "Unmarshal": - return true - } - } - switch selector.Sel.Name { - case "Decode", "Encode": - return objectPackagePath(selectedObject(pass, selector)) == "encoding/json" - default: - return false - } -} - -func isErrgroupWaitCall(pass *analysis.Pass, call *ast.CallExpr) bool { - selector, ok := call.Fun.(*ast.SelectorExpr) - if !ok || selector.Sel.Name != "Wait" { - return false - } - return namedPackagePath(deref(pass.TypesInfo.Types[selector.X].Type)) == "golang.org/x/sync/errgroup" -} - -func isWritePathCall(pass *analysis.Pass, call *ast.CallExpr) bool { - selector, ok := call.Fun.(*ast.SelectorExpr) - if !ok || !hasErrorResult(pass.TypesInfo.Types[call].Type) { - return false - } - name := selector.Sel.Name - for _, prefix := range []string{"Claim", "Create", "Delete", "Grant", "Insert", "Revert", "Save", "Trigger", "Update", "Upsert"} { - if strings.HasPrefix(name, prefix) { - return true - } - } - return false -} - -func selectedObject(pass *analysis.Pass, selector *ast.SelectorExpr) types.Object { - if selection := pass.TypesInfo.Selections[selector]; selection != nil { - return selection.Obj() - } - return pass.TypesInfo.Uses[selector.Sel] -} - -func objectPackagePath(obj types.Object) string { - if obj == nil || obj.Pkg() == nil { - return "" - } - return obj.Pkg().Path() -} - -func namedPackagePath(t types.Type) string { - named, ok := t.(*types.Named) - if !ok || named.Obj() == nil || named.Obj().Pkg() == nil { - return "" - } - return named.Obj().Pkg().Path() -} - -func deref(t types.Type) types.Type { - if ptr, ok := t.(*types.Pointer); ok { - return ptr.Elem() - } - return t -} - -func hasErrorResult(t types.Type) bool { - if isErrorType(t) { - return true - } - results := resultTuple(t) - if results == nil { - return false - } - for i := 0; i < results.Len(); i++ { - if isErrorType(results.At(i).Type()) { - return true - } - } - return false -} - -func isErrorType(t types.Type) bool { - named, ok := t.(*types.Named) - return ok && named.Obj() != nil && named.Obj().Name() == "error" && named.Obj().Pkg() == nil -} - -func generated(file *ast.File) bool { - return ast.IsGenerated(file) -} - -type suppressions struct { - pass *analysis.Pass - line map[int]bool - file bool -} - -func newSuppressions(pass *analysis.Pass, file *ast.File) suppressions { - s := suppressions{ - pass: pass, - line: make(map[int]bool), - } - for _, group := range file.Comments { - for _, comment := range group.List { - kind, ok := directiveApplies(comment.Text) - if !ok { - continue - } - switch kind { - case "lint:ignore": - s.line[pass.Fset.PositionFor(comment.End(), false).Line+1] = true - case "lint:file-ignore": - s.file = true - } - } - } - return s -} - -func (s suppressions) ignored(pos token.Pos) bool { - return s.file || s.line[s.pass.Fset.PositionFor(pos, false).Line] -} - -func directiveApplies(raw string) (string, bool) { - fields := strings.Fields(commentText(raw)) - if len(fields) < 3 { - return "", false - } - kind := fields[0] - if kind != "lint:ignore" && kind != "lint:file-ignore" { - return "", false - } - for analyzer := range strings.SplitSeq(fields[1], ",") { - analyzer = strings.TrimSpace(analyzer) - if analyzer == analyzerName || analyzer == "all" || analyzer == "*" { - return kind, true - } - } - return "", false -} - -func commentText(raw string) string { - switch { - case strings.HasPrefix(raw, "//"): - return strings.TrimSpace(strings.TrimPrefix(raw, "//")) - case strings.HasPrefix(raw, "/*") && strings.HasSuffix(raw, "*/"): - raw = strings.TrimPrefix(raw, "/*") - raw = strings.TrimSuffix(raw, "*/") - return strings.TrimSpace(raw) - default: - return strings.TrimSpace(raw) - } -} diff --git a/uncheckederrlint/analyzer_test.go b/uncheckederrlint/analyzer_test.go deleted file mode 100644 index 52af6b5..0000000 --- a/uncheckederrlint/analyzer_test.go +++ /dev/null @@ -1,11 +0,0 @@ -package uncheckederrlint - -import ( - "testing" - - "golang.org/x/tools/go/analysis/analysistest" -) - -func TestAnalyzer(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), Analyzer, "a", "fileignore") -} diff --git a/uncheckederrlint/doc.go b/uncheckederrlint/doc.go deleted file mode 100644 index 151ef63..0000000 --- a/uncheckederrlint/doc.go +++ /dev/null @@ -1,3 +0,0 @@ -// Package uncheckederrlint reports discarded errors that should usually be -// handled or logged. -package uncheckederrlint diff --git a/uncheckederrlint/go.mod b/uncheckederrlint/go.mod deleted file mode 100644 index 3fa7fb4..0000000 --- a/uncheckederrlint/go.mod +++ /dev/null @@ -1,12 +0,0 @@ -module github.com/omniaura/go-kit/uncheckederrlint - -go 1.25.5 - -toolchain go1.26.1 - -require golang.org/x/tools v0.44.0 - -require ( - golang.org/x/mod v0.35.0 // indirect - golang.org/x/sync v0.21.0 // indirect -) diff --git a/uncheckederrlint/go.sum b/uncheckederrlint/go.sum deleted file mode 100644 index fb61b4f..0000000 --- a/uncheckederrlint/go.sum +++ /dev/null @@ -1,7 +0,0 @@ -github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -golang.org/x/mod v0.35.0 h1:Ww1D637e6Pg+Zb2KrWfHQUnH2dQRLBQyAtpr/haaJeM= -golang.org/x/mod v0.35.0/go.mod h1:+GwiRhIInF8wPm+4AoT6L0FA1QWAad3OMdTRx4tFYlU= -golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= -golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= -golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= diff --git a/uncheckederrlint/testdata/src/a/a.go b/uncheckederrlint/testdata/src/a/a.go deleted file mode 100644 index f99a99f..0000000 --- a/uncheckederrlint/testdata/src/a/a.go +++ /dev/null @@ -1,74 +0,0 @@ -package a - -import ( - "encoding/json" - "strings" - - "golang.org/x/sync/errgroup" -) - -type store struct{} - -func (store) CreateThing() (int, error) { - return 0, nil -} - -func (store) ReadThing() (int, error) { - return 0, nil -} - -func ignoredUnmarshal(data []byte) { - var value map[string]any - _ = json.Unmarshal(data, &value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func nakedUnmarshal(data []byte) { - var value map[string]any - json.Unmarshal(data, &value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func ignoredMarshal(value map[string]any) { - _, _ = json.Marshal(value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func nakedMarshal(value map[string]any) { - json.Marshal(value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func ignoredDecode(raw string) { - var value map[string]any - _ = json.NewDecoder(strings.NewReader(raw)).Decode(&value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func ignoredEncode(value map[string]any) { - var out strings.Builder - _ = json.NewEncoder(&out).Encode(value) // want "discarded JSON encode/decode error should be handled or logged" -} - -func ignoredErrgroupWait() { - var group errgroup.Group - _ = group.Wait() // want "discarded errgroup.Wait error should be handled or use sync.WaitGroup when goroutines cannot fail" -} - -func ignoredWritePath(s store) { - _, _ = s.CreateThing() // want "discarded write-path error should be handled or logged" -} - -func handled(data []byte, s store) error { - var value map[string]any - if err := json.Unmarshal(data, &value); err != nil { - return err - } - _, err := s.CreateThing() - return err -} - -func readOnlyIgnored(s store) { - _, _ = s.ReadThing() -} - -func ignoredWithComment(data []byte) { - var value map[string]any - //lint:ignore uncheckederrlint fixture verifies line suppression - _ = json.Unmarshal(data, &value) -} diff --git a/uncheckederrlint/testdata/src/fileignore/fileignore.go b/uncheckederrlint/testdata/src/fileignore/fileignore.go deleted file mode 100644 index b8a2921..0000000 --- a/uncheckederrlint/testdata/src/fileignore/fileignore.go +++ /dev/null @@ -1,9 +0,0 @@ -//lint:file-ignore uncheckederrlint fixture verifies file suppression -package fileignore - -import "encoding/json" - -func ignoredFile(data []byte) { - var value map[string]any - _ = json.Unmarshal(data, &value) -} diff --git a/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go b/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go deleted file mode 100644 index 27fa4c2..0000000 --- a/uncheckederrlint/testdata/src/golang.org/x/sync/errgroup/errgroup.go +++ /dev/null @@ -1,7 +0,0 @@ -package errgroup - -type Group struct{} - -func (*Group) Wait() error { - return nil -} From a83d01769aeecb13ac6af059a43d17aecc1ad9ea Mon Sep 17 00:00:00 2001 From: Peyton Spencer Date: Thu, 9 Jul 2026 16:51:49 -0400 Subject: [PATCH 4/5] Tune string builder lint cutoff --- stringbuilderlint/analyzer.go | 27 ++- stringbuilderlint/benchmark_test.go | 220 ++++++++++++++++++ stringbuilderlint/testdata/src/a/a.go | 12 +- stringbuilderlint/testdata/src/a/a.go.golden | 84 +++++-- .../testdata/src/sprintf/sprintf.go | 8 + .../testdata/src/sprintf/sprintf.go.golden | 15 ++ 6 files changed, 344 insertions(+), 22 deletions(-) diff --git a/stringbuilderlint/analyzer.go b/stringbuilderlint/analyzer.go index 68c3edf..a94a006 100644 --- a/stringbuilderlint/analyzer.go +++ b/stringbuilderlint/analyzer.go @@ -16,9 +16,11 @@ import ( ) const ( - analyzerName = "stringbuilderlint" - stringsPath = "strings" - strconvPath = "strconv" + analyzerName = "stringbuilderlint" + minConcatDynamicParts = 32 + minSprintfBuilderParts = 2 + stringsPath = "strings" + strconvPath = "strconv" ) // Analyzer reports string concatenation and primitive fmt.Sprintf calls that @@ -77,7 +79,8 @@ func (r reporter) checkConcat(expr *ast.BinaryExpr) { return } parts := stringConcatParts(r.pass, expr) - if len(parts) <= 2 { + dynamicParts := countDynamicStringParts(r.pass, parts) + if dynamicParts < minConcatDynamicParts { return } if r.ignores.ignored(expr.Pos()) { @@ -105,7 +108,7 @@ func (r reporter) checkConcat(expr *ast.BinaryExpr) { diag := analysis.Diagnostic{ Pos: expr.Pos(), End: expr.End(), - Message: fmt.Sprintf("string concatenation with %d parts should use strings.Builder", len(parts)), + Message: fmt.Sprintf("string concatenation with %d dynamic parts should use strings.Builder", dynamicParts), } if ok { diag.SuggestedFixes = []analysis.SuggestedFix{fix} @@ -121,6 +124,9 @@ func (r reporter) checkSprintf(call *ast.CallExpr) { if !ok { return } + if len(pieces) < minSprintfBuilderParts { + return + } fix, ok := r.builderFix(call.Pos(), call.End(), pieces, "Replace with strings.Builder") diag := analysis.Diagnostic{ Pos: call.Pos(), @@ -154,6 +160,17 @@ func stringLiteralPiece(pass *analysis.Pass, expr ast.Expr) (builderPiece, bool) }, true } +func countDynamicStringParts(pass *analysis.Pass, parts []ast.Expr) int { + count := 0 + for _, part := range parts { + if _, ok := stringLiteralPiece(pass, part); ok { + continue + } + count++ + } + return count +} + type conversionKind int const ( diff --git a/stringbuilderlint/benchmark_test.go b/stringbuilderlint/benchmark_test.go index c9cdfc5..1b55fdb 100644 --- a/stringbuilderlint/benchmark_test.go +++ b/stringbuilderlint/benchmark_test.go @@ -15,6 +15,33 @@ var ( benchC = "charlie" benchD = "delta" benchE = "echo" + benchF = "foxtrot" + benchG = "golf" + benchH = "hotel" + benchI = "india" + benchJ = "juliet" + benchK = "kilo" + benchL = "lima" + benchM = "mike" + benchN = "november" + benchO = "oscar" + benchP = "papa" + benchQ = "quebec" + benchR = "romeo" + benchS = "sierra" + benchT = "tango" + benchU = "uniform" + benchV = "victor" + benchW = "whiskey" + benchX = "xray" + benchY = "yankee" + benchZ = "zulu" + benchAA = "alpha-alpha" + benchAB = "alpha-bravo" + benchAC = "alpha-charlie" + benchAD = "alpha-delta" + benchAE = "alpha-echo" + benchAF = "alpha-foxtrot" benchCount = 12345 benchEnabled = true ) @@ -39,6 +66,28 @@ func BenchmarkBuilderConcat3(b *testing.B) { } } +func BenchmarkConcat4(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + benchD + } +} + +func BenchmarkBuilderConcat4(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + part3 := benchD + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2) + len(part3)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + sb.WriteString(part3) + benchmarkStringSink = sb.String() + } +} + func BenchmarkConcat5(b *testing.B) { for b.Loop() { benchmarkStringSink = benchA + benchB + benchC + benchD + benchE @@ -63,6 +112,177 @@ func BenchmarkBuilderConcat5(b *testing.B) { } } +func BenchmarkConcat8(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + benchD + benchE + benchF + benchG + benchH + } +} + +func BenchmarkBuilderConcat8(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + part3 := benchD + part4 := benchE + part5 := benchF + part6 := benchG + part7 := benchH + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2) + len(part3) + len(part4) + len(part5) + len(part6) + len(part7)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + sb.WriteString(part3) + sb.WriteString(part4) + sb.WriteString(part5) + sb.WriteString(part6) + sb.WriteString(part7) + benchmarkStringSink = sb.String() + } +} + +func BenchmarkConcat16(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + benchD + benchE + benchF + benchG + benchH + benchI + benchJ + benchK + benchL + benchM + benchN + benchO + benchP + } +} + +func BenchmarkBuilderConcat16(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + part3 := benchD + part4 := benchE + part5 := benchF + part6 := benchG + part7 := benchH + part8 := benchI + part9 := benchJ + part10 := benchK + part11 := benchL + part12 := benchM + part13 := benchN + part14 := benchO + part15 := benchP + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2) + len(part3) + len(part4) + len(part5) + len(part6) + len(part7) + len(part8) + len(part9) + len(part10) + len(part11) + len(part12) + len(part13) + len(part14) + len(part15)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + sb.WriteString(part3) + sb.WriteString(part4) + sb.WriteString(part5) + sb.WriteString(part6) + sb.WriteString(part7) + sb.WriteString(part8) + sb.WriteString(part9) + sb.WriteString(part10) + sb.WriteString(part11) + sb.WriteString(part12) + sb.WriteString(part13) + sb.WriteString(part14) + sb.WriteString(part15) + benchmarkStringSink = sb.String() + } +} + +func BenchmarkConcat32(b *testing.B) { + for b.Loop() { + benchmarkStringSink = benchA + benchB + benchC + benchD + benchE + benchF + benchG + benchH + benchI + benchJ + benchK + benchL + benchM + benchN + benchO + benchP + benchQ + benchR + benchS + benchT + benchU + benchV + benchW + benchX + benchY + benchZ + benchAA + benchAB + benchAC + benchAD + benchAE + benchAF + } +} + +func BenchmarkBuilderConcat32(b *testing.B) { + for b.Loop() { + part0 := benchA + part1 := benchB + part2 := benchC + part3 := benchD + part4 := benchE + part5 := benchF + part6 := benchG + part7 := benchH + part8 := benchI + part9 := benchJ + part10 := benchK + part11 := benchL + part12 := benchM + part13 := benchN + part14 := benchO + part15 := benchP + part16 := benchQ + part17 := benchR + part18 := benchS + part19 := benchT + part20 := benchU + part21 := benchV + part22 := benchW + part23 := benchX + part24 := benchY + part25 := benchZ + part26 := benchAA + part27 := benchAB + part28 := benchAC + part29 := benchAD + part30 := benchAE + part31 := benchAF + var sb strings.Builder + sb.Grow(len(part0) + len(part1) + len(part2) + len(part3) + len(part4) + len(part5) + len(part6) + len(part7) + len(part8) + len(part9) + len(part10) + len(part11) + len(part12) + len(part13) + len(part14) + len(part15) + len(part16) + len(part17) + len(part18) + len(part19) + len(part20) + len(part21) + len(part22) + len(part23) + len(part24) + len(part25) + len(part26) + len(part27) + len(part28) + len(part29) + len(part30) + len(part31)) + sb.WriteString(part0) + sb.WriteString(part1) + sb.WriteString(part2) + sb.WriteString(part3) + sb.WriteString(part4) + sb.WriteString(part5) + sb.WriteString(part6) + sb.WriteString(part7) + sb.WriteString(part8) + sb.WriteString(part9) + sb.WriteString(part10) + sb.WriteString(part11) + sb.WriteString(part12) + sb.WriteString(part13) + sb.WriteString(part14) + sb.WriteString(part15) + sb.WriteString(part16) + sb.WriteString(part17) + sb.WriteString(part18) + sb.WriteString(part19) + sb.WriteString(part20) + sb.WriteString(part21) + sb.WriteString(part22) + sb.WriteString(part23) + sb.WriteString(part24) + sb.WriteString(part25) + sb.WriteString(part26) + sb.WriteString(part27) + sb.WriteString(part28) + sb.WriteString(part29) + sb.WriteString(part30) + sb.WriteString(part31) + benchmarkStringSink = sb.String() + } +} + +func BenchmarkSprintfOnePrimitive(b *testing.B) { + for b.Loop() { + benchmarkStringSink = fmt.Sprintf("id=%d", benchCount) + } +} + +func BenchmarkBuilderSprintfOnePrimitive(b *testing.B) { + for b.Loop() { + part1 := strconv.FormatInt(int64(benchCount), 10) + var sb strings.Builder + sb.Grow(3 + len(part1)) + sb.WriteString("id=") + sb.WriteString(part1) + benchmarkStringSink = sb.String() + } +} + func BenchmarkSprintfPrimitive(b *testing.B) { for b.Loop() { benchmarkStringSink = fmt.Sprintf("%s/%d/%t", benchA, benchCount, benchEnabled) diff --git a/stringbuilderlint/testdata/src/a/a.go b/stringbuilderlint/testdata/src/a/a.go index 056ee91..641a981 100644 --- a/stringbuilderlint/testdata/src/a/a.go +++ b/stringbuilderlint/testdata/src/a/a.go @@ -5,11 +5,19 @@ func twoParts(a, b string) string { } func threeParts(a, b, c string) string { - return a + ":" + b + ":" + c // want "string concatenation with 5 parts should use strings.Builder" + return a + ":" + b + ":" + c } func nested(a, b, c string) string { - return (a + b) + c // want "string concatenation with 3 parts should use strings.Builder" + return (a + b) + c +} + +func fiveDynamic(a, b, c, d, e string) string { + return a + ":" + b + ":" + c + ":" + d + ":" + e +} + +func thirtyTwoDynamic(a string, b string, c string, d string, e string, f string, g string, h string, i string, j string, k string, l string, m string, n string, o string, p string, q string, r string, s string, t string, u string, v string, w string, x string, y string, z string, aa string, ab string, ac string, ad string, ae string, af string) string { + return a + b + c + d + e + f + g + h + i + j + k + l + m + n + o + p + q + r + s + t + u + v + w + x + y + z + aa + ab + ac + ad + ae + af // want "string concatenation with 32 dynamic parts should use strings.Builder" } func constantOnly() string { diff --git a/stringbuilderlint/testdata/src/a/a.go.golden b/stringbuilderlint/testdata/src/a/a.go.golden index 69e6b2c..9cb0623 100644 --- a/stringbuilderlint/testdata/src/a/a.go.golden +++ b/stringbuilderlint/testdata/src/a/a.go.golden @@ -9,33 +9,87 @@ func twoParts(a, b string) string { } func threeParts(a, b, c string) string { - return func() string { - builderPart0 := a - builderPart2 := b - builderPart4 := c - var sb strings.Builder - sb.Grow(len(builderPart0) + 1 + len(builderPart2) + 1 + len(builderPart4)) - sb.WriteString(builderPart0) - sb.WriteString(":") - sb.WriteString(builderPart2) - sb.WriteString(":") - sb.WriteString(builderPart4) - return sb.String() - }() // want "string concatenation with 5 parts should use strings.Builder" + return a + ":" + b + ":" + c } func nested(a, b, c string) string { + return (a + b) + c +} + +func fiveDynamic(a, b, c, d, e string) string { + return a + ":" + b + ":" + c + ":" + d + ":" + e +} + +func thirtyTwoDynamic(a string, b string, c string, d string, e string, f string, g string, h string, i string, j string, k string, l string, m string, n string, o string, p string, q string, r string, s string, t string, u string, v string, w string, x string, y string, z string, aa string, ab string, ac string, ad string, ae string, af string) string { return func() string { builderPart0 := a builderPart1 := b builderPart2 := c + builderPart3 := d + builderPart4 := e + builderPart5 := f + builderPart6 := g + builderPart7 := h + builderPart8 := i + builderPart9 := j + builderPart10 := k + builderPart11 := l + builderPart12 := m + builderPart13 := n + builderPart14 := o + builderPart15 := p + builderPart16 := q + builderPart17 := r + builderPart18 := s + builderPart19 := t + builderPart20 := u + builderPart21 := v + builderPart22 := w + builderPart23 := x + builderPart24 := y + builderPart25 := z + builderPart26 := aa + builderPart27 := ab + builderPart28 := ac + builderPart29 := ad + builderPart30 := ae + builderPart31 := af var sb strings.Builder - sb.Grow(len(builderPart0) + len(builderPart1) + len(builderPart2)) + sb.Grow(len(builderPart0) + len(builderPart1) + len(builderPart2) + len(builderPart3) + len(builderPart4) + len(builderPart5) + len(builderPart6) + len(builderPart7) + len(builderPart8) + len(builderPart9) + len(builderPart10) + len(builderPart11) + len(builderPart12) + len(builderPart13) + len(builderPart14) + len(builderPart15) + len(builderPart16) + len(builderPart17) + len(builderPart18) + len(builderPart19) + len(builderPart20) + len(builderPart21) + len(builderPart22) + len(builderPart23) + len(builderPart24) + len(builderPart25) + len(builderPart26) + len(builderPart27) + len(builderPart28) + len(builderPart29) + len(builderPart30) + len(builderPart31)) sb.WriteString(builderPart0) sb.WriteString(builderPart1) sb.WriteString(builderPart2) + sb.WriteString(builderPart3) + sb.WriteString(builderPart4) + sb.WriteString(builderPart5) + sb.WriteString(builderPart6) + sb.WriteString(builderPart7) + sb.WriteString(builderPart8) + sb.WriteString(builderPart9) + sb.WriteString(builderPart10) + sb.WriteString(builderPart11) + sb.WriteString(builderPart12) + sb.WriteString(builderPart13) + sb.WriteString(builderPart14) + sb.WriteString(builderPart15) + sb.WriteString(builderPart16) + sb.WriteString(builderPart17) + sb.WriteString(builderPart18) + sb.WriteString(builderPart19) + sb.WriteString(builderPart20) + sb.WriteString(builderPart21) + sb.WriteString(builderPart22) + sb.WriteString(builderPart23) + sb.WriteString(builderPart24) + sb.WriteString(builderPart25) + sb.WriteString(builderPart26) + sb.WriteString(builderPart27) + sb.WriteString(builderPart28) + sb.WriteString(builderPart29) + sb.WriteString(builderPart30) + sb.WriteString(builderPart31) return sb.String() - }() // want "string concatenation with 3 parts should use strings.Builder" + }() // want "string concatenation with 32 dynamic parts should use strings.Builder" } func constantOnly() string { diff --git a/stringbuilderlint/testdata/src/sprintf/sprintf.go b/stringbuilderlint/testdata/src/sprintf/sprintf.go index 06c86af..07b1592 100644 --- a/stringbuilderlint/testdata/src/sprintf/sprintf.go +++ b/stringbuilderlint/testdata/src/sprintf/sprintf.go @@ -14,6 +14,14 @@ func floatPrimitive(name string, ratio float64) string { return fmt.Sprintf("%s=%.2f", name, ratio) // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" } +func onePrimitive(count int) string { + return fmt.Sprintf("id=%d", count) // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func barePrimitive(count int) string { + return fmt.Sprintf("%d", count) +} + func unsupportedStruct(value payload) string { return fmt.Sprintf("%v", value) } diff --git a/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden b/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden index e97df35..fede2a3 100644 --- a/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden +++ b/stringbuilderlint/testdata/src/sprintf/sprintf.go.golden @@ -39,6 +39,21 @@ func floatPrimitive(name string, ratio float64) string { }() // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" } +func onePrimitive(count int) string { + return func() string { + builderPart1 := strconv.FormatInt(int64(count), 10) + var sb strings.Builder + sb.Grow(3 + len(builderPart1)) + sb.WriteString("id=") + sb.WriteString(builderPart1) + return sb.String() + }() // want "fmt.Sprintf with primitive arguments should use strings.Builder and strconv" +} + +func barePrimitive(count int) string { + return fmt.Sprintf("%d", count) +} + func unsupportedStruct(value payload) string { return fmt.Sprintf("%v", value) } From 74a03a539f3f225ccd6412fecd5cca2e625941cd Mon Sep 17 00:00:00 2001 From: Peyton Spencer Date: Thu, 9 Jul 2026 17:04:13 -0400 Subject: [PATCH 5/5] add benchmark dispatch workflow --- .github/workflows/go.yml | 132 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ef47dbe..c050f4c 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -5,9 +5,21 @@ on: branches: - main pull_request: + workflow_dispatch: + inputs: + arch: + description: Runner architecture for benchmarks + required: true + default: all + type: choice + options: + - all + - amd64 + - arm64 jobs: test: + if: ${{ github.event_name != 'workflow_dispatch' }} runs-on: ubuntu-latest env: GOTOOLCHAIN: local @@ -29,3 +41,123 @@ jobs: - name: Run workspace tests run: ./scripts/test-all.sh + + benchmark-amd64: + name: Benchmarks (linux/amd64) + if: ${{ github.event_name == 'workflow_dispatch' && (inputs.arch == 'all' || inputs.arch == 'amd64') }} + runs-on: ubuntu-latest + env: + ARCH: amd64 + GOTOOLCHAIN: local + steps: + - name: Check out repository + uses: actions/checkout@v7 + + - name: Set up latest stable Go + uses: actions/setup-go@v6 + with: + go-version: stable + check-latest: true + + - name: Run string builder benchmarks + shell: bash + run: | + set -euo pipefail + mkdir -p benchmark-results + { + printf '# stringbuilderlint benchmarks (%s/%s)\n' "$(go env GOOS)" "$(go env GOARCH)" + go version + printf '\n' + cd stringbuilderlint + go test -run '^$' -bench 'Benchmark(Concat|BuilderConcat|Sprintf|BuilderSprintf)' -benchmem -count 5 + } | tee "benchmark-results/stringbuilderlint-${ARCH}.txt" + + - name: Run lookup table benchmarks + shell: bash + run: | + set -euo pipefail + mkdir -p benchmark-results + { + printf '# lookuptablelint benchmarks (%s/%s)\n' "$(go env GOOS)" "$(go env GOARCH)" + go version + printf '\n' + cd lookuptablelint + go test -run '^$' -bench 'Benchmark(MapLookup|SwitchLookup)' -benchmem -count 5 + } | tee "benchmark-results/lookuptablelint-${ARCH}.txt" + + - name: Write benchmark summary + shell: bash + run: | + set -euo pipefail + { + printf '## Benchmarks (linux/%s)\n\n' "${ARCH}" + printf '```text\n' + cat benchmark-results/*.txt + printf '```\n' + } >> "${GITHUB_STEP_SUMMARY}" + + - name: Upload benchmark results + uses: actions/upload-artifact@v7 + with: + name: benchmark-results-amd64 + path: benchmark-results/*.txt + + benchmark-arm64: + name: Benchmarks (linux/arm64) + if: ${{ github.event_name == 'workflow_dispatch' && (inputs.arch == 'all' || inputs.arch == 'arm64') }} + runs-on: ubuntu-24.04-arm + env: + ARCH: arm64 + GOTOOLCHAIN: local + steps: + - name: Check out repository + uses: actions/checkout@v7 + + - name: Set up latest stable Go + uses: actions/setup-go@v6 + with: + go-version: stable + check-latest: true + + - name: Run string builder benchmarks + shell: bash + run: | + set -euo pipefail + mkdir -p benchmark-results + { + printf '# stringbuilderlint benchmarks (%s/%s)\n' "$(go env GOOS)" "$(go env GOARCH)" + go version + printf '\n' + cd stringbuilderlint + go test -run '^$' -bench 'Benchmark(Concat|BuilderConcat|Sprintf|BuilderSprintf)' -benchmem -count 5 + } | tee "benchmark-results/stringbuilderlint-${ARCH}.txt" + + - name: Run lookup table benchmarks + shell: bash + run: | + set -euo pipefail + mkdir -p benchmark-results + { + printf '# lookuptablelint benchmarks (%s/%s)\n' "$(go env GOOS)" "$(go env GOARCH)" + go version + printf '\n' + cd lookuptablelint + go test -run '^$' -bench 'Benchmark(MapLookup|SwitchLookup)' -benchmem -count 5 + } | tee "benchmark-results/lookuptablelint-${ARCH}.txt" + + - name: Write benchmark summary + shell: bash + run: | + set -euo pipefail + { + printf '## Benchmarks (linux/%s)\n\n' "${ARCH}" + printf '```text\n' + cat benchmark-results/*.txt + printf '```\n' + } >> "${GITHUB_STEP_SUMMARY}" + + - name: Upload benchmark results + uses: actions/upload-artifact@v7 + with: + name: benchmark-results-arm64 + path: benchmark-results/*.txt