diff --git a/plugins/beagle-go/skills/go-code-review/SKILL.md b/plugins/beagle-go/skills/go-code-review/SKILL.md index 5dfc744..b0eab5e 100644 --- a/plugins/beagle-go/skills/go-code-review/SKILL.md +++ b/plugins/beagle-go/skills/go-code-review/SKILL.md @@ -1,71 +1,124 @@ --- name: go-code-review -description: Reviews Go code for idiomatic patterns, error handling, concurrency safety, and common mistakes. Use when reviewing .go files, checking error handling, goroutine usage, or interface design. +description: Reviews Go code for idiomatic patterns, error handling, concurrency safety, and common mistakes. Use when reviewing .go files, checking error handling, goroutine usage, or interface design. Covers Go 1.20+ features including errors.Join, slog, generics, and Go 1.22 loop variable semantics. --- # Go Code Review +## Review Workflow + +Follow this sequence to avoid false positives and catch version-specific issues: + +1. **Check `go.mod`** — Note the Go version. This determines which patterns apply (loop variable capture is only an issue pre-1.22, `slog` is available from 1.21, `errors.Join` from 1.20). Skip version-gated checks that don't apply. +2. **Scan changed files** — Read full functions, not just diffs. Many Go bugs hide in what surrounds the change. +3. **Check each category** — Work through the checklist below, loading references as needed. +4. **Verify before reporting** — Load [review-verification-protocol](../review-verification-protocol/SKILL.md) before submitting findings. + +## Output Format + +Report findings as: + +``` +[FILE:LINE] Issue title +Severity: Critical | Major | Minor | Informational +Description of the issue and why it matters. +``` + ## Quick Reference | Issue Type | Reference | |------------|-----------| -| Missing error checks, wrapped errors | [references/error-handling.md](references/error-handling.md) | -| Race conditions, channel misuse | [references/concurrency.md](references/concurrency.md) | -| Interface pollution, naming | [references/interfaces.md](references/interfaces.md) | -| Resource leaks, defer misuse | [references/common-mistakes.md](references/common-mistakes.md) | +| Missing error checks, wrapping, errors.Join | [references/error-handling.md](references/error-handling.md) | +| Race conditions, channel misuse, goroutine lifecycle | [references/concurrency.md](references/concurrency.md) | +| Interface pollution, naming, generics | [references/interfaces.md](references/interfaces.md) | +| Resource leaks, defer misuse, slog, naming | [references/common-mistakes.md](references/common-mistakes.md) | ## Review Checklist -- [ ] All errors are checked (no `_ = err`) +### Error Handling +- [ ] All errors checked (no `_ = err` without justifying comment) - [ ] Errors wrapped with context (`fmt.Errorf("...: %w", err)`) -- [ ] Resources closed with `defer` immediately after creation -- [ ] No goroutine leaks (channels closed, contexts canceled) +- [ ] `errors.Is`/`errors.As` used instead of string matching +- [ ] `errors.Join` used for aggregating multiple errors (Go 1.20+) +- [ ] Zero values returned alongside errors + +### Concurrency +- [ ] No goroutine leaks (context cancellation or shutdown signal exists) +- [ ] Channels closed by sender only, exactly once +- [ ] Shared state protected by mutex or sync types +- [ ] WaitGroups used to wait for goroutine completion +- [ ] Context propagated through call chain +- [ ] Loop variable capture handled (pre-Go 1.22 codebases only) + +### Interfaces and Types - [ ] Interfaces defined by consumers, not producers -- [ ] Interface names end in `-er` (Reader, Writer, Handler) +- [ ] Interface names follow `-er` convention +- [ ] Interfaces minimal (1-3 methods) +- [ ] Concrete types returned from constructors +- [ ] `any` preferred over `interface{}` (Go 1.18+) +- [ ] Generics used where appropriate instead of `any` or code generation + +### Resources and Lifecycle +- [ ] Resources closed with `defer` immediately after creation +- [ ] HTTP response bodies always closed +- [ ] No `defer` in loops without closure wrapping +- [ ] `init()` functions avoided in favor of explicit initialization + +### Naming and Style - [ ] Exported names have doc comments +- [ ] No stuttering names (`user.UserService` → `user.Service`) - [ ] No naked returns in functions > 5 lines - [ ] Context passed as first parameter -- [ ] Mutexes protect shared state, not methods +- [ ] `slog` used over `log` for structured logging (Go 1.21+) -## When to Load References +## Severity Calibration -- Reviewing error return patterns → error-handling.md -- Reviewing goroutines/channels → concurrency.md -- Reviewing type definitions → interfaces.md -- General Go review → common-mistakes.md +### Critical (Block Merge) +- Unchecked errors on I/O, network, or database operations +- Goroutine leaks (no shutdown path) +- Race conditions on shared state (concurrent map access without sync) +- Unbounded resource accumulation (defer in loop, unclosed connections) + +### Major (Should Fix) +- Errors returned without context (bare `return err`) +- Missing WaitGroup for spawned goroutines +- `panic` for recoverable errors +- Context not propagated to downstream calls -## Review Questions +### Minor (Consider Fixing) +- `interface{}` instead of `any` in Go 1.18+ codebases +- Missing doc comments on exports +- Stuttering names +- Slice not preallocated when size is known -1. Are all error returns checked and wrapped? -2. Are goroutines properly managed with context cancellation? -3. Are resources (files, connections) closed with defer? -4. Are interfaces minimal and defined where used? +### Informational (Note Only) +- Suggestions to add generics where code generation exists +- Refactoring ideas for interface design +- Performance optimizations without measured impact + +## When to Load References + +- Reviewing error return patterns → error-handling.md +- Reviewing goroutines, channels, or sync types → concurrency.md +- Reviewing type definitions, interfaces, or generics → interfaces.md +- General review (resources, naming, init, performance) → common-mistakes.md ## Valid Patterns (Do NOT Flag) -These patterns are acceptable and should NOT be flagged as issues: - -- **`_ = err` with reason comment** - Intentionally ignored errors with explanation - ```go - _ = conn.Close() // Best effort cleanup, already handling primary error - ``` -- **Empty interface `interface{}`** - For truly generic code (pre-generics codebases) -- **Naked returns in short functions** - Acceptable in functions < 5 lines with named returns -- **Channel without close** - When consumer stops via context cancellation, not channel close -- **Mutex protecting struct fields** - Even if accessed only via methods, this is correct encapsulation -- **`//nolint` directives with reason** - Acceptable when accompanied by explanation - ```go - //nolint:errcheck // Error logged but not returned per API contract - ``` -- **Defer in loop** - When function scope cleanup is intentional (e.g., processing files in batches) -- **Functional options pattern** - `type Option func(*T)` with `With*` constructors is idiomatic for configurable types - ```go - func NewServer(addr string, opts ...Option) *Server { ... } - ``` -- **`sync.Pool` for hot paths** - Acceptable for reducing allocation pressure in performance-critical code - ```go - var bufPool = sync.Pool{New: func() any { return new(bytes.Buffer) }} - ``` +These are acceptable Go patterns — reporting them wastes developer time: + +- **`_ = err` with reason comment** — Intentionally ignored errors with explanation +- **Empty interface / `any`** — For truly generic code or interop with untyped APIs +- **Naked returns in short functions** — Acceptable in functions < 5 lines with named returns +- **Channel without close** — When consumer stops via context cancellation, not channel close +- **Mutex protecting struct fields** — Even if accessed only via methods, this is correct encapsulation +- **`//nolint` directives with reason** — Acceptable when accompanied by explanation +- **Defer in loop** — When function scope cleanup is intentional (e.g., processing files in batches) +- **Functional options pattern** — `type Option func(*T)` with `With*` constructors is idiomatic +- **`sync.Pool` for hot paths** — Acceptable for reducing allocation pressure in performance-critical code +- **`context.Background()` in main/tests** — Valid root context for top-level calls +- **`select` with `default`** — Non-blocking channel operation, intentional pattern +- **Short variable names in small scope** — `i`, `err`, `ctx`, `ok` are idiomatic Go ## Context-Sensitive Rules @@ -77,6 +130,8 @@ Only flag these issues when the specific conditions apply: | Goroutine leak | No context cancellation path exists for the goroutine | | Missing defer | Resource isn't explicitly closed before next acquisition or return | | Interface pollution | Interface has > 1 method AND only one consumer exists | +| Loop variable capture | `go.mod` specifies Go < 1.22 | +| Missing slog | `go.mod` specifies Go >= 1.21 AND code uses `log` package for structured output | ## Before Submitting Findings diff --git a/plugins/beagle-go/skills/go-code-review/references/common-mistakes.md b/plugins/beagle-go/skills/go-code-review/references/common-mistakes.md index 9870a6f..88125d0 100644 --- a/plugins/beagle-go/skills/go-code-review/references/common-mistakes.md +++ b/plugins/beagle-go/skills/go-code-review/references/common-mistakes.md @@ -4,7 +4,7 @@ ### 1. Missing defer for Close -**Problem**: Resources leaked on early return. +Resources leaked on early return. The `defer` should come immediately after the error check for the open/create call. ```go // BAD @@ -34,17 +34,17 @@ func readFile(path string) ([]byte, error) { ### 2. Defer in Loop -**Problem**: Resources accumulate until function returns. +`defer` runs at function exit, not loop iteration exit. In a loop, resources accumulate until the function returns. ```go -// BAD - files stay open until loop ends +// BAD - files stay open until function returns for _, path := range paths { f, _ := os.Open(path) - defer f.Close() // deferred until function returns + defer f.Close() process(f) } -// GOOD - close in each iteration or use closure +// GOOD - wrap in closure for per-iteration cleanup for _, path := range paths { func() { f, _ := os.Open(path) @@ -56,7 +56,7 @@ for _, path := range paths { ### 3. HTTP Response Body Not Closed -**Problem**: Connection pool exhaustion. +Every `http.Client` call that returns a non-nil response has a body that must be closed, even if you don't read it. Failing to close it leaks the underlying TCP connection. ```go // BAD @@ -64,7 +64,6 @@ resp, err := http.Get(url) if err != nil { return err } -// body never closed! data, _ := io.ReadAll(resp.Body) // GOOD @@ -80,7 +79,7 @@ data, _ := io.ReadAll(resp.Body) ### 4. Stuttering Names -**Problem**: Redundant when used with package name. +Package names are part of the identifier at the call site. Repeating the package name in the type or function name creates redundancy. ```go // BAD @@ -94,7 +93,7 @@ type Service struct { ... } // user.Service ### 5. Missing Doc Comments on Exports -**Problem**: godoc can't generate documentation. +Exported names without doc comments can't be documented by `godoc`/`pkgsite`. The comment should start with the name being documented. ```go // BAD @@ -107,20 +106,18 @@ func NewServer(addr string) *Server { ... } ### 6. Naked Returns in Long Functions -**Problem**: Hard to track what's being returned. +Named returns are convenient in short functions, but in longer functions they obscure what's being returned. The threshold is roughly 5 lines — beyond that, be explicit. ```go // BAD func process(data []byte) (result string, err error) { // 50 lines of code... - return // what's being returned? } // GOOD - explicit returns func process(data []byte) (string, error) { // 50 lines of code... - return processedString, nil } ``` @@ -129,7 +126,7 @@ func process(data []byte) (string, error) { ### 7. Init Function Overuse -**Problem**: Hidden side effects, hard to test. +`init()` functions run before `main()`, create hidden dependencies, make testing harder, and can cause subtle ordering issues when multiple packages have init functions. ```go // BAD - global state via init @@ -159,7 +156,7 @@ func NewApp(dbURL string) (*App, error) { ### 8. Global Mutable State -**Problem**: Race conditions, hard to test. +Package-level mutable variables create race conditions in concurrent code and make testing unreliable because tests share state. ```go // BAD @@ -179,11 +176,41 @@ func NewServer(cfg Config) *Server { } ``` +## Structured Logging (Go 1.21+) + +### 9. Using `log` Instead of `slog` + +The `log/slog` package (Go 1.21+) provides structured, leveled logging that's far more useful in production than unstructured `log.Println` output. + +```go +// OLD - unstructured, hard to parse +log.Printf("failed to load user %d: %v", userID, err) + +// MODERN - structured, machine-parseable +slog.Error("failed to load user", + "user_id", userID, + "error", err, +) + +// With logger groups and attributes +logger := slog.With("service", "auth") +logger.Info("user logged in", + "user_id", userID, + "ip", req.RemoteAddr, +) +``` + +Key `slog` patterns: +- Use `slog.With()` to add common attributes to a logger +- Pass `*slog.Logger` as a dependency, don't use the global default in libraries +- Implement `slog.LogValuer` for custom types that appear frequently in logs +- Use `slog.Group()` to namespace related attributes + ## Performance -### 9. String Concatenation in Loop +### 10. String Concatenation in Loop -**Problem**: O(n²) allocation overhead. +String concatenation with `+` in a loop creates a new string allocation on every iteration, resulting in O(n^2) memory usage. ```go // BAD @@ -201,9 +228,9 @@ for _, s := range items { result := b.String() ``` -### 10. Slice Preallocation +### 11. Slice Preallocation -**Problem**: Repeated reallocations. +When you know the final size, preallocate to avoid repeated backing array copies as the slice grows. ```go // BAD - grows dynamically @@ -219,46 +246,27 @@ for _, item := range items { } ``` -## Testing - -### 11. Table-Driven Tests Missing +### 12. Range Over Integer (Go 1.22+) -**Problem**: Verbose, repetitive test code. +Go 1.22 added `range` over integers, replacing the classic C-style for loop for simple counting: ```go -// BAD -func TestAdd(t *testing.T) { - if Add(1, 2) != 3 { - t.Error("1+2 should be 3") - } - if Add(0, 0) != 0 { - t.Error("0+0 should be 0") - } +// OLD +for i := 0; i < n; i++ { + process(i) } -// GOOD -func TestAdd(t *testing.T) { - tests := []struct { - a, b, want int - }{ - {1, 2, 3}, - {0, 0, 0}, - {-1, 1, 0}, - } - for _, tt := range tests { - got := Add(tt.a, tt.b) - if got != tt.want { - t.Errorf("Add(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) - } - } +// MODERN (Go 1.22+) +for i := range n { + process(i) } ``` ## Sync and Performance -### 12. sync.Pool Misuse +### 13. sync.Pool Misuse -**Problem**: Using sync.Pool for objects that shouldn't be pooled, or not resetting objects. +Objects returned to a `sync.Pool` must be reset first, otherwise the next consumer gets stale data. ```go // BAD - not resetting before Put @@ -275,9 +283,9 @@ defer func() { buf.WriteString("data") ``` -### 13. Missing Functional Options +### 14. Functional Options -**Problem**: Constructor with many parameters, hard to extend. +Constructors with many parameters are hard to read and painful to extend. The functional options pattern provides a clean API with sensible defaults. ```go // BAD - parameter bloat @@ -291,7 +299,7 @@ func WithTimeout(d time.Duration) Option { } func NewServer(addr string, opts ...Option) *Server { - s := &Server{addr: addr, timeout: 30 * time.Second} // defaults + s := &Server{addr: addr, timeout: 30 * time.Second} for _, opt := range opts { opt(s) } @@ -299,6 +307,41 @@ func NewServer(addr string, opts ...Option) *Server { } ``` +## Testing + +### 15. Table-Driven Tests Missing + +Table-driven tests reduce repetition and make it easy to add new cases. + +```go +// BAD +func TestAdd(t *testing.T) { + if Add(1, 2) != 3 { + t.Error("1+2 should be 3") + } + if Add(0, 0) != 0 { + t.Error("0+0 should be 0") + } +} + +// GOOD +func TestAdd(t *testing.T) { + tests := []struct { + a, b, want int + }{ + {1, 2, 3}, + {0, 0, 0}, + {-1, 1, 0}, + } + for _, tt := range tests { + got := Add(tt.a, tt.b) + if got != tt.want { + t.Errorf("Add(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) + } + } +} +``` + ## Review Questions 1. Is `defer Close()` called immediately after opening resources? @@ -307,3 +350,4 @@ func NewServer(addr string, opts ...Option) *Server { 4. Do exported symbols have doc comments? 5. Is mutable global state avoided? 6. Are slices preallocated when size is known? +7. Is `slog` used instead of `log` for structured output (Go 1.21+)? diff --git a/plugins/beagle-go/skills/go-code-review/references/concurrency.md b/plugins/beagle-go/skills/go-code-review/references/concurrency.md index 433e128..6d44a0a 100644 --- a/plugins/beagle-go/skills/go-code-review/references/concurrency.md +++ b/plugins/beagle-go/skills/go-code-review/references/concurrency.md @@ -4,7 +4,7 @@ ### 1. Goroutine Leak -**Problem**: Goroutines block forever, consuming memory. +Goroutines that block forever consume memory and can accumulate over time, eventually exhausting resources. ```go // BAD - no way to stop the goroutine @@ -33,7 +33,7 @@ func startWorker(ctx context.Context) { ### 2. Unbounded Channel Send -**Problem**: Sender blocks forever if receiver dies. +If the receiver dies or falls behind, the sender blocks forever. Always provide an escape hatch via context. ```go // BAD - blocks if nobody reads @@ -49,7 +49,7 @@ case <-ctx.Done(): ### 3. Closing Channel Multiple Times -**Problem**: Panic at runtime. +Closing a closed channel panics at runtime. The rule: only the sender closes the channel, and only once. ```go // BAD - potential double close @@ -58,7 +58,7 @@ close(ch) // panic! // GOOD - only sender closes, once func produce(ch chan<- int) { - defer close(ch) // close happens exactly once + defer close(ch) for i := 0; i < 10; i++ { ch <- i } @@ -67,7 +67,7 @@ func produce(ch chan<- int) { ### 4. Race Condition on Shared State -**Problem**: Data corruption, undefined behavior. +Concurrent reads and writes to maps, slices, or structs without synchronization cause data corruption and crashes. ```go // BAD - concurrent map access @@ -95,7 +95,7 @@ func Set(key string, val int) { cache[key] = val } -// BETTER - sync.Map for simple cases +// ALTERNATIVE - sync.Map for simple concurrent access patterns var cache sync.Map func Get(key string) (int, bool) { v, ok := cache.Load(key) @@ -108,7 +108,7 @@ func Get(key string) (int, bool) { ### 5. Missing WaitGroup -**Problem**: Program exits before goroutines complete. +Without synchronization, the calling function may return before spawned goroutines finish their work. ```go // BAD - may exit before done @@ -129,31 +129,36 @@ for _, item := range items { wg.Wait() ``` -### 6. Loop Variable Capture +### 6. Loop Variable Capture (Pre-Go 1.22) -**Problem**: All goroutines see the same variable value. +**Go 1.22+ fixed this** — each iteration gets its own variable. Only flag in codebases with `go.mod` specifying Go < 1.22. ```go -// BAD (pre-Go 1.22) +// ISSUE in Go < 1.22 - all goroutines see the last item for _, item := range items { go func() { - process(item) // all see last item! + process(item) // captures loop variable }() } -// GOOD - capture in closure +// FIX for Go < 1.22 - capture in closure parameter for _, item := range items { go func(item Item) { process(item) }(item) } -// Note: Go 1.22+ fixes this by default +// Go 1.22+ - this is fine, each iteration has its own variable +for _, item := range items { + go func() { + process(item) // safe + }() +} ``` ### 7. Context Not Propagated -**Problem**: Can't cancel downstream operations. +When context isn't passed to downstream calls, cancellation signals don't reach them. This means timeouts and cancellation from the caller have no effect. ```go // BAD @@ -164,7 +169,7 @@ func Handler(ctx context.Context) error { // GOOD func Handler(ctx context.Context) error { - result, err := doWork(ctx) // passes ctx + result, err := doWork(ctx) if err != nil { return err } @@ -172,6 +177,35 @@ func Handler(ctx context.Context) error { } ``` +## sync.OnceValue and sync.OnceFunc (Go 1.21+) + +These replace the common `sync.Once` + package-level variable pattern with a cleaner API: + +```go +// OLD PATTERN +var ( + dbOnce sync.Once + db *sql.DB +) +func getDB() *sql.DB { + dbOnce.Do(func() { + db, _ = sql.Open("postgres", os.Getenv("DATABASE_URL")) + }) + return db +} + +// NEW PATTERN (Go 1.21+) - type-safe, no package variable +var getDB = sync.OnceValue(func() *sql.DB { + db, _ := sql.Open("postgres", os.Getenv("DATABASE_URL")) + return db +}) + +// With error handling +var getDB = sync.OnceValues(func() (*sql.DB, error) { + return sql.Open("postgres", os.Getenv("DATABASE_URL")) +}) +``` + ## Worker Pool Pattern ```go @@ -218,6 +252,25 @@ func processItems(ctx context.Context, items []Item) error { } ``` +## errgroup Pattern + +The `golang.org/x/sync/errgroup` package simplifies the worker pool pattern with built-in context cancellation: + +```go +func processItems(ctx context.Context, items []Item) error { + g, ctx := errgroup.WithContext(ctx) + g.SetLimit(5) + + for _, item := range items { + g.Go(func() error { + return process(ctx, item) + }) + } + + return g.Wait() +} +``` + ## Review Questions 1. Are all goroutines stoppable via context? @@ -225,3 +278,5 @@ func processItems(ctx context.Context, items []Item) error { 3. Is shared state protected by mutex or sync types? 4. Are WaitGroups used to wait for goroutine completion? 5. Is context passed through the call chain? +6. Is loop variable capture handled correctly for the target Go version? +7. Are `sync.OnceValue`/`sync.OnceFunc` used instead of `sync.Once` + variable (Go 1.21+)? diff --git a/plugins/beagle-go/skills/go-code-review/references/error-handling.md b/plugins/beagle-go/skills/go-code-review/references/error-handling.md index 812d637..3aa8a42 100644 --- a/plugins/beagle-go/skills/go-code-review/references/error-handling.md +++ b/plugins/beagle-go/skills/go-code-review/references/error-handling.md @@ -4,7 +4,7 @@ ### 1. Ignoring Errors -**Problem**: Silent failures are impossible to debug. +Silent failures are impossible to debug. ```go // BAD @@ -21,7 +21,7 @@ defer file.Close() ### 2. Unwrapped Errors -**Problem**: Loses context for debugging. +Loses context for debugging. When an error bubbles up through multiple layers, each layer should add context about what it was trying to do. ```go // BAD - raw error @@ -37,11 +37,12 @@ if err != nil { ### 3. String Errors Instead of Wrapping -**Problem**: Breaks error inspection with `errors.Is/As`. +Using `%s` or `.Error()` breaks the error chain — callers can no longer use `errors.Is` or `errors.As` to inspect the underlying cause. ```go -// BAD +// BAD - breaks error inspection return fmt.Errorf("failed: %s", err.Error()) +return fmt.Errorf("failed: %v", err) // GOOD - preserves error chain return fmt.Errorf("failed: %w", err) @@ -49,14 +50,14 @@ return fmt.Errorf("failed: %w", err) ### 4. Panic for Recoverable Errors -**Problem**: Crashes the program unexpectedly. +Panics crash the program and bypass normal error handling. Reserve them for truly unrecoverable situations (programmer bugs, violated invariants), not for expected failures like I/O errors. ```go // BAD func GetConfig(path string) Config { data, err := os.ReadFile(path) if err != nil { - panic(err) // Never panic for expected errors + panic(err) } ... } @@ -73,7 +74,7 @@ func GetConfig(path string) (Config, error) { ### 5. Checking Error String Instead of Type -**Problem**: Brittle, breaks with error message changes. +Error messages can change between releases. Type-based checking is stable. ```go // BAD @@ -95,13 +96,13 @@ if errors.Is(err, ErrNotFound) { ### 6. Returning Error and Valid Value -**Problem**: Confuses callers about error semantics. +Callers expect zero values when errors are returned. Returning a meaningful value alongside an error creates ambiguity about whether the value is usable. ```go -// BAD - what does partial result mean? +// BAD - -1 is a valid integer, confuses callers func Parse(s string) (int, error) { if s == "" { - return -1, errors.New("empty string") // -1 is valid integer + return -1, errors.New("empty string") } ... } @@ -115,6 +116,47 @@ func Parse(s string) (int, error) { } ``` +## Multi-Error Aggregation (Go 1.20+) + +When a function encounters multiple independent errors (cleanup, batch processing, parallel operations), combine them with `errors.Join` instead of dropping all but one. + +```go +// BAD - loses the first error +func cleanup(db *sql.DB, f *os.File) error { + err := db.Close() + err = f.Close() // overwrites db error + return err +} + +// GOOD - preserves both errors +func cleanup(db *sql.DB, f *os.File) error { + return errors.Join(db.Close(), f.Close()) +} +``` + +`errors.Join` returns `nil` when all errors are `nil`, and the joined error supports `errors.Is`/`errors.As` for each constituent error: + +```go +err := errors.Join(ErrNotFound, ErrTimeout) +errors.Is(err, ErrNotFound) // true +errors.Is(err, ErrTimeout) // true +``` + +This is especially useful in defer chains: + +```go +func processFile(path string) (retErr error) { + f, err := os.Open(path) + if err != nil { + return fmt.Errorf("opening %s: %w", path, err) + } + defer func() { + retErr = errors.Join(retErr, f.Close()) + }() + // ... process file +} +``` + ## Sentinel Errors Pattern ```go @@ -139,6 +181,27 @@ if errors.Is(err, ErrNotFound) { } ``` +## Custom Error Types + +When you need to carry structured data with an error, implement the `error` interface: + +```go +type ValidationError struct { + Field string + Message string +} + +func (e *ValidationError) Error() string { + return fmt.Sprintf("validation failed on %s: %s", e.Field, e.Message) +} + +// Caller extracts structured data +var ve *ValidationError +if errors.As(err, &ve) { + log.Printf("field %s: %s", ve.Field, ve.Message) +} +``` + ## Review Questions 1. Are all error returns checked (no `_`)? @@ -146,3 +209,4 @@ if errors.Is(err, ErrNotFound) { 3. Are sentinel errors used for expected error conditions? 4. Does the code use `errors.Is/As` instead of string matching? 5. Does it return zero values alongside errors? +6. Are multiple independent errors aggregated with `errors.Join`? diff --git a/plugins/beagle-go/skills/go-code-review/references/interfaces.md b/plugins/beagle-go/skills/go-code-review/references/interfaces.md index 49ff8be..20b5a03 100644 --- a/plugins/beagle-go/skills/go-code-review/references/interfaces.md +++ b/plugins/beagle-go/skills/go-code-review/references/interfaces.md @@ -1,10 +1,10 @@ -# Interfaces +# Interfaces and Types ## Critical Anti-Patterns ### 1. Premature Interface Definition -**Problem**: Interfaces defined before needed, creating abstraction overhead. +Interfaces should be defined where they're consumed, not where the implementation lives. Defining them in the producer package couples the abstraction to a specific implementation. ```go // BAD - interface in producer package @@ -31,7 +31,7 @@ func NewUserService(users UserGetter) *UserService { ### 2. Interface Pollution (Too Many Methods) -**Problem**: Hard to implement, hard to mock, violates ISP. +Fat interfaces are hard to implement, hard to mock, and force consumers to depend on methods they don't use. ```go // BAD - fat interface @@ -42,10 +42,9 @@ type UserStore interface { Delete(id int) error Search(query string) ([]*User, error) Count() (int, error) - // ... 10 more methods } -// GOOD - focused interfaces +// GOOD - focused interfaces composed as needed type UserGetter interface { Get(id int) (*User, error) } @@ -62,7 +61,7 @@ type UserStore interface { ### 3. Wrong Interface Names -**Problem**: Doesn't follow Go conventions, less readable. +Go convention: single-method interfaces are named after the method with an `-er` suffix. ```go // BAD @@ -82,7 +81,7 @@ type UserWriter interface { ### 4. Returning Interface Instead of Concrete Type -**Problem**: Hides implementation details unnecessarily. +Returning interfaces from constructors hides information from callers and prevents them from accessing implementation-specific methods. Accept interfaces, return structs. ```go // BAD - returns interface @@ -96,48 +95,93 @@ func NewServer(addr string) *HTTPServer { } ``` -### 5. Empty Interface Overuse +### 5. Interface for Single Implementation -**Problem**: Loses type safety, requires type assertions. +An interface with only one implementation adds indirection without benefit. Introduce interfaces when you actually need them (testing, multiple implementations, package boundary decoupling). ```go -// BAD -func Process(data interface{}) interface{} { - switch v := data.(type) { - case string: - return strings.ToUpper(v) - case int: - return v * 2 - } - return nil +// BAD - interface with only one implementation and no tests mocking it +type ConfigLoader interface { + Load() (*Config, error) } -// GOOD - use generics (Go 1.18+) -func Process[T string | int](data T) T { - // type-safe processing -} +type fileConfigLoader struct { ... } -// Or use specific types -func ProcessString(data string) string -func ProcessInt(data int) int +// GOOD - just use the concrete type until you need the abstraction +type ConfigLoader struct { ... } + +func (c *ConfigLoader) Load() (*Config, error) { ... } ``` -### 6. Interface for Single Implementation +## Generics (Go 1.18+) -**Problem**: Unnecessary abstraction with no benefit. +### Prefer `any` over `interface{}` + +The `any` keyword is an alias for `interface{}` introduced in Go 1.18. It's clearer and more idiomatic in modern Go code. ```go -// BAD - interface with only one implementation -type ConfigLoader interface { - Load() (*Config, error) +// OLD +func Process(data interface{}) interface{} { ... } + +// MODERN +func Process(data any) any { ... } +``` + +### Use Type Constraints Instead of `any` + +When you know the set of types you need, use constraints to preserve type safety. `any` in a generic function means you've given up type checking. + +```go +// BAD - any constraint means no useful operations +func Max[T any](a, b T) T { + // Can't compare a and b! } -type fileConfigLoader struct { ... } +// GOOD - constrained to comparable and ordered types +func Max[T cmp.Ordered](a, b T) T { + if a > b { + return a + } + return b +} +``` -// GOOD - just use the concrete type -type ConfigLoader struct { ... } +### Common Generic Anti-Patterns -func (c *ConfigLoader) Load() (*Config, error) { ... } +```go +// BAD - generic function that only works with one type +func ParseUserID[T ~string](s T) (int, error) { + return strconv.Atoi(string(s)) +} +// Just use string directly + +// BAD - over-genericized struct +type Cache[K comparable, V any] struct { ... } +// Only used as Cache[string, *User] throughout the codebase +// Generics add value when there are multiple instantiations + +// GOOD - generics for truly reusable code +func Map[T, U any](slice []T, fn func(T) U) []U { + result := make([]U, len(slice)) + for i, v := range slice { + result[i] = fn(v) + } + return result +} +``` + +### Type Constraints with `~` (Underlying Types) + +The `~` prefix matches types with the same underlying type, which is important for custom types: + +```go +type UserID int64 + +// Without ~: only accepts int64, not UserID +func Format[T int64](id T) string { ... } + +// With ~: accepts int64 AND UserID +func Format[T ~int64](id T) string { ... } ``` ## Accept Interfaces, Return Structs @@ -161,32 +205,18 @@ WriteData(buf, []byte("hello")) // Buffer implements io.Writer ## Standard Library Interfaces to Use -```go -// io.Reader - anything that can be read from -type Reader interface { - Read(p []byte) (n int, err error) -} - -// io.Writer - anything that can be written to -type Writer interface { - Write(p []byte) (n int, err error) -} +Prefer these over custom interfaces when your use case matches: -// io.Closer - anything that can be closed -type Closer interface { - Close() error -} - -// fmt.Stringer - custom string representation -type Stringer interface { - String() string -} - -// error - the error interface -type error interface { - Error() string -} -``` +| Interface | Package | Use When | +|-----------|---------|----------| +| `io.Reader` | io | Anything that provides bytes | +| `io.Writer` | io | Anything that accepts bytes | +| `io.Closer` | io | Anything that releases resources | +| `fmt.Stringer` | fmt | Custom string representation | +| `error` | builtin | Any error condition | +| `sort.Interface` | sort | Custom sort ordering (pre-generics; prefer `slices.SortFunc` in Go 1.21+) | +| `encoding.TextMarshaler` | encoding | Custom text serialization | +| `slog.LogValuer` | log/slog | Custom structured log values (Go 1.21+) | ## Review Questions @@ -194,4 +224,6 @@ type error interface { 2. Are interfaces minimal (1-3 methods)? 3. Do interface names end in `-er`? 4. Are concrete types returned from constructors? -5. Is `interface{}` avoided in favor of generics or specific types? +5. Is `any` used instead of `interface{}` (Go 1.18+)? +6. Are generics used where they add real value (multiple instantiations)? +7. Are type constraints specific enough (not just `any`)?