diff --git a/Makefile b/Makefile index 75cbdb6..d63a6fd 100644 --- a/Makefile +++ b/Makefile @@ -47,7 +47,7 @@ help: @printf " make test Run the Go test suite\n" @printf " make codeguard-ci Validate and scan this repository with codeguard\n" @printf " make check Run fmt-check, lint, test, and codeguard-ci\n" - @printf " make ci Run the local CI gate\n" + @printf " make ci Run the full local CI gate, including golangci-lint\n" @printf " make build Build the codeguard CLI\n" @printf " make release Build snapshot release artifacts with GoReleaser\n" @printf " make release-check Validate GoReleaser config without publishing\n" @@ -76,7 +76,7 @@ lint: $(GO) vet ./... lint-strict: - golangci-lint run + env -u GOROOT golangci-lint run test: @set -o pipefail; $(GO) test ./... 2>&1 | grep -v '\[no test files\]' @@ -87,7 +87,7 @@ codeguard-ci: build check: fmt-check lint test codeguard-ci -ci: check +ci: fmt-check lint lint-strict test codeguard-ci build: @mkdir -p dist diff --git a/docs/checks.md b/docs/checks.md index 4ee8929..34fd0c3 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -453,6 +453,7 @@ Purpose: - Unbounded concurrency: goroutines launched from loops (Go), accumulated/detached threads or tasks (C++), promises created in loops without a limiter (TS/JS), `asyncio` tasks created in loops without a semaphore (Python) - Sequential `await` in TS/JS loops that could batch through `Promise.all` - Memory-pressure patterns: `time.After` timers leaked in Go loops, `setInterval` without `clearInterval` and listeners added in TS/JS loops without cleanup, unbounded whole-input reads (`io.ReadAll` in Go handlers/loops, `.read()`/`.readlines()` in Python loops) +- Hot-path patterns: repeated linear membership scans in Go loops (`slices.Contains` / `slices.Index` family and obvious nested membership scans), mutexes held across blocking calls in Go, per-iteration stream flushes in C++ loops (`std::endl`, `std::flush`), and range-for copies of heavy C++ values - Framework-aware smells, gated on file-level framework evidence: Django relation access in queryset loops, Django/SQLAlchemy ORM point queries in loops, expensive per-render work in React components, CPU-heavy synchronous calls in Express middleware - Change intelligence (diff scans): loop-nesting complexity regressions in functions touched by the diff - Measurement gates: artifact size budgets, clang `-ftime-trace` budgets, and `go test -bench` regression detection against a stored baseline @@ -479,6 +480,7 @@ Config keys: "detect_timer_leaks": true, "detect_unbounded_reads": true, "detect_complexity_regression": true, + "detect_hot_path_patterns": true, "detect_framework_patterns": true, "detect_rebuild_cascade": true } @@ -514,6 +516,11 @@ Rules: | `performance.{typescript,javascript}.timer-listener-leak` | TS, JS | `detect_timer_leaks` | | `performance.unbounded-read` | Go, Python | `detect_unbounded_reads` | | `performance.complexity-regression` | Go | `detect_complexity_regression` (diff scans only) | +| `performance.go.nested-loop-scan` | Go | `detect_hot_path_patterns` | +| `performance.go.slice-membership-in-loop` | Go | `detect_hot_path_patterns` | +| `performance.go.lock-held-across-blocking-call` | Go | `detect_hot_path_patterns` | +| `performance.cpp.range-for-copy` | C++ | `detect_hot_path_patterns` | +| `performance.cpp.flush-in-loop` | C++ | `detect_hot_path_patterns` | | `performance.python.django-nplusone-relation` | Python | `detect_framework_patterns` | | `performance.python.orm-query-in-loop` | Python | `detect_framework_patterns` | | `performance.{typescript,javascript}.react-expensive-render` | TS, JS | `detect_framework_patterns` | @@ -531,6 +538,11 @@ Notes on precision: - `defer-in-loop` scopes to the enclosing function: `defer wg.Done()` inside a goroutine launched from a loop runs per goroutine and is not flagged. - `await-in-loop` exempts `for await` streams and any file using a concurrency limiter (`p-limit`/`p-queue`); keep the loop (or disable the toggle) when iterations genuinely depend on each other. - `unbounded-read` does not fire when the reader is already bounded (`io.LimitReader`, `http.MaxBytesReader`, `read(n)`). +- `go.nested-loop-scan` is intentionally narrow: it looks for nested `range` loops where the equality test directly compares an outer loop variable to an inner loop variable. It does not yet infer indexed lookups, helper wrappers, or non-equality membership checks. +- `go.slice-membership-in-loop` currently targets explicit `slices.Contains` / `ContainsFunc` / `Index` / `IndexFunc` calls inside loops. It does not yet infer helper wrappers or prove the scanned slice is large enough to matter, so treat it as a hot-path review cue rather than a universal ban. +- `go.lock-held-across-blocking-call` targets obvious blocking calls while a `sync.Mutex` / `sync.RWMutex` is held: synchronous file I/O, `time.Sleep`, package-level HTTP calls, SQL-style `Query`/`Exec` methods on `db`/`tx`/`conn`/`pool`-shaped receivers, and `client.Do`. It is lexical rather than flow-sensitive, so treat it as a review cue when branches unlock before the call. +- `cpp.range-for-copy` currently flags structured bindings by value (`for (auto [k, v] : map)`) and explicit by-value iteration over obvious heavy standard-library types such as `std::string`, `std::vector`, and map/set pair types. Plain `auto value : items` is intentionally out of scope in this version. +- `cpp.flush-in-loop` matches `<< std::endl`, `<< endl`, `<< std::flush`, and `<< flush` inside loops. Ordinary newline writes such as `'\n'` are intentionally not flagged. - The TS/JS timer/listener rule treats any `clearInterval` in the file as interval cleanup, and any `removeEventListener`/`AbortSignal` usage as listener cleanup. - Python task creation is exempt when the file shows a bounding construct (`asyncio.Semaphore`, `TaskGroup`, `aiolimiter`, `anyio.CapacityLimiter`). @@ -563,10 +575,10 @@ When the performance section runs and produces findings, each target publishes a | Family | Rules | Weight | |---|---|---| | Query in loop (N+1) | `n-plus-one-query` | 5 | -| Blocking I/O | `sync-io-in-request-path`, `{typescript,javascript}.sync-io-in-handler`, `python.sync-io-in-async` | 4 | +| Blocking I/O | `sync-io-in-request-path`, `{typescript,javascript}.sync-io-in-handler`, `python.sync-io-in-async`, `go.lock-held-across-blocking-call` | 4 | | Unbounded concurrency | `unbounded-goroutines-in-loop`, `cpp.unbounded-concurrency`, `{typescript,javascript,python}.unbounded-concurrency` | 4 | | Memory pressure | `unbounded-read`, `go.timer-leak-in-loop`, `{typescript,javascript}.timer-listener-leak` | 3 | -| Repeated loop work | `regex-compile-in-loop`, `go.defer-in-loop`, `{go,rust,cpp}.sleep-in-loop`, `{typescript,javascript}.await-in-loop` | 2 | +| Repeated loop work | `regex-compile-in-loop`, `go.defer-in-loop`, `{go,rust,cpp}.sleep-in-loop`, `{typescript,javascript}.await-in-loop`, `go.nested-loop-scan`, `go.slice-membership-in-loop`, `cpp.range-for-copy`, `cpp.flush-in-loop` | 2 | | Allocation churn | `{go,rust,cpp}.alloc-in-loop`, `string-concat-in-loop` | 1 | The score trend is persisted per target next to the scan cache (`.perf-history.`) whenever the cache is enabled; subsequent scans annotate the artifact with `previous_score` and `delta`. `performance_rules.score_history: false` disables persistence and `performance_rules.score_history_limit` caps retained entries per target (default 100). Print the recorded trend with: diff --git a/internal/codeguard/checks/performance/performance_cpp.go b/internal/codeguard/checks/performance/performance_cpp.go index 10e81ff..a42eccc 100644 --- a/internal/codeguard/checks/performance/performance_cpp.go +++ b/internal/codeguard/checks/performance/performance_cpp.go @@ -17,6 +17,9 @@ var ( cppStringConcatPattern = regexp.MustCompile(`^\s*([A-Za-z_]\w*)\s*\+=`) cppStringAppendPattern = regexp.MustCompile(`\b([A-Za-z_]\w*)\.(?:append|push_back)\s*\(`) cppThreadSleepPattern = regexp.MustCompile(`\bstd::this_thread::sleep_(?:for|until)\s*\(`) + cppStreamFlushPattern = regexp.MustCompile(`<<\s*(?:std::)?(?:endl|flush)\b`) + cppRangeForStructuredCopy = regexp.MustCompile(`\bfor\s*\(\s*(?:const\s+)?auto\s*\[[^\]]+\]\s*:`) + cppRangeForValueCopy = regexp.MustCompile(`\bfor\s*\(\s*(?:const\s+)?(?:std::(?:basic_string\s*<[^>]+>|string|wstring|u8string|u16string|u32string|vector\s*<[^>]+>|array\s*<[^>]+>|map\s*<[^>]+>|unordered_map\s*<[^>]+>|set\s*<[^>]+>|unordered_set\s*<[^>]+>|pair\s*<[^>]+>|tuple\s*<[^>]+>)|[A-Za-z_]\w*::[A-Za-z_]\w*(?:\s*<[^>]+>)?)\s+[A-Za-z_]\w*\s*:`) cppUnboundedThreadPattern = regexp.MustCompile(`(?:\b[A-Za-z_]\w*\.(?:emplace_back|push_back)\s*\(\s*(?:std::)?(?:jthread|thread|async)\b|\bstd::(?:jthread|thread)\s*\([^;\n]*\)\s*\.detach\s*\()`) cppThreadVectorDecl = regexp.MustCompile(`\bstd::vector\s*<\s*std::(?:jthread|thread)\s*>\s*([A-Za-z_]\w*)`) cppContainerAppend = regexp.MustCompile(`\b([A-Za-z_]\w*)\.(?:emplace_back|push_back)\s*\(`) @@ -88,6 +91,14 @@ func (s *cppPerformanceScan) checkLine(lineNo int, line string, rawLine string, s.addFinding("performance.cpp.sleep-in-loop", lineNo, "std::this_thread::sleep_* inside a loop usually marks a poll; prefer a condition variable, timer primitive, or bounded backoff helper") } + if toggleEnabled(s.rules.DetectHotPathPatterns) && s.isRangeForCopy(rawLine) { + s.addFinding("performance.cpp.range-for-copy", lineNo, + "range-for loop copies each element by value; use const auto& or const T& unless a per-iteration copy is intentional") + } + if toggleEnabled(s.rules.DetectHotPathPatterns) && cppStreamFlushPattern.MatchString(line) { + s.addFinding("performance.cpp.flush-in-loop", lineNo, + "stream flush inside a loop defeats buffering and can dominate hot-path latency; write a newline and flush once after the loop unless immediate delivery is required") + } if toggleEnabled(s.rules.DetectUnboundedConcurrency) && s.isUnboundedThreadLaunch(line) { s.addFinding("performance.cpp.unbounded-concurrency", lineNo, "C++ thread/task launch accumulated or detached inside a loop has no visible concurrency bound; use a fixed worker pool, semaphore, or bounded executor") @@ -114,6 +125,10 @@ func (s *cppPerformanceScan) isStringGrowth(line string) bool { return false } +func (s *cppPerformanceScan) isRangeForCopy(line string) bool { + return cppRangeForStructuredCopy.MatchString(line) || cppRangeForValueCopy.MatchString(line) +} + func (s *cppPerformanceScan) addFinding(ruleID string, lineNo int, message string) { s.findings = append(s.findings, warnFinding(s.env, ruleID, s.file, lineNo, 1, message)) } diff --git a/internal/codeguard/checks/performance/performance_go_hotpath.go b/internal/codeguard/checks/performance/performance_go_hotpath.go new file mode 100644 index 0000000..5e76862 --- /dev/null +++ b/internal/codeguard/checks/performance/performance_go_hotpath.go @@ -0,0 +1,151 @@ +package performance + +import ( + "go/ast" + "go/token" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +var goSliceMembershipNames = map[string]struct{}{ + "Contains": {}, + "ContainsFunc": {}, + "Index": {}, + "IndexFunc": {}, +} + +type goHotPathConfig struct { + slicesAliases map[string]struct{} +} + +type goHotPathFinding struct { + ruleID string + pos token.Position + message string +} + +func goHotPathFindings(env support.Context, file string, fset *token.FileSet, parsed *ast.File) []core.Finding { + if !toggleEnabled(env.Config.Checks.PerformanceRules.DetectHotPathPatterns) { + return nil + } + cfg := newGoHotPathConfig(parsed) + if !cfg.enabled() { + return nil + } + + findings := make([]core.Finding, 0) + seen := make(map[int]struct{}) + walkASTWithStack(parsed, func(node ast.Node, stack []ast.Node) bool { + var finding *goHotPathFinding + switch node := node.(type) { + case *ast.CallExpr: + if hasLoopAncestor(stack) { + finding = cfg.callFinding(node, fset) + } + case *ast.BinaryExpr: + finding = cfg.binaryFinding(node, stack, fset) + } + if finding == nil { + return true + } + if _, dup := seen[finding.pos.Line]; !dup { + seen[finding.pos.Line] = struct{}{} + findings = append(findings, warnFinding(env, finding.ruleID, file, finding.pos.Line, finding.pos.Column, finding.message)) + } + return true + }) + return findings +} + +func newGoHotPathConfig(parsed *ast.File) goHotPathConfig { + aliases := importAliasesForPath(parsed, "slices") + for alias := range importAliasesForPath(parsed, "golang.org/x/exp/slices") { + aliases[alias] = struct{}{} + } + return goHotPathConfig{slicesAliases: aliases} +} + +func (c goHotPathConfig) enabled() bool { + return true +} + +func (c goHotPathConfig) callFinding(call *ast.CallExpr, fset *token.FileSet) *goHotPathFinding { + alias, name, ok := packageCall(call) + if !ok || !aliasHas(c.slicesAliases, alias) || !nameIn(goSliceMembershipNames, name) { + return nil + } + pos := fset.Position(call.Pos()) + return &goHotPathFinding{ + ruleID: "performance.go.slice-membership-in-loop", + pos: pos, + message: "slices membership scan inside a loop is linear each iteration; precompute a map/set or otherwise hoist the lookup if this path is hot", + } +} + +func (c goHotPathConfig) binaryFinding(expr *ast.BinaryExpr, stack []ast.Node, fset *token.FileSet) *goHotPathFinding { + if expr.Op != token.EQL { + return nil + } + outerVars, innerVars := nestedLoopVarSets(stack) + if len(outerVars) == 0 || len(innerVars) == 0 { + return nil + } + left := identName(expr.X) + right := identName(expr.Y) + if left == "" || right == "" { + return nil + } + if (!outerVars[left] || !innerVars[right]) && (!outerVars[right] || !innerVars[left]) { + return nil + } + pos := fset.Position(expr.Pos()) + return &goHotPathFinding{ + ruleID: "performance.go.nested-loop-scan", + pos: pos, + message: "nested loops compare outer items against inner items linearly; if this is a membership test on hot data, precompute a map/set for the inner collection", + } +} + +func nestedLoopVarSets(stack []ast.Node) (map[string]bool, map[string]bool) { + rangeLoops := make([]*ast.RangeStmt, 0, 4) + for _, node := range stack { + if loop, ok := node.(*ast.RangeStmt); ok { + rangeLoops = append(rangeLoops, loop) + } + } + if len(rangeLoops) < 2 { + return nil, nil + } + inner := rangeLoopVarSet(rangeLoops[len(rangeLoops)-1]) + outer := make(map[string]bool) + for _, loop := range rangeLoops[:len(rangeLoops)-1] { + for name := range rangeLoopVarSet(loop) { + outer[name] = true + } + } + return outer, inner +} + +func rangeLoopVarSet(loop *ast.RangeStmt) map[string]bool { + names := make(map[string]bool) + if loop == nil { + return names + } + if name := identName(loop.Key); name != "" && name != "_" { + names[name] = true + } + if name := identName(loop.Value); name != "" && name != "_" { + names[name] = true + } + return names +} + +func identName(expr ast.Expr) string { + ident, ok := expr.(*ast.Ident) + if !ok { + return "" + } + return strings.TrimSpace(ident.Name) +} diff --git a/internal/codeguard/checks/performance/performance_go_lock.go b/internal/codeguard/checks/performance/performance_go_lock.go new file mode 100644 index 0000000..bd3c4db --- /dev/null +++ b/internal/codeguard/checks/performance/performance_go_lock.go @@ -0,0 +1,271 @@ +package performance + +import ( + "go/ast" + "go/token" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +var ( + goBlockingHTTPNames = map[string]struct{}{ + "Get": {}, + "Head": {}, + "Post": {}, + "PostForm": {}, + } + goBlockingSQLMethodNames = map[string]struct{}{ + "Exec": {}, + "ExecContext": {}, + "Query": {}, + "QueryContext": {}, + "QueryRow": {}, + "QueryRowContext": {}, + } +) + +type goLockHeldConfig struct { + syncIOAliases map[string]map[string]struct{} + timeAliases map[string]struct{} + httpAliases map[string]struct{} + hasDatabase bool +} + +type goLockHeldScan struct { + env support.Context + file string + fset *token.FileSet + cfg goLockHeldConfig + findings []core.Finding + seen map[int]struct{} +} + +func goLockHeldFindings(env support.Context, file string, fset *token.FileSet, parsed *ast.File) []core.Finding { + if !toggleEnabled(env.Config.Checks.PerformanceRules.DetectHotPathPatterns) { + return nil + } + cfg := newGoLockHeldConfig(parsed) + scan := goLockHeldScan{ + env: env, + file: file, + fset: fset, + cfg: cfg, + findings: make([]core.Finding, 0), + seen: make(map[int]struct{}), + } + for _, decl := range parsed.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + scan.scanBlock(fn.Body, map[string]struct{}{}) + } + return scan.findings +} + +func newGoLockHeldConfig(parsed *ast.File) goLockHeldConfig { + _, hasDatabase := parsedImportPath(parsed, "database/sql") + return goLockHeldConfig{ + syncIOAliases: syncIOAliases(parsed), + timeAliases: importAliasesForPath(parsed, "time"), + httpAliases: importAliasesForPath(parsed, "net/http"), + hasDatabase: hasDatabase, + } +} + +func (s *goLockHeldScan) scanBlock(block *ast.BlockStmt, held map[string]struct{}) { + if block == nil { + return + } + for _, stmt := range block.List { + s.scanStmt(stmt, held) + } +} + +func (s *goLockHeldScan) scanStmt(stmt ast.Stmt, held map[string]struct{}) { + switch node := stmt.(type) { + case *ast.BlockStmt: + s.scanBlock(node, cloneHeldLocks(held)) + return + case *ast.IfStmt: + s.maybeReportBlockingCall(node.Init, held) + s.maybeReportBlockingExpr(node.Cond, held) + s.scanBlock(node.Body, cloneHeldLocks(held)) + if node.Else != nil { + s.scanStmt(node.Else, cloneHeldLocks(held)) + } + return + case *ast.ForStmt: + s.maybeReportBlockingCall(node.Init, held) + s.maybeReportBlockingExpr(node.Cond, held) + s.maybeReportBlockingCall(node.Post, held) + s.scanBlock(node.Body, cloneHeldLocks(held)) + return + case *ast.RangeStmt: + s.maybeReportBlockingExpr(node.X, held) + s.scanBlock(node.Body, cloneHeldLocks(held)) + return + case *ast.SwitchStmt: + s.maybeReportBlockingCall(node.Init, held) + s.maybeReportBlockingExpr(node.Tag, held) + s.scanBlock(node.Body, cloneHeldLocks(held)) + return + case *ast.TypeSwitchStmt: + s.maybeReportBlockingCall(node.Init, held) + s.maybeReportBlockingCall(node.Assign, held) + s.scanBlock(node.Body, cloneHeldLocks(held)) + return + case *ast.SelectStmt: + s.scanBlock(node.Body, cloneHeldLocks(held)) + return + } + + s.maybeReportBlockingCall(stmt, held) + if key, kind, ok := mutexCallInStmt(stmt); ok { + switch kind { + case "lock", "rlock": + held[key] = struct{}{} + case "unlock", "runlock": + delete(held, key) + } + } +} + +func (s *goLockHeldScan) maybeReportBlockingExpr(expr ast.Expr, held map[string]struct{}) { + if expr == nil { + return + } + s.reportBlockingNodes(expr, held) +} + +func (s *goLockHeldScan) maybeReportBlockingCall(node ast.Node, held map[string]struct{}) { + if node == nil { + return + } + s.reportBlockingNodes(node, held) +} + +func (s *goLockHeldScan) reportBlockingNodes(root ast.Node, held map[string]struct{}) { + if len(held) == 0 || root == nil { + return + } + ast.Inspect(root, func(node ast.Node) bool { + if _, ok := node.(*ast.FuncLit); ok { + return false + } + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + if blockingCallKind(s.cfg, call) == "" { + return true + } + pos := s.fset.Position(call.Pos()) + if _, dup := s.seen[pos.Line]; dup { + return false + } + s.seen[pos.Line] = struct{}{} + s.findings = append(s.findings, warnFinding(s.env, "performance.go.lock-held-across-blocking-call", s.file, pos.Line, pos.Column, + "mutex held across a blocking call can serialize callers and amplify tail latency; copy the needed state and release the lock before the call")) + return false + }) +} + +func mutexCallInStmt(stmt ast.Stmt) (string, string, bool) { + node, ok := stmt.(*ast.ExprStmt) + if !ok { + return "", "", false + } + call, _ := node.X.(*ast.CallExpr) + if call == nil { + return "", "", false + } + return mutexCallKind(call) +} + +func mutexCallKind(call *ast.CallExpr) (string, string, bool) { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return "", "", false + } + switch sel.Sel.Name { + case "Lock": + return normalizedExprString(sel.X), "lock", true + case "Unlock": + return normalizedExprString(sel.X), "unlock", true + case "RLock": + return normalizedExprString(sel.X), "rlock", true + case "RUnlock": + return normalizedExprString(sel.X), "runlock", true + default: + return "", "", false + } +} + +func blockingCallKind(cfg goLockHeldConfig, call *ast.CallExpr) string { + if _, _, ok := mutexCallKind(call); ok { + return "" + } + if alias, name, ok := packageCall(call); ok { + if operations, hit := cfg.syncIOAliases[alias]; hit { + if _, op := operations[name]; op { + return "sync-io" + } + } + if aliasHas(cfg.timeAliases, alias) && name == "Sleep" { + return "sleep" + } + if aliasHas(cfg.httpAliases, alias) && nameIn(goBlockingHTTPNames, name) { + return "http" + } + return "" + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return "" + } + if cfg.hasDatabase && nameIn(goBlockingSQLMethodNames, sel.Sel.Name) && looksLikeDatabaseReceiver(normalizedExprString(sel.X)) { + return "sql" + } + if len(cfg.httpAliases) > 0 && sel.Sel.Name == "Do" && looksLikeHTTPClientReceiver(normalizedExprString(sel.X)) { + return "http" + } + return "" +} + +func looksLikeDatabaseReceiver(name string) bool { + name = strings.ToLower(name) + switch name { + case "db", "tx", "stmt", "conn", "pool": + return true + } + return strings.HasSuffix(name, ".db") || + strings.HasSuffix(name, ".tx") || + strings.HasSuffix(name, ".stmt") || + strings.HasSuffix(name, ".conn") || + strings.HasSuffix(name, ".pool") +} + +func looksLikeHTTPClientReceiver(name string) bool { + name = strings.ToLower(name) + return strings.HasSuffix(name, "client") || strings.Contains(name, ".client") +} + +func parsedImportPath(parsed *ast.File, importPath string) (*ast.ImportSpec, bool) { + for _, imp := range parsed.Imports { + if strings.Trim(imp.Path.Value, `"`) == importPath { + return imp, true + } + } + return nil, false +} + +func cloneHeldLocks(src map[string]struct{}) map[string]struct{} { + dst := make(map[string]struct{}, len(src)) + for key := range src { + dst[key] = struct{}{} + } + return dst +} diff --git a/internal/codeguard/checks/performance/performance_go_loops.go b/internal/codeguard/checks/performance/performance_go_loops.go index 9c80cbe..36b1305 100644 --- a/internal/codeguard/checks/performance/performance_go_loops.go +++ b/internal/codeguard/checks/performance/performance_go_loops.go @@ -26,6 +26,7 @@ func toggleEnabled(value *bool) bool { func goPerformanceFindings(env support.Context, file string, fset *token.FileSet, parsed *ast.File) []core.Finding { findings := goCorePerformanceFindings(env, file, fset, parsed) + findings = append(findings, goLockHeldFindings(env, file, fset, parsed)...) findings = append(findings, goLoopCallFindings(env, file, fset, parsed)...) if toggleEnabled(env.Config.Checks.PerformanceRules.DetectNPlusOneQuery) { findings = append(findings, goNPlusOneFindings(env, file, fset, parsed)...) @@ -33,6 +34,7 @@ func goPerformanceFindings(env support.Context, file string, fset *token.FileSet if toggleEnabled(env.Config.Checks.PerformanceRules.DetectAllocInLoop) { findings = append(findings, goAllocInLoopFindings(env, file, fset, parsed)...) } + findings = append(findings, goHotPathFindings(env, file, fset, parsed)...) return findings } diff --git a/internal/codeguard/checks/performance/performance_score.go b/internal/codeguard/checks/performance/performance_score.go index b880c36..3594b01 100644 --- a/internal/codeguard/checks/performance/performance_score.go +++ b/internal/codeguard/checks/performance/performance_score.go @@ -19,10 +19,11 @@ import ( var performanceScoreWeights = map[string]int{ "performance.n-plus-one-query": 5, - "performance.sync-io-in-request-path": 4, - "performance.typescript.sync-io-in-handler": 4, - "performance.javascript.sync-io-in-handler": 4, - "performance.python.sync-io-in-async": 4, + "performance.sync-io-in-request-path": 4, + "performance.typescript.sync-io-in-handler": 4, + "performance.javascript.sync-io-in-handler": 4, + "performance.python.sync-io-in-async": 4, + "performance.go.lock-held-across-blocking-call": 4, "performance.unbounded-goroutines-in-loop": 4, "performance.cpp.unbounded-concurrency": 4, @@ -35,13 +36,17 @@ var performanceScoreWeights = map[string]int{ "performance.typescript.timer-listener-leak": 3, "performance.javascript.timer-listener-leak": 3, - "performance.regex-compile-in-loop": 2, - "performance.go.defer-in-loop": 2, - "performance.go.sleep-in-loop": 2, - "performance.cpp.sleep-in-loop": 2, - "performance.rust.sleep-in-loop": 2, - "performance.typescript.await-in-loop": 2, - "performance.javascript.await-in-loop": 2, + "performance.regex-compile-in-loop": 2, + "performance.go.defer-in-loop": 2, + "performance.go.sleep-in-loop": 2, + "performance.cpp.sleep-in-loop": 2, + "performance.rust.sleep-in-loop": 2, + "performance.go.nested-loop-scan": 2, + "performance.go.slice-membership-in-loop": 2, + "performance.cpp.range-for-copy": 2, + "performance.cpp.flush-in-loop": 2, + "performance.typescript.await-in-loop": 2, + "performance.javascript.await-in-loop": 2, "performance.go.alloc-in-loop": 1, "performance.cpp.alloc-in-loop": 1, diff --git a/internal/codeguard/config/defaults_rules.go b/internal/codeguard/config/defaults_rules.go index ffa01bb..aad12bb 100644 --- a/internal/codeguard/config/defaults_rules.go +++ b/internal/codeguard/config/defaults_rules.go @@ -48,6 +48,7 @@ func applyPerformanceDefaults(dst *core.PerformanceRulesConfig) { &dst.DetectTimerLeaks, &dst.DetectUnboundedReads, &dst.DetectComplexityRegression, + &dst.DetectHotPathPatterns, &dst.DetectFrameworkPatterns, &dst.DetectRebuildCascade, ) diff --git a/internal/codeguard/core/config_rule_types.go b/internal/codeguard/core/config_rule_types.go index 48d25aa..5260bb5 100644 --- a/internal/codeguard/core/config_rule_types.go +++ b/internal/codeguard/core/config_rule_types.go @@ -54,6 +54,11 @@ type PerformanceRulesConfig struct { // DetectComplexityRegression only applies in diff scans, where a base // revision exists for comparing loop nesting in changed functions. DetectComplexityRegression *bool `json:"detect_complexity_regression,omitempty" yaml:"detect_complexity_regression,omitempty"` + // DetectHotPathPatterns gates targeted hot-path smells that are cheap to + // identify statically but do not fit the broader loop/allocation toggles, + // such as repeated linear membership scans in Go loops and per-iteration + // stream flushes in C++ loops. + DetectHotPathPatterns *bool `json:"detect_hot_path_patterns,omitempty" yaml:"detect_hot_path_patterns,omitempty"` // DetectFrameworkPatterns gates the framework-aware rules: Django relation // access and ORM point queries in Python loops (Django/SQLAlchemy), // expensive per-render work in React components, and CPU-heavy synchronous diff --git a/tests/checks/performance_cpp_test.go b/tests/checks/performance_cpp_test.go index 1c9ffab..7b8b225 100644 --- a/tests/checks/performance_cpp_test.go +++ b/tests/checks/performance_cpp_test.go @@ -11,14 +11,14 @@ import ( func TestPerformanceCheckWarnsForCPPRegexAllocAndSleepInLoop(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "src", "render.cpp"), - "#include \n#include \n#include \n#include \n#include \n\nstd::string render(const std::vector& rows) {\n std::string out;\n for (const auto& row : rows) {\n std::regex digits(\"[0-9]+$\");\n if (std::regex_search(row, digits)) {\n out += row;\n }\n std::this_thread::sleep_for(std::chrono::milliseconds(1));\n }\n return out;\n}\n") + "#include \n#include \n#include \n#include \n#include \n#include \n\nstd::string render(const std::vector& rows) {\n std::ostringstream out;\n for (const auto& row : rows) {\n std::regex digits(\"[0-9]+$\");\n if (std::regex_search(row, digits)) {\n out << row << std::endl;\n }\n std::this_thread::sleep_for(std::chrono::milliseconds(1));\n }\n return out.str();\n}\n") report, err := codeguard.Run(context.Background(), performanceConfig("performance-cpp-loop-smells", dir, "c++")) if err != nil { t.Fatalf("run: %v", err) } assertFindingRulePresent(t, report, "Performance", "performance.regex-compile-in-loop") - assertFindingRulePresent(t, report, "Performance", "performance.cpp.alloc-in-loop") + assertFindingRulePresent(t, report, "Performance", "performance.cpp.flush-in-loop") assertFindingRulePresent(t, report, "Performance", "performance.cpp.sleep-in-loop") } @@ -33,5 +33,30 @@ func TestPerformanceCheckSkipsReservedCPPStringGrowth(t *testing.T) { } assertFindingRuleAbsent(t, report, "Performance", "performance.cpp.alloc-in-loop") assertFindingRuleAbsent(t, report, "Performance", "performance.regex-compile-in-loop") + assertFindingRuleAbsent(t, report, "Performance", "performance.cpp.flush-in-loop") assertFindingRuleAbsent(t, report, "Performance", "performance.cpp.sleep-in-loop") } + +func TestPerformanceCheckWarnsForCPPRangeForCopy(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "src", "pairs.cpp"), + "#include \n#include \n\nvoid emit(const std::map& values) {\n for (auto [key, value] : values) {\n (void)key;\n (void)value;\n }\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-cpp-range-copy", dir, "cpp")) + if err != nil { + t.Fatalf("run: %v", err) + } + assertFindingRulePresent(t, report, "Performance", "performance.cpp.range-for-copy") +} + +func TestPerformanceCheckSkipsCPPRangeForReference(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "src", "pairs.cpp"), + "#include \n#include \n\nvoid emit(const std::map& values) {\n for (const auto& [key, value] : values) {\n (void)key;\n (void)value;\n }\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-cpp-range-copy-neg", dir, "cpp")) + if err != nil { + t.Fatalf("run: %v", err) + } + assertFindingRuleAbsent(t, report, "Performance", "performance.cpp.range-for-copy") +} diff --git a/tests/checks/performance_go_rules_test.go b/tests/checks/performance_go_rules_test.go index 1511049..b975ac9 100644 --- a/tests/checks/performance_go_rules_test.go +++ b/tests/checks/performance_go_rules_test.go @@ -124,6 +124,119 @@ func TestPerformanceCheckAllocInLoopToggleOff(t *testing.T) { assertFindingRuleAbsent(t, report, "Performance", "performance.go.alloc-in-loop") } +func TestPerformanceCheckWarnsForGoSliceMembershipInLoop(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "filter.go"), + "package filter\n\nimport \"slices\"\n\nfunc KeepAllowed(ids []int, allowed []int) []int {\n\tout := make([]int, 0, len(ids))\n\tfor _, id := range ids {\n\t\tif slices.Contains(allowed, id) {\n\t\t\tout = append(out, id)\n\t\t}\n\t}\n\treturn out\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-slice-membership", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Performance", "performance.go.slice-membership-in-loop") +} + +func TestPerformanceCheckWarnsForGoNestedLoopMembershipScan(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "filter.go"), + "package filter\n\nfunc KeepAllowed(ids []int, allowed []int) []int {\n\tout := make([]int, 0, len(ids))\n\tfor _, id := range ids {\n\t\tfor _, allowedID := range allowed {\n\t\t\tif id == allowedID {\n\t\t\t\tout = append(out, id)\n\t\t\t\tbreak\n\t\t\t}\n\t\t}\n\t}\n\treturn out\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-nested-loop-scan", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Performance", "performance.go.nested-loop-scan") +} + +func TestPerformanceCheckSkipsNestedLoopWithoutMembershipCompare(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "matrix.go"), + "package matrix\n\nfunc SumPairs(xs []int, ys []int) int {\n\tout := 0\n\tfor _, x := range xs {\n\t\tfor _, y := range ys {\n\t\t\tout += x + y\n\t\t}\n\t}\n\treturn out\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-nested-loop-neg", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRuleAbsent(t, report, "Performance", "performance.go.nested-loop-scan") +} + +func TestPerformanceCheckSkipsGoSliceMembershipOutsideLoop(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "filter.go"), + "package filter\n\nimport \"slices\"\n\nfunc IsAllowed(id int, allowed []int) bool {\n\treturn slices.Contains(allowed, id)\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-slice-membership-neg", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRuleAbsent(t, report, "Performance", "performance.go.slice-membership-in-loop") +} + +func TestPerformanceCheckWarnsForGoLockHeldAcrossBlockingCall(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "state.go"), + "package state\n\nimport (\n\t\"os\"\n\t\"sync\"\n)\n\nfunc Load(path string) ([]byte, error) {\n\tvar mu sync.Mutex\n\tmu.Lock()\n\tdefer mu.Unlock()\n\treturn os.ReadFile(path)\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-lock-held", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRulePresent(t, report, "Performance", "performance.go.lock-held-across-blocking-call") +} + +func TestPerformanceCheckSkipsGoBlockingCallAfterUnlock(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "state.go"), + "package state\n\nimport (\n\t\"os\"\n\t\"sync\"\n)\n\nfunc Load(path string) ([]byte, error) {\n\tvar mu sync.Mutex\n\tmu.Lock()\n\tpathCopy := path\n\tmu.Unlock()\n\treturn os.ReadFile(pathCopy)\n}\n") + + report, err := codeguard.Run(context.Background(), performanceConfig("performance-go-lock-held-neg", dir, "go")) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRuleAbsent(t, report, "Performance", "performance.go.lock-held-across-blocking-call") +} + +func TestPerformanceCheckHotPathPatternsToggleOff(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "filter.go"), + "package filter\n\nimport \"slices\"\n\nfunc KeepAllowed(ids []int, allowed []int) []int {\n\tfor _, id := range ids {\n\t\tif slices.Contains(allowed, id) {\n\t\t\tfor _, allowedID := range allowed {\n\t\t\t\tif id == allowedID {\n\t\t\t\t\treturn []int{id}\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\t}\n\treturn nil\n}\n") + + off := false + cfg := performanceConfig("performance-go-hot-path-off", dir, "go") + cfg.Checks.PerformanceRules.DetectHotPathPatterns = &off + + report, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRuleAbsent(t, report, "Performance", "performance.go.slice-membership-in-loop") + assertFindingRuleAbsent(t, report, "Performance", "performance.go.nested-loop-scan") +} + +func TestPerformanceCheckHotPathPatternsToggleOffForLockHeld(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "state.go"), + "package state\n\nimport (\n\t\"os\"\n\t\"sync\"\n)\n\nfunc Load(path string) ([]byte, error) {\n\tvar mu sync.Mutex\n\tmu.Lock()\n\tdefer mu.Unlock()\n\treturn os.ReadFile(path)\n}\n") + + off := false + cfg := performanceConfig("performance-go-hot-path-lock-off", dir, "go") + cfg.Checks.PerformanceRules.DetectHotPathPatterns = &off + + report, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("run: %v", err) + } + + assertFindingRuleAbsent(t, report, "Performance", "performance.go.lock-held-across-blocking-call") +} + func TestQualityCheckNoLongerEmitsPerformanceRules(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "repo.go"),