From b930b5111468b3314765e90ec1a01fa10ed68e7d Mon Sep 17 00:00:00 2001 From: Corey Schuman Date: Mon, 5 Jan 2026 22:48:45 -0500 Subject: [PATCH] fix: address critical issues from panel review Security hardening: - Add parser timeout (5s) to prevent DoS via malicious files - Add AST complexity limits (100k nodes, 1k depth) - Add ASTComplexityValidator to enforce limits during walks - Wrap parsing in goroutine with panic recovery Error handling: - Analyze() now returns ([]Issue, []string) with errors - No more silent file/parse error swallowing - Verbose mode logs skipped files Honesty: - Rename --fix to --suggest (no files are modified) - Update all "auto-fix" language to "suggestion" - Clear documentation that fixes are not applied Config file support: - Add config/config.go with YAML config loading - Support .goperf.yml for team settings - Add .goperf.yml.example template Documentation: - Create docs/RULES.md with all 20+ rules documented - Include false positive rate disclosure (77%) - Add examples, fixes, and edge cases for each rule Testing: - Add rules/analyzer_test.go with 7 test functions - Add testdata/ fixtures for analyzer tests - Tests cover basics, ignore paths, empty files, syntax errors, context extraction, and multi-file analysis Generated using OpenAI Codex CLI with parallel task execution. --- .goperf.yml.example | 26 + CHANGELOG.md | 4 +- CONTRIBUTING.md | 2 +- README.md | 39 +- SECURITY.md | 6 +- config/config.go | 38 ++ docs/RULES.md | 791 ++++++++++++++++++++++++++++++ fixer/fixer.go | 26 +- go.mod | 2 + go.sum | 4 + main.go | 71 ++- rules/analyzer.go | 116 ++++- rules/analyzer_test.go | 211 ++++++++ rules/types.go | 2 +- testdata/analyzer_basic.go | 17 + testdata/analyzer_context.go | 9 + testdata/analyzer_empty.go | 0 testdata/analyzer_ignored.go | 9 + testdata/analyzer_multi_a.go | 9 + testdata/analyzer_multi_b.go | 9 + testdata/analyzer_scanned.go | 9 + testdata/analyzer_syntax_error.go | 6 + 22 files changed, 1349 insertions(+), 57 deletions(-) create mode 100644 .goperf.yml.example create mode 100644 config/config.go create mode 100644 docs/RULES.md create mode 100644 go.sum create mode 100644 rules/analyzer_test.go create mode 100644 testdata/analyzer_basic.go create mode 100644 testdata/analyzer_context.go create mode 100644 testdata/analyzer_empty.go create mode 100644 testdata/analyzer_ignored.go create mode 100644 testdata/analyzer_multi_a.go create mode 100644 testdata/analyzer_multi_b.go create mode 100644 testdata/analyzer_scanned.go create mode 100644 testdata/analyzer_syntax_error.go diff --git a/.goperf.yml.example b/.goperf.yml.example new file mode 100644 index 0000000..27bc954 --- /dev/null +++ b/.goperf.yml.example @@ -0,0 +1,26 @@ +# goperf configuration file +# Place this file as .goperf.yml in your project root. + +# rules: list of rule categories to run (use "all" for everything) +rules: + - algorithm + - database + - concurrency + +# ignore_paths: path substrings to skip during file collection +ignore_paths: + - vendor + - testdata + - generated + +# fail_on: exit 1 if issues at this level or higher (low, medium, high, critical) +fail_on: high + +# format: output format (console, json, diff) +format: console + +# context: lines of context to show around issues +context: 3 + +# verbose: show verbose output +verbose: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 07fc900..372ef66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ All notable changes to goperf will be documented in this file. - Output formats: `console`, `json`, `diff` - CI integration with `--fail-on` threshold -- Auto-fix suggestions with `--fix` and `--dry-run` +- Fix suggestions with `--suggest` and `--dry-run` - Ignore comments: `// perf:ignore`, `// perf:ignore-start/end` - Smart detection to reduce false positives: - Recognizes prepared statements in database loops @@ -28,7 +28,7 @@ All notable changes to goperf will be documented in this file. ### Dogfooding Results Ran goperf on itself: - **Before**: 147 issues (36 medium, 111 low) -- **Fixed**: 34 issues (slice preallocation, strings.Builder, map hints) +- **Addressed**: 34 issues (slice preallocation, strings.Builder, map hints) - **After**: 113 issues (3 medium, 110 low) The 3 remaining medium issues are intentional AST traversal patterns, and the 110 low issues are benchmark suggestions. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0afab3c..5a5934b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -71,7 +71,7 @@ goperf/ │ ├── allocation.go # Memory allocation patterns │ ├── database.go # N+1 query detection │ └── ... -├── fixer/ # Auto-fix suggestions +├── fixer/ # Fix suggestions ├── reporter/ # Output formatters (console, JSON) └── examples/ # Example problematic code ``` diff --git a/README.md b/README.md index 173e185..7d393e7 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,9 @@ goperf --rules=algorithm,database ./... # CI mode - fail on high severity issues goperf --fail-on=high --format=json ./... + +# Show fix suggestions (does not modify files) +goperf --suggest ./... ``` ## What It Detects @@ -125,16 +128,38 @@ goperf --fail-on=critical ./... ## Configuration +### Config File + +`goperf` will load `.goperf.yml` from the current directory. CLI flags override config values. + +Example: + +```yaml +rules: + - algorithm + - database +ignore_paths: + - vendor + - testdata +fail_on: high +format: console +context: 3 +verbose: false +``` + +See `.goperf.yml.example` for a fully documented template. + ### Command Line Flags | Flag | Default | Description | |------|---------|-------------| -| `--rules` | `all` | Rules to run: `algorithm,allocation,database,concurrency,io,cache` | -| `--format` | `console` | Output format: `console`, `json` | +| `--rules` | `all` | Rules to run: `algorithm,allocation,database,concurrency,io,cache,context,memory,benchmark` | +| `--format` | `console` | Output format: `console`, `json`, `diff` | | `--fail-on` | - | Exit code 1 if issues at this level: `low,medium,high,critical` | | `--context` | `3` | Lines of code context to show | | `--ignore` | - | Comma-separated paths to ignore | | `--verbose` | `false` | Show verbose output | +| `--suggest` | `false` | Show fix suggestions (does not modify files) | ## Severity Levels @@ -197,7 +222,7 @@ $ goperf ./... ╰───────────────────────────────────────────────────────────────────╯ ``` -**We fixed all 34 actionable issues:** +**We manually addressed 34 actionable issues based on suggestions:** | Issue Type | Count | Fix Applied | |------------|-------|-------------| @@ -205,7 +230,7 @@ $ goperf ./... | String concat in loops | 2 | `strings.Builder` | | Map without size hint | 1 | `make(map[K]V, size)` | -**After fixes:** +**After applying those changes:** ``` $ goperf ./... ╭───────────────────────────────────────────────────────────────────╮ @@ -217,7 +242,7 @@ The remaining 113 issues are: - **3 medium**: Nested loops for AST traversal (intentional, not O(n²) on data) - **110 low**: "Consider adding benchmarks" suggestions -This demonstrates that `goperf` finds real issues - including in itself - and that fixing them is straightforward. +This demonstrates that `goperf` finds real issues - including in itself - and that acting on suggestions is straightforward. ## Contributing @@ -229,7 +254,7 @@ Areas we'd love help with: - [ ] False positive reduction - [ ] IDE integrations (VS Code, GoLand) - [ ] Benchmark integration -- [ ] Auto-fix suggestions +- [ ] Fix suggestions Please read our [Code of Conduct](CODE_OF_CONDUCT.md) before contributing. @@ -246,3 +271,5 @@ MIT License - see [LICENSE](LICENSE) for details. Inspired by the philosophy that **preventing performance problems is better than fixing them**. Built with Go's excellent `go/ast` package for static analysis. + +Note: automatic code fixing is not yet implemented. `goperf` only produces suggestions for manual changes. diff --git a/SECURITY.md b/SECURITY.md index cd965cf..914d6c1 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -54,7 +54,7 @@ We appreciate responsible disclosure and will: goperf is designed with security in mind: -1. **Read-only by default**: Only analyzes code, doesn't modify unless `--fix` is used +1. **Read-only by default**: Only analyzes code and does not modify files (automatic fixing is not implemented) 2. **Path validation**: Refuses to operate outside the current working directory 3. **Symlink protection**: Validates symlinks don't escape the working directory 4. **Resource limits**: Caps file count, file size, and directory depth @@ -62,6 +62,6 @@ goperf is designed with security in mind: ## Best Practices for Users -1. **Review before fixing**: Always use `--dry-run` before `--fix` -2. **Trust but verify**: Review auto-fix suggestions before applying +1. **Review suggestions**: Use `--suggest` to review proposed changes +2. **Trust but verify**: Treat suggestions as guidance and apply manually 3. **Keep updated**: Use the latest version for security fixes diff --git a/config/config.go b/config/config.go new file mode 100644 index 0000000..407f883 --- /dev/null +++ b/config/config.go @@ -0,0 +1,38 @@ +package config + +import ( + "fmt" + "os" + + "gopkg.in/yaml.v3" +) + +// Config defines .goperf.yml settings. +type Config struct { + Rules []string `yaml:"rules"` + IgnorePaths []string `yaml:"ignore_paths"` + FailOn string `yaml:"fail_on"` + Format string `yaml:"format"` + Context int `yaml:"context"` + Verbose bool `yaml:"verbose"` +} + +// LoadConfig reads .goperf.yml from the current working directory. +func LoadConfig() (Config, error) { + const configFile = ".goperf.yml" + + data, err := os.ReadFile(configFile) + if err != nil { + if os.IsNotExist(err) { + return Config{}, nil + } + return Config{}, fmt.Errorf("read %s: %w", configFile, err) + } + + var cfg Config + if err := yaml.Unmarshal(data, &cfg); err != nil { + return Config{}, fmt.Errorf("parse %s: %w", configFile, err) + } + + return cfg, nil +} diff --git a/docs/RULES.md b/docs/RULES.md new file mode 100644 index 0000000..f57fb1d --- /dev/null +++ b/docs/RULES.md @@ -0,0 +1,791 @@ +# Rules + +This document describes every rule in the `rules` package, what it detects, and how to address it. Severity values reflect the rule's default behavior and may vary with heuristics. + +## Expected false positive rate +Dogfooding indicates an expected false positive rate of about 77%. Treat findings as signals that need developer judgment and context, not absolute defects. + +## nested-range +**Category:** algorithm | **Severity:** Low-Medium-High +**What it detects:** Nested `range` loops over collections, especially when inner loops iterate over the same or related collection without map lookup optimization. +**Why it's bad:** Nested iteration can be O(n^2) and becomes very slow as data grows. +**Example bad code:** +```go +for _, user := range users { + for _, order := range orders { + if order.UserID == user.ID { + process(order) + } + } +} +``` +**Example fix:** +```go +ordersByUser := make(map[int][]Order, len(orders)) +for _, order := range orders { + ordersByUser[order.UserID] = append(ordersByUser[order.UserID], order) +} +for _, user := range users { + for _, order := range ordersByUser[user.ID] { + process(order) + } +} +``` +**False positives:** When the inner loop is bounded to a tiny size, exits early, or uses an optimization pattern not recognized by the heuristic. + +## linear-search-in-loop +**Category:** algorithm | **Severity:** Medium +**What it detects:** A linear search loop nested inside another loop, using a comparison and a `break`/`return` pattern. +**Why it's bad:** O(n) lookup inside an outer loop becomes O(n*m), often replaceable with a map lookup. +**Example bad code:** +```go +for _, u := range users { + for _, id := range activeIDs { + if u.ID == id { + markActive(u) + break + } + } +} +``` +**Example fix:** +```go +active := make(map[int]struct{}, len(activeIDs)) +for _, id := range activeIDs { + active[id] = struct{}{} +} +for _, u := range users { + if _, ok := active[u.ID]; ok { + markActive(u) + } +} +``` +**False positives:** When the inner loop is not actually a search or when the collection sizes are tiny and performance is irrelevant. + +## unpreallocated-slice +**Category:** allocation | **Severity:** Low-Medium +**What it detects:** `append` to a slice inside a loop when the slice was not preallocated with capacity. +**Why it's bad:** Repeated reallocations and copies cause unnecessary CPU and memory overhead. +**Example bad code:** +```go +var out []Item +for _, in := range inputs { + out = append(out, transform(in)) +} +``` +**Example fix:** +```go +out := make([]Item, 0, len(inputs)) +for _, in := range inputs { + out = append(out, transform(in)) +} +``` +**False positives:** When the output size is unknown or very small, or if preallocation is handled in another scope that the rule does not see. + +## string-concat-loop +**Category:** allocation | **Severity:** Medium +**What it detects:** String concatenation with `+=` inside loops. +**Why it's bad:** Strings are immutable, so each `+=` copies all previous content, causing O(n^2) allocation behavior. +**Example bad code:** +```go +var s string +for _, part := range parts { + s += part +} +``` +**Example fix:** +```go +var b strings.Builder +for _, part := range parts { + b.WriteString(part) +} +result := b.String() +``` +**False positives:** When the loop is very small or the concatenated strings are tiny; also if a builder is used but not detected by the heuristic. + +## map-without-size +**Category:** allocation | **Severity:** Low +**What it detects:** `make(map[K]V)` without a size hint when the map is populated inside a loop. +**Why it's bad:** Maps rehash and grow as they fill, which can be avoided when the size is known or estimable. +**Example bad code:** +```go +m := make(map[int]User) +for _, u := range users { + m[u.ID] = u +} +``` +**Example fix:** +```go +m := make(map[int]User, len(users)) +for _, u := range users { + m[u.ID] = u +} +``` +**False positives:** When the size is truly unknown or map growth is minimal compared to other costs. + +## error-wrap-in-loop +**Category:** allocation | **Severity:** Low +**What it detects:** `errors.Wrap`, `errors.Wrapf`, or `fmt.Errorf` inside loops. +**Why it's bad:** Error wrapping/formatting allocates on every iteration, adding GC pressure in hot paths. +**Example bad code:** +```go +for _, item := range items { + if err := do(item); err != nil { + return errors.Wrap(err, "failed") + } +} +``` +**Example fix:** +```go +for _, item := range items { + if err := do(item); err != nil { + return err + } +} +``` +**False positives:** When the error path is rare or the loop is not performance-critical. + +## fmt-errorf-wrap-loop +**Category:** allocation | **Severity:** Medium +**What it detects:** `fmt.Errorf` with `%w` inside loops (error wrapping chains). +**Why it's bad:** Wrapping with `%w` allocates a chain of errors each iteration. +**Example bad code:** +```go +for _, item := range items { + if err := do(item); err != nil { + return fmt.Errorf("item %s: %w", item.ID, err) + } +} +``` +**Example fix:** +```go +if err := processItems(items); err != nil { + return fmt.Errorf("process items: %w", err) +} +``` +**False positives:** When detailed per-iteration context is required and the loop is not hot. + +## context-background-in-handler +**Category:** context | **Severity:** Medium +**What it detects:** `context.Background()` or `context.TODO()` inside HTTP handlers. +**Why it's bad:** Handler work should use `r.Context()` so it is cancelled on client disconnect. +**Example bad code:** +```go +func handler(w http.ResponseWriter, r *http.Request) { + ctx := context.Background() + _ = doWork(ctx) +} +``` +**Example fix:** +```go +func handler(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + _ = doWork(ctx) +} +``` +**False positives:** When work is intentionally detached from the request lifecycle (background jobs), and that intent is documented. + +## missing-context-timeout +**Category:** context | **Severity:** Medium +**What it detects:** Use of `http.NewRequest` (without context) for outbound calls. +**Why it's bad:** Requests without contexts cannot be cancelled or timed out, leading to resource leaks and hangs. +**Example bad code:** +```go +req, _ := http.NewRequest("GET", url, nil) +``` +**Example fix:** +```go +req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) +``` +**False positives:** When a context is intentionally not required or timeouts are applied elsewhere (e.g., transport-level timeouts). + +## context-leak +**Category:** context | **Severity:** High +**What it detects:** `context.WithCancel`, `WithTimeout`, or `WithDeadline` where the returned `cancel` function is never called. +**Why it's bad:** Unused cancel functions leak timers, goroutines, and other resources tied to the context. +**Example bad code:** +```go +ctx, cancel := context.WithTimeout(ctx, time.Second) +_ = ctx +``` +**Example fix:** +```go +ctx, cancel := context.WithTimeout(ctx, time.Second) +defer cancel() +``` +**False positives:** When cancel is invoked in another function or stored for later use, and the heuristic cannot see it. + +## missing-connection-pool-config +**Category:** database | **Severity:** Medium +**What it detects:** `sql.Open` without any pool configuration calls on the resulting `*sql.DB`. +**Why it's bad:** Default pool settings may be inappropriate, causing connection exhaustion or wasted resources. +**Example bad code:** +```go +db, _ := sql.Open("pgx", dsn) +``` +**Example fix:** +```go +db, _ := sql.Open("pgx", dsn) +db.SetMaxOpenConns(25) +db.SetMaxIdleConns(5) +db.SetConnMaxLifetime(5 * time.Minute) +``` +**False positives:** When configuration happens in another file or function that the rule does not scan. + +## unlimited-connection-pool +**Category:** database | **Severity:** High +**What it detects:** `SetMaxOpenConns(0)` which allows unlimited connections. +**Why it's bad:** Unlimited connections can overwhelm the database during traffic spikes. +**Example bad code:** +```go +db.SetMaxOpenConns(0) +``` +**Example fix:** +```go +db.SetMaxOpenConns(25) +``` +**False positives:** When running against a database with very high limits and deliberate unlimited pools, which is uncommon. + +## sql-in-loop +**Category:** database | **Severity:** Medium-High-Critical +**What it detects:** `Query`, `QueryRow`, `Exec`, `Get`, or similar SQL calls inside loops, excluding prepared statement use. +**Why it's bad:** This is the classic N+1 query pattern and causes many round trips. Writes are especially costly. +**Example bad code:** +```go +for _, id := range ids { + row := db.QueryRow("SELECT name FROM users WHERE id = ?", id) + _ = row +} +``` +**Example fix:** +```go +rows, _ := db.Query("SELECT id, name FROM users WHERE id IN (?)", ids) +_ = rows +``` +**False positives:** When the loop is small, the receiver is not actually a database object, or the query cost is negligible compared to other work. + +## indirect-sql-in-loop +**Category:** database | **Severity:** Medium-High +**What it detects:** Calling a function inside a loop when that function itself contains SQL calls. +**Why it's bad:** Hides N+1 patterns in helper functions, still creating repeated round trips. +**Example bad code:** +```go +for _, id := range ids { + user, _ := loadUser(id) // loadUser executes SQL + _ = user +} +``` +**Example fix:** +```go +users, _ := loadUsers(ids) // batch query inside +_ = users +``` +**False positives:** When the helper function only conditionally hits the database or the loop is trivially small. + +## unbatched-insert +**Category:** database | **Severity:** Medium-High +**What it detects:** ORM-style `Create`, `Insert`, or `Save` calls inside loops. +**Why it's bad:** Single-row inserts are much slower than batch inserts or bulk copy operations. +**Example bad code:** +```go +for _, row := range rows { + db.Create(&row) +} +``` +**Example fix:** +```go +db.CreateInBatches(rows, 100) +``` +**False positives:** When each insert must be independent (e.g., requires per-row transactions or side effects). + +## missing-max-bytes-reader +**Category:** io | **Severity:** Medium +**What it detects:** Reading an HTTP request body without `http.MaxBytesReader` in handlers. +**Why it's bad:** Allows unbounded request bodies, which can lead to denial-of-service via memory exhaustion. +**Example bad code:** +```go +body, _ := io.ReadAll(r.Body) +``` +**Example fix:** +```go +r.Body = http.MaxBytesReader(w, r.Body, 1<<20) +body, _ := io.ReadAll(r.Body) +``` +**False positives:** When body size is already enforced by a reverse proxy or server configuration. + +## missing-body-close +**Category:** io | **Severity:** High +**What it detects:** HTTP client response bodies that are never closed. +**Why it's bad:** Leaks connections, exhausts the HTTP client's connection pool, and can stall future requests. +**Example bad code:** +```go +resp, _ := http.Get(url) +_ = resp +``` +**Example fix:** +```go +resp, _ := http.Get(url) +if resp != nil { + defer resp.Body.Close() +} +``` +**False positives:** When the body is closed in helper functions not visible to the rule. + +## response-writer-buffering +**Category:** io | **Severity:** Low +**What it detects:** Writing to `http.ResponseWriter` in a loop without using `http.Flusher`. +**Why it's bad:** Writes may be buffered and not reach the client until the handler returns, breaking streaming responses. +**Example bad code:** +```go +for _, chunk := range chunks { + _, _ = w.Write(chunk) +} +``` +**Example fix:** +```go +if f, ok := w.(http.Flusher); ok { + for _, chunk := range chunks { + _, _ = w.Write(chunk) + f.Flush() + } +} +``` +**False positives:** When writes are small and buffering is acceptable or another flush mechanism is already in place. + +## json-in-loop +**Category:** io | **Severity:** Low-Medium +**What it detects:** `json.Marshal`/`json.Unmarshal` in loops or `json.NewEncoder().Encode()` created inside the loop. +**Why it's bad:** JSON uses reflection and allocates; repeated setup wastes CPU and memory. +**Example bad code:** +```go +for _, item := range items { + data, _ := json.Marshal(item) + _ = data +} +``` +**Example fix:** +```go +enc := json.NewEncoder(w) +for _, item := range items { + _ = enc.Encode(item) +} +``` +**False positives:** When the loop is very small or the encoder/decoder must be tied to per-iteration resources. + +## http-client-creation +**Category:** io | **Severity:** Low-Medium-High +**What it detects:** Creating `http.Client{}` inside functions, especially inside loops. +**Why it's bad:** Each client has its own connection pool, so per-request creation loses pooling benefits and adds overhead. +**Example bad code:** +```go +func fetch(url string) { + client := http.Client{} + _, _ = client.Get(url) +} +``` +**Example fix:** +```go +var httpClient = &http.Client{} + +func fetch(url string) { + _, _ = httpClient.Get(url) +} +``` +**False positives:** When distinct clients are required for different transports, or creation is intentionally rare. + +## read-all +**Category:** io | **Severity:** Low-Medium +**What it detects:** `io.ReadAll`/`ioutil.ReadAll`, especially when reading HTTP response bodies. +**Why it's bad:** Reads entire content into memory, which can cause high memory usage or OOM on large inputs. +**Example bad code:** +```go +data, _ := io.ReadAll(resp.Body) +_ = data +``` +**Example fix:** +```go +_, _ = io.Copy(dst, resp.Body) +``` +**False positives:** When content size is small, bounded, or guaranteed by protocol constraints. + +## interface-boxing-loop +**Category:** allocation | **Severity:** Low +**What it detects:** Passing concrete values to `interface{}` parameters in loops, causing boxing. +**Why it's bad:** Boxing allocates interface headers and can add GC pressure in hot loops. +**Example bad code:** +```go +for _, v := range values { + useAny(v) +} +``` +**Example fix:** +```go +for _, v := range values { + useTyped(v) +} +``` +**False positives:** When the called function truly needs `interface{}` or the loop is not performance-critical. + +## variadic-interface +**Category:** allocation | **Severity:** Low +**What it detects:** Printf-style variadic calls in loops with many complex arguments. +**Why it's bad:** Each argument is boxed to `interface{}`, and complex expressions can allocate twice. +**Example bad code:** +```go +for _, v := range values { + log.Printf("v=%v, a=%v, b=%v", v, computeA(), computeB()) +} +``` +**Example fix:** +```go +for _, v := range values { + if logEnabled { + log.Printf("v=%v", v) + } +} +``` +**False positives:** When logging is essential and the overhead is acceptable or gated by log levels. + +## type-assertion-loop +**Category:** allocation | **Severity:** Low +**What it detects:** Multiple type assertions or type switches inside a loop body. +**Why it's bad:** Repeated assertions add overhead and may indicate a hint for a type-specific path. +**Example bad code:** +```go +for _, v := range values { + if s, ok := v.(string); ok { + _ = s + } + if n, ok := v.(int); ok { + _ = n + } +} +``` +**Example fix:** +```go +typeValues := splitByType(values) +for _, s := range typeValues.strings { + _ = s +} +for _, n := range typeValues.ints { + _ = n +} +``` +**False positives:** When the loop is short or type checks are rare and necessary. + +## time-parse-in-loop +**Category:** allocation | **Severity:** Low-Medium +**What it detects:** `time.Parse` calls inside loops. +**Why it's bad:** Parsing time layouts repeatedly is relatively expensive. +**Example bad code:** +```go +for _, s := range timestamps { + _, _ = time.Parse(time.RFC3339, s) +} +``` +**Example fix:** +```go +layout := time.RFC3339 +for _, s := range timestamps { + _, _ = time.Parse(layout, s) +} +``` +**False positives:** When inputs or layouts are unique per iteration and caching provides no benefit. + +## time-location-in-loop +**Category:** allocation | **Severity:** Medium +**What it detects:** `time.LoadLocation` inside loops. +**Why it's bad:** Loading time zones reads data from disk or tzdata and is slow. +**Example bad code:** +```go +for _, name := range zones { + _, _ = time.LoadLocation(name) +} +``` +**Example fix:** +```go +loc, _ := time.LoadLocation("America/New_York") +_ = loc +``` +**False positives:** When loading different zones dynamically is required and caching is not feasible. + +## time-format-loop +**Category:** io | **Severity:** Low +**What it detects:** Multiple `time.Time.Format` calls inside a loop body. +**Why it's bad:** Formatting allocates strings; repeated calls per iteration multiply allocations. +**Example bad code:** +```go +for _, t := range times { + _ = t.Format(time.RFC3339) + _ = t.Format(time.RFC3339Nano) +} +``` +**Example fix:** +```go +for _, t := range times { + _ = t.Format(time.RFC3339) +} +``` +**False positives:** When multiple formats are truly required and the loop is small. + +## repeated-regexp-compile +**Category:** cache | **Severity:** Medium +**What it detects:** `regexp.Compile` or `regexp.MustCompile` inside functions. +**Why it's bad:** Compiling regexes is expensive and should be done once and reused. +**Example bad code:** +```go +func isValid(s string) bool { + re := regexp.MustCompile(`^[a-z]+$`) + return re.MatchString(s) +} +``` +**Example fix:** +```go +var re = regexp.MustCompile(`^[a-z]+$`) + +func isValid(s string) bool { + return re.MatchString(s) +} +``` +**False positives:** When the pattern is truly dynamic and cannot be compiled once. + +## repeated-template-parse +**Category:** cache | **Severity:** Medium +**What it detects:** `template.Parse`, `ParseFiles`, or `ParseGlob` inside functions. +**Why it's bad:** Template parsing is expensive and should be done at startup. +**Example bad code:** +```go +func render(w io.Writer, name string) { + _, _ = template.New("x").ParseFiles(name) +} +``` +**Example fix:** +```go +var templates = template.Must(template.ParseGlob("*.html")) +``` +**False positives:** When templates are intentionally dynamic or user-provided at runtime. + +## regexp-match-string-loop +**Category:** cache | **Severity:** High +**What it detects:** `regexp.MatchString` or related package-level helpers inside loops. +**Why it's bad:** These helpers compile the pattern on every call. +**Example bad code:** +```go +for _, s := range inputs { + if regexp.MatchString("^foo", s) { + use(s) + } +} +``` +**Example fix:** +```go +re := regexp.MustCompile("^foo") +for _, s := range inputs { + if re.MatchString(s) { + use(s) + } +} +``` +**False positives:** When the pattern changes per iteration or the loop size is tiny. + +## json-schema-in-loop +**Category:** cache | **Severity:** Medium +**What it detects:** JSON schema compilation or validation calls inside loops. +**Why it's bad:** Schema compilation is expensive and should be done once. +**Example bad code:** +```go +for _, doc := range docs { + _ = jsonschema.Compile(schemaText) + _ = doc +} +``` +**Example fix:** +```go +schema, _ := jsonschema.Compile(schemaText) +for _, doc := range docs { + _ = schema.Validate(doc) +} +``` +**False positives:** When schema changes per iteration and cannot be cached. + +## benchmark-suggestion +**Category:** benchmark | **Severity:** Low +**What it detects:** Functions containing performance-sensitive patterns such as loops, allocations, SQL, or reflection. +**Why it's bad:** Lacking benchmarks makes it harder to track performance regressions in hot code paths. +**Example bad code:** +```go +func Process(items []Item) { + for _, item := range items { + _ = json.Marshal(item) + } +} +``` +**Example fix:** +```go +func BenchmarkProcess(b *testing.B) { + for i := 0; i < b.N; i++ { + Process(testItems) + } +} +``` +**False positives:** When the function is not performance-critical or already covered by other benchmarks. + +## pprof-in-hot-path +**Category:** memory | **Severity:** Medium-High +**What it detects:** `pprof` calls inside loops or HTTP handlers. +**Why it's bad:** Profiling operations are expensive and add significant overhead in hot paths. +**Example bad code:** +```go +for i := 0; i < n; i++ { + pprof.StartCPUProfile(w) + pprof.StopCPUProfile() +} +``` +**Example fix:** +```go +if enableProfile { + pprof.StartCPUProfile(w) + defer pprof.StopCPUProfile() +} +``` +**False positives:** When profiling is used in debug-only or test-only paths. + +## large-struct-copy +**Category:** memory | **Severity:** Medium +**What it detects:** Passing large structs by value or copying large structs inside loops. +**Why it's bad:** Copying large structs costs CPU and memory bandwidth. +**Example bad code:** +```go +func process(s BigStruct) { + _ = s +} +``` +**Example fix:** +```go +func process(s *BigStruct) { + _ = s +} +``` +**False positives:** When value semantics are required or the struct is not actually large (size estimate heuristic). + +## escape-to-heap +**Category:** memory | **Severity:** Low +**What it detects:** Intended to flag pointer creation in loops that likely causes heap escapes, but currently no issues are emitted. +**Why it's bad:** Heap escapes increase GC pressure and reduce cache locality. +**Example bad code:** +```go +for i := 0; i < n; i++ { + x := i + _ = &x +} +``` +**Example fix:** +```go +for i := 0; i < n; i++ { + useValue(i) +} +``` +**False positives:** The current heuristic is a placeholder; if enabled, false positives are expected without type and escape analysis. + +## reflection-in-loop +**Category:** io | **Severity:** Medium +**What it detects:** Reflection calls like `reflect.ValueOf`, `reflect.TypeOf`, or `reflect.MakeSlice` inside loops. +**Why it's bad:** Reflection is slow and magnifies cost when repeated per iteration. +**Example bad code:** +```go +for _, v := range values { + _ = reflect.ValueOf(v) +} +``` +**Example fix:** +```go +for _, v := range values { + useTyped(v) +} +``` +**False positives:** When reflection results cannot be cached or the loop is tiny. + +## sync-pool-opportunity +**Category:** allocation | **Severity:** Low +**What it detects:** Repeated buffer allocations like `make([]byte, ...)` or `bytes.NewBuffer` inside loops. +**Why it's bad:** Frequent allocations increase GC pressure. `sync.Pool` can reuse buffers. +**Example bad code:** +```go +for i := 0; i < n; i++ { + buf := make([]byte, 4096) + _ = buf +} +``` +**Example fix:** +```go +var bufPool = sync.Pool{New: func() any { return make([]byte, 4096) }} +for i := 0; i < n; i++ { + buf := bufPool.Get().([]byte) + // use buf + bufPool.Put(buf) +} +``` +**False positives:** When pooling hurts performance (very small allocations or single-use buffers). + +## unbuffered-channel +**Category:** concurrency | **Severity:** Low +**What it detects:** Creating unbuffered channels (`make(chan T)` or `make(chan T, 0)`) that are not used in select or common signal patterns. +**Why it's bad:** Unbuffered channels block senders, which can cause deadlocks if not coordinated carefully. +**Example bad code:** +```go +ch := make(chan int) +``` +**Example fix:** +```go +ch := make(chan int, 1) +``` +**False positives:** When the channel is intentionally unbuffered for synchronization or used in patterns the heuristic does not recognize. + +## mutex-in-loop +**Category:** concurrency | **Severity:** Low-Medium +**What it detects:** `Lock` or `RLock` calls inside loops. +**Why it's bad:** Repeated locking adds overhead and can cause contention. +**Example bad code:** +```go +for _, v := range values { + mu.Lock() + store[v.ID] = v + mu.Unlock() +} +``` +**Example fix:** +```go +mu.Lock() +for _, v := range values { + store[v.ID] = v +} +mu.Unlock() +``` +**False positives:** When each iteration must be isolated for correctness or the loop is very small. + +## goroutine-leak +**Category:** concurrency | **Severity:** High +**What it detects:** Goroutines running infinite loops without a cancellation mechanism (context or done channel). +**Why it's bad:** Goroutines that never terminate leak memory and CPU. +**Example bad code:** +```go +go func() { + for { + work() + } +}() +``` +**Example fix:** +```go +go func(ctx context.Context) { + for { + select { + case <-ctx.Done(): + return + default: + work() + } + } +}(ctx) +``` +**False positives:** When termination is handled by another mechanism not visible to the heuristic. diff --git a/fixer/fixer.go b/fixer/fixer.go index 0ef2cb1..fbd54d1 100644 --- a/fixer/fixer.go +++ b/fixer/fixer.go @@ -13,7 +13,7 @@ import ( "github.com/unsaid-dev/goperf/rules" ) -// Fix represents an auto-fixable change +// Fix represents a suggested change type Fix struct { File string Line int @@ -23,7 +23,7 @@ type Fix struct { Applied bool } -// Fixer handles automatic code fixes +// Fixer handles fix suggestions type Fixer struct { DryRun bool Verbose bool @@ -80,7 +80,7 @@ func validatePathForWrite(filename string) error { return nil } -// FixIssues attempts to fix the given issues +// FixIssues generates fix suggestions for the given issues func (f *Fixer) FixIssues(issues []rules.Issue) []Fix { // Preallocate: assume ~1 fix per issue fixes := make([]Fix, 0, len(issues)) @@ -130,10 +130,10 @@ func (f *Fixer) fixFile(filename string, issues []rules.Issue) []Fix { } } - // NOTE: Auto-fix currently only generates suggestions. - // Actual AST modification and file writing is not implemented + // NOTE: Suggestions are output only. + // Automatic AST modification and file writing is not implemented // because safe AST modification is complex and error-prone. - // The fixes are displayed as suggestions for manual application. + // Suggestions are displayed for manual application. return fixes } @@ -154,7 +154,7 @@ func (f *Fixer) attemptFix(issue rules.Issue, file *ast.File, fset *token.FileSe File: issue.File, Line: issue.Line, Original: getLine(lines, issue.Line), - Fixed: "", // No auto-fix available + Fixed: "", // No suggestion available Rule: issue.Rule, Applied: false, } @@ -295,18 +295,18 @@ func getLine(lines []string, lineNum int) string { return "" } -// PrintFixes displays the fixes in a readable format +// PrintFixes displays the suggestions in a readable format func PrintFixes(fixes []Fix, dryRun bool) { if len(fixes) == 0 { - fmt.Println("No auto-fixes available for the detected issues.") + fmt.Println("No fix suggestions available for the detected issues.") return } if dryRun { - fmt.Println("=== DRY RUN: Suggested fixes (no files modified) ===") + fmt.Println("=== DRY RUN: Fix suggestions (no files modified) ===") fmt.Println() } else { - fmt.Println("=== Suggested fixes ===") + fmt.Println("=== Fix suggestions ===") fmt.Println() } @@ -317,9 +317,9 @@ func PrintFixes(fixes []Fix, dryRun bool) { fmt.Printf("Original: %s\n", strings.TrimSpace(fix.Original)) } if fix.Fixed != "" { - fmt.Printf("Fix: %s\n", fix.Fixed) + fmt.Printf("Suggestion: %s\n", fix.Fixed) } else { - fmt.Println("Fix: Manual intervention required - see issue suggestion") + fmt.Println("Suggestion: Manual intervention required - see issue detail") } fmt.Println() } diff --git a/go.mod b/go.mod index 52ebe23..4d9bf85 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,5 @@ module github.com/unsaid-dev/goperf go 1.21 + +require gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..a62c313 --- /dev/null +++ b/go.sum @@ -0,0 +1,4 @@ +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/main.go b/main.go index 396ded5..90efe6c 100644 --- a/main.go +++ b/main.go @@ -8,23 +8,12 @@ import ( "path/filepath" "strings" + "github.com/unsaid-dev/goperf/config" "github.com/unsaid-dev/goperf/fixer" "github.com/unsaid-dev/goperf/reporter" "github.com/unsaid-dev/goperf/rules" ) -var ( - rulesFlag = flag.String("rules", "all", "Comma-separated rules to run: algorithm,allocation,database,concurrency,io,cache,context,memory,benchmark,all") - formatFlag = flag.String("format", "console", "Output format: console, json, diff") - failOnFlag = flag.String("fail-on", "", "Exit with code 1 if issues at this level or higher: low, medium, high, critical") - contextFlag = flag.Int("context", 3, "Lines of context to show around issues") - ignoreFlag = flag.String("ignore", "", "Comma-separated paths to ignore") - verboseFlag = flag.Bool("verbose", false, "Show verbose output") - versionFlag = flag.Bool("version", false, "Show version") - fixFlag = flag.Bool("fix", false, "Automatically fix issues where possible") - dryRunFlag = flag.Bool("dry-run", false, "Show fixes without applying them (use with --fix)") -) - // Resource limits to prevent DoS const ( MaxFilesPerScan = 10000 @@ -35,6 +24,46 @@ const ( var version = "0.1.0" func main() { + cfg, err := config.LoadConfig() + if err != nil { + fmt.Fprintf(os.Stderr, "Error loading config: %v\n", err) + os.Exit(1) + } + + rulesDefault := "all" + if len(cfg.Rules) > 0 { + rulesDefault = strings.Join(cfg.Rules, ",") + } + + ignoreDefault := "" + if len(cfg.IgnorePaths) > 0 { + ignoreDefault = strings.Join(cfg.IgnorePaths, ",") + } + + formatDefault := "console" + if cfg.Format != "" { + formatDefault = cfg.Format + } + + failOnDefault := cfg.FailOn + + contextDefault := 3 + if cfg.Context != 0 { + contextDefault = cfg.Context + } + + verboseDefault := cfg.Verbose + + rulesFlag := flag.String("rules", rulesDefault, "Comma-separated rules to run: algorithm,allocation,database,concurrency,io,cache,context,memory,benchmark,all") + formatFlag := flag.String("format", formatDefault, "Output format: console, json, diff") + failOnFlag := flag.String("fail-on", failOnDefault, "Exit with code 1 if issues at this level or higher: low, medium, high, critical") + contextFlag := flag.Int("context", contextDefault, "Lines of context to show around issues") + ignoreFlag := flag.String("ignore", ignoreDefault, "Comma-separated paths to ignore") + verboseFlag := flag.Bool("verbose", verboseDefault, "Show verbose output") + versionFlag := flag.Bool("version", false, "Show version") + suggestFlag := flag.Bool("suggest", false, "Show fix suggestions (does not modify files)") + dryRunFlag := flag.Bool("dry-run", false, "Show fix suggestions with a dry-run banner") + flag.Usage = func() { fmt.Fprintf(os.Stderr, `goperf - Performance Pattern Detector for Go @@ -48,8 +77,8 @@ Examples: goperf --rules=algorithm ./internal/ # Only algorithm rules goperf --format=json ./... # JSON output for CI goperf --fail-on=high ./... # Exit 1 if high+ issues - goperf --fix --dry-run ./... # Preview auto-fixes - goperf --fix ./... # Apply auto-fixes + goperf --suggest ./... # Show fix suggestions + goperf --suggest --format=diff ./... # Show suggestions as a diff Flags: `) @@ -72,8 +101,8 @@ Severity Levels: medium - Moderate impact, should fix low - Minor optimization opportunity -Auto-Fix Support: - The following rules support auto-fix: +Fix Suggestion Support: + The following rules support suggestions (no files are modified): - string-concat-loop → strings.Builder suggestion - unpreallocated-slice → make() with capacity - missing-body-close → defer Body.Close() @@ -125,10 +154,10 @@ Auto-Fix Support: } // Run analysis - issues := analyzer.Analyze(files) + issues, _ := analyzer.Analyze(files) - // Handle fix mode - if *fixFlag { + // Handle suggestion mode + if *suggestFlag { f := fixer.NewFixer(*dryRunFlag, *verboseFlag) fixes := f.FixIssues(issues) @@ -147,7 +176,7 @@ Auto-Fix Support: fixable++ } } - fmt.Printf("Auto-fixable: %d\n", fixable) + fmt.Printf("Suggestions with replacements: %d\n", fixable) } return } @@ -158,7 +187,7 @@ Auto-Fix Support: case "json": rep = &reporter.JSONReporter{} case "diff": - // Generate diff output even without --fix + // Generate diff output even without --suggest f := fixer.NewFixer(true, *verboseFlag) fixes := f.FixIssues(issues) fmt.Println(fixer.GenerateDiff(fixes)) diff --git a/rules/analyzer.go b/rules/analyzer.go index 1794215..a206b9f 100644 --- a/rules/analyzer.go +++ b/rules/analyzer.go @@ -1,17 +1,70 @@ package rules import ( + "context" + "fmt" "go/ast" "go/parser" "go/token" "os" "strings" + "time" +) + +const ( + ParseTimeout = 5 * time.Second + MaxASTNodes = 100000 + MaxASTDepth = 1000 ) // Analyzer runs rules against Go source files type Analyzer struct { config AnalyzerConfig rules []Rule + Errors []string +} + +type parseResult struct { + file *ast.File + err error + panicErr error +} + +// ASTComplexityValidator enforces node count and depth limits during AST walks. +type ASTComplexityValidator struct { + maxNodes int + maxDepth int + nodes int + depth int +} + +func (v *ASTComplexityValidator) Visit(n ast.Node) ast.Visitor { + if n == nil { + if v.depth > 0 { + v.depth-- + } + return v + } + + v.nodes++ + v.depth++ + + if v.nodes > v.maxNodes { + panic("ast node limit exceeded") + } + if v.depth > v.maxDepth { + panic("ast depth limit exceeded") + } + + return v +} + +func ValidateASTComplexity(file *ast.File) { + validator := &ASTComplexityValidator{ + maxNodes: MaxASTNodes, + maxDepth: MaxASTDepth, + } + ast.Walk(validator, file) } // NewAnalyzer creates a new analyzer with the given config @@ -34,9 +87,10 @@ func NewAnalyzer(config AnalyzerConfig) *Analyzer { } // Analyze runs all rules against the given files -func (a *Analyzer) Analyze(files []string) []Issue { +func (a *Analyzer) Analyze(files []string) ([]Issue, []string) { // Preallocate with estimate: ~2 issues per file on average allIssues := make([]Issue, 0, len(files)*2) + a.Errors = make([]string, 0, len(files)) fset := token.NewFileSet() @@ -56,13 +110,44 @@ func (a *Analyzer) Analyze(files []string) []Issue { // Read source src, err := os.ReadFile(filename) if err != nil { + a.recordError("read", filename, err) continue } - // Parse file - file, err := parser.ParseFile(fset, filename, src, parser.ParseComments) - if err != nil { + // Parse file with a timeout and AST complexity enforcement + parseCtx, cancel := context.WithTimeout(context.Background(), ParseTimeout) + resultCh := make(chan parseResult, 1) + go func() { + defer func() { + if r := recover(); r != nil { + resultCh <- parseResult{panicErr: fmt.Errorf("parser panic: %v", r)} + } + }() + + file, err := parser.ParseFile(fset, filename, src, parser.ParseComments) + if err == nil { + ValidateASTComplexity(file) + } + resultCh <- parseResult{file: file, err: err} + }() + + var file *ast.File + select { + case <-parseCtx.Done(): + cancel() + a.recordError("parse", filename, parseCtx.Err()) continue + case result := <-resultCh: + cancel() + if result.panicErr != nil || result.err != nil { + if result.panicErr != nil { + a.recordError("parse", filename, result.panicErr) + } else { + a.recordError("parse", filename, result.err) + } + continue + } + file = result.file } // Parse ignore comments @@ -89,7 +174,18 @@ func (a *Analyzer) Analyze(files []string) []Issue { } } - return allIssues + if len(a.Errors) > 0 { + fmt.Fprintf(os.Stderr, "Warning: %d files could not be analyzed\n", len(a.Errors)) + } + + return allIssues, a.Errors +} + +func (a *Analyzer) recordError(action, filename string, err error) { + a.Errors = append(a.Errors, fmt.Sprintf("%s: %v", filename, err)) + if a.config.Verbose { + fmt.Fprintf(os.Stderr, "Warning: failed to %s %s: %v\n", action, filename, err) + } } // Helper functions for rule implementations @@ -224,14 +320,14 @@ func FindSQLInLoop(file *ast.File, fset *token.FileSet) []SQLInLoopInfo { results := make([]SQLInLoopInfo, 0, 4) sqlMethods := map[string]bool{ - "Query": true, - "QueryRow": true, - "Exec": true, + "Query": true, + "QueryRow": true, + "Exec": true, "QueryRowContext": true, "QueryContext": true, "ExecContext": true, - "Get": true, - "Select": true, + "Get": true, + "Select": true, } ast.Inspect(file, func(n ast.Node) bool { diff --git a/rules/analyzer_test.go b/rules/analyzer_test.go new file mode 100644 index 0000000..2bd74fc --- /dev/null +++ b/rules/analyzer_test.go @@ -0,0 +1,211 @@ +package rules + +import ( + "go/parser" + "go/token" + "path/filepath" + "testing" +) + +func testdataPath(t *testing.T, name string) string { + t.Helper() + return filepath.Join("..", "testdata", name) +} + +func TestAnalyzerBasics(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + }) + + file := testdataPath(t, "analyzer_basic.go") + issues, errs := analyzer.Analyze([]string{file}) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if len(issues) != 2 { + t.Fatalf("got %d issues, want 2", len(issues)) + } + + seen := make(map[string]bool) + for _, issue := range issues { + if issue.File != file { + t.Fatalf("issue file = %q, want %q", issue.File, file) + } + seen[issue.Rule] = true + } + + if !seen["unpreallocated-slice"] { + t.Errorf("missing rule: unpreallocated-slice") + } + if !seen["string-concat-loop"] { + t.Errorf("missing rule: string-concat-loop") + } +} + +func TestAnalyzerIgnorePaths(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + IgnorePaths: []string{"analyzer_ignored.go"}, + }) + + ignored := testdataPath(t, "analyzer_ignored.go") + scanned := testdataPath(t, "analyzer_scanned.go") + issues, errs := analyzer.Analyze([]string{ignored, scanned}) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if len(issues) != 1 { + t.Fatalf("got %d issues, want 1", len(issues)) + } + if issues[0].File != scanned { + t.Fatalf("issue file = %q, want %q", issues[0].File, scanned) + } +} + +func TestAnalyzerEmptyFile(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + }) + + file := testdataPath(t, "analyzer_empty.go") + issues, errs := analyzer.Analyze([]string{file}) + if len(errs) != 1 { + t.Fatalf("got %d errors, want 1", len(errs)) + } + if len(issues) != 0 { + t.Fatalf("got %d issues, want 0", len(issues)) + } +} + +func TestAnalyzerSyntaxError(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + }) + + bad := testdataPath(t, "analyzer_syntax_error.go") + good := testdataPath(t, "analyzer_basic.go") + issues, errs := analyzer.Analyze([]string{bad, good}) + if len(errs) != 1 { + t.Fatalf("got %d errors, want 1", len(errs)) + } + if len(issues) != 2 { + t.Fatalf("got %d issues, want 2", len(issues)) + } + for _, issue := range issues { + if issue.File == bad { + t.Fatalf("unexpected issue from syntax error file: %q", issue.Rule) + } + } +} + +func TestAnalyzerContextExtraction(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + Context: 1, + }) + + file := testdataPath(t, "analyzer_context.go") + issues, errs := analyzer.Analyze([]string{file}) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if len(issues) != 1 { + t.Fatalf("got %d issues, want 1", len(issues)) + } + + want := []string{ + "\tfor _, item := range items {", + "\t\tout = append(out, item)", + "\t}", + } + + if len(issues[0].Context) != len(want) { + t.Fatalf("context lines = %d, want %d", len(issues[0].Context), len(want)) + } + for i := range want { + if issues[0].Context[i] != want[i] { + t.Fatalf("context[%d] = %q, want %q", i, issues[0].Context[i], want[i]) + } + } +} + +func TestAnalyzerMultipleFiles(t *testing.T) { + analyzer := NewAnalyzer(AnalyzerConfig{ + Rules: []string{"allocation"}, + }) + + fileA := testdataPath(t, "analyzer_multi_a.go") + fileB := testdataPath(t, "analyzer_multi_b.go") + issues, errs := analyzer.Analyze([]string{fileA, fileB}) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if len(issues) != 2 { + t.Fatalf("got %d issues, want 2", len(issues)) + } + + seen := map[string]bool{ + fileA: false, + fileB: false, + } + for _, issue := range issues { + if _, ok := seen[issue.File]; ok { + seen[issue.File] = true + } + } + for file, ok := range seen { + if !ok { + t.Fatalf("missing issue from file: %q", file) + } + } +} + +func TestAnalyzerHelpers(t *testing.T) { + src := `package main + +func nested(items [][]int) { + for _, outer := range items { + for _, inner := range outer { + _ = inner + } + } +} + +func appendInLoop(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} + +func concatLoop(items []string) string { + var s string + for _, item := range items { + s += item + "!" + } + return s +} +` + + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "helpers.go", src, 0) + if err != nil { + t.Fatalf("failed to parse helpers src: %v", err) + } + + nested := FindNestedRangeLoops(file, fset) + if len(nested) != 1 { + t.Fatalf("nested loops = %d, want 1", len(nested)) + } + + appends := FindAppendInLoop(file, fset) + if len(appends) != 1 { + t.Fatalf("append-in-loop = %d, want 1", len(appends)) + } + + concat := FindStringConcatInLoop(file, fset) + if len(concat) != 1 { + t.Fatalf("string concat in loop = %d, want 1", len(concat)) + } +} diff --git a/rules/types.go b/rules/types.go index 214a460..f54e0e6 100644 --- a/rules/types.go +++ b/rules/types.go @@ -11,7 +11,7 @@ // Rules: []string{"algorithm", "database"}, // Context: 3, // }) -// issues := analyzer.Analyze("./...") +// issues, _ := analyzer.Analyze("./...") package rules import ( diff --git a/testdata/analyzer_basic.go b/testdata/analyzer_basic.go new file mode 100644 index 0000000..65551e0 --- /dev/null +++ b/testdata/analyzer_basic.go @@ -0,0 +1,17 @@ +package main + +func basic(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} + +func concat(items []string) string { + s := "" + for _, item := range items { + s += item + "!" + } + return s +} diff --git a/testdata/analyzer_context.go b/testdata/analyzer_context.go new file mode 100644 index 0000000..34a1ea3 --- /dev/null +++ b/testdata/analyzer_context.go @@ -0,0 +1,9 @@ +package main + +func contextExample(items []int) []int { + out := []int{} + for _, item := range items { + out = append(out, item) + } + return out +} diff --git a/testdata/analyzer_empty.go b/testdata/analyzer_empty.go new file mode 100644 index 0000000..e69de29 diff --git a/testdata/analyzer_ignored.go b/testdata/analyzer_ignored.go new file mode 100644 index 0000000..16b1071 --- /dev/null +++ b/testdata/analyzer_ignored.go @@ -0,0 +1,9 @@ +package main + +func ignored(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} diff --git a/testdata/analyzer_multi_a.go b/testdata/analyzer_multi_a.go new file mode 100644 index 0000000..6480fef --- /dev/null +++ b/testdata/analyzer_multi_a.go @@ -0,0 +1,9 @@ +package main + +func multiA(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} diff --git a/testdata/analyzer_multi_b.go b/testdata/analyzer_multi_b.go new file mode 100644 index 0000000..be4cafc --- /dev/null +++ b/testdata/analyzer_multi_b.go @@ -0,0 +1,9 @@ +package main + +func multiB(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} diff --git a/testdata/analyzer_scanned.go b/testdata/analyzer_scanned.go new file mode 100644 index 0000000..f3b6a0d --- /dev/null +++ b/testdata/analyzer_scanned.go @@ -0,0 +1,9 @@ +package main + +func scanned(items []int) []int { + var out []int + for _, item := range items { + out = append(out, item) + } + return out +} diff --git a/testdata/analyzer_syntax_error.go b/testdata/analyzer_syntax_error.go new file mode 100644 index 0000000..df91ce9 --- /dev/null +++ b/testdata/analyzer_syntax_error.go @@ -0,0 +1,6 @@ +package main + +func broken() { + if true { + println("oops") +}