DEV Community

Cover image for 10 Go Mistakes That Survive Code Review (2026 Edition)
Gabriel Anhaia
Gabriel Anhaia

Posted on

10 Go Mistakes That Survive Code Review (2026 Edition)

๐Ÿ“š This post pairs with two books I've written on Go. Book 1: The Complete Guide to Go Programming. Book 2: Hexagonal Architecture in Go. Or both together as the Thinking in Go collection: Kindle / Paperback. Short blurbs at the bottom.

Series - Thinking in Go

Every "Go mistakes" listicle on the internet is written for beginners. Half of them still include loop-variable capture in goroutines, which Go 1.22 fixed two years ago. The other half include nil maps and := shadowing, which every Go developer reading this post already knows about.

This is the list for people past that. Every mistake below passes go vet on Go 1.26. Every one looks like reasonable code in review. Every one has shipped to production more times than anybody wants to admit. Let's go.

1. sync.Mutex copied by value via struct embedding

A classic that still shows up in PRs. You embed a sync.Mutex in a struct, then someone passes the struct by value somewhere, and now you have two mutexes pretending to be one.

type Counter struct {
    sync.Mutex // embedded
    count int
}

func (c Counter) Inc() { // receiver is VALUE, not pointer
    c.Lock()
    c.count++
    c.Unlock()
}
Enter fullscreen mode Exit fullscreen mode

Every call to Inc gets its own copy of c, including its own copy of the embedded mutex. The counter doesn't increment across goroutines because each goroutine is locking its own local mutex.

The fix is a pointer receiver, and for safety, the govet copylocks check:

func (c *Counter) Inc() {
    c.Lock()
    c.count++
    c.Unlock()
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: Go lets you call Lock and Unlock on a value-received mutex without a compile error. The copying is legal. The bug is behavioral. go vet has a copylocks check that catches most cases, but not all of them, especially when the struct ends up in a slice or map.

2. Nil interface vs interface holding a typed nil

The trap everyone writes once and remembers forever:

type Fetcher interface {
    Fetch() ([]byte, error)
}

type HTTPFetcher struct{}

func (h *HTTPFetcher) Fetch() ([]byte, error) { return nil, nil }

func newFetcher(useHTTP bool) Fetcher {
    var f *HTTPFetcher
    if useHTTP {
        f = &HTTPFetcher{}
    }
    return f // <-- trap
}

func main() {
    f := newFetcher(false)
    if f == nil {
        fmt.Println("nil fetcher, skipping")
    } else {
        f.Fetch() // PANIC
    }
}
Enter fullscreen mode Exit fullscreen mode

newFetcher(false) returns a Fetcher interface that contains a typed nil *HTTPFetcher. The interface itself is not nil. It has a concrete type (*HTTPFetcher) and a nil value. Interface equality with nil checks whether both the type and the value are nil. Only the value is.

Result: the if f == nil check fails, the code calls f.Fetch(), and because f is pointing at a nil pointer, the method call panics inside Fetch the first time it dereferences the receiver.

Fix: never return a typed nil through an interface. Return the interface's nil explicitly:

func newFetcher(useHTTP bool) Fetcher {
    if useHTTP {
        return &HTTPFetcher{}
    }
    return nil // interface nil, not typed nil
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: the buggy version is a direct refactor of "build the concrete type, return it as the interface." It looks symmetrical. The asymmetry is that untyped nil and typed nil wrapped in an interface are not the same value.

3. append to a sliced slice mutating the parent

Slices are views over a backing array. append sometimes reuses that backing array, and when it does, you can mutate a completely different slice that you didn't touch.

func main() {
    original := []int{1, 2, 3, 4, 5}
    view := original[:2]                // [1 2], backed by original
    withExtra := append(view, 99)       // does this copy? depends.

    fmt.Println(original)  // [1 2 99 4 5], original[2] got overwritten
    fmt.Println(withExtra) // [1 2 99]
}
Enter fullscreen mode Exit fullscreen mode

view has length 2, capacity 5 (inherited from original). append(view, 99) has room to write into the existing backing array, so it does, at index 2. That index is original[2], which you just changed from 3 to 99 without meaning to.

The fix is to force a copy when you need the view to be independent:

view := append([]int(nil), original[:2]...) // now view has its own backing array
withExtra := append(view, 99)               // safe
Enter fullscreen mode Exit fullscreen mode

Or in Go 1.21+, use slices.Clone:

view := slices.Clone(original[:2])
Enter fullscreen mode Exit fullscreen mode

Why it slips through: the bug only happens when the parent slice has extra capacity, which depends on how the parent was constructed. It won't show up in your unit tests if you built the parent with exact-capacity make. It shows up in production when someone passes a slice from somewhere else.

4. Shadowing err inside nested if blocks

This one is the Go error-handling version of "wait, why isn't my error bubbling up?":

func load() error {
    data, err := fetch()
    if err != nil {
        return err
    }

    if len(data) > 0 {
        // NEW err, shadows the outer one
        if err := validate(data); err != nil {
            log.Println("invalid:", err)
            // fallthrough
        }
    }

    // outer err is still whatever fetch returned, probably nil
    return err
}
Enter fullscreen mode Exit fullscreen mode

The inner if err := validate(data); err != nil declares a new err in the inner block. Nothing propagates to the outer err. If validate failed, the function returns nil anyway and the caller thinks everything is fine.

Fix: don't shadow. Use a different name, or assign to the outer variable:

func load() error {
    data, err := fetch()
    if err != nil {
        return err
    }

    if len(data) > 0 {
        if verr := validate(data); verr != nil {
            return verr
        }
    }
    return nil
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: := in an inner scope is legal Go and sometimes exactly what you want. go vet's shadow check is not on by default because it's too noisy. The linter won't save you here.

5. == on errors instead of errors.Is

The moment somebody wraps an error with fmt.Errorf("context: %w", err), every downstream == check against a sentinel silently stops working:

// in package A
var ErrNotFound = errors.New("not found")

func FindUser(id string) (*User, error) {
    return nil, fmt.Errorf("users.FindUser: %w", ErrNotFound) // wraps it
}

// in package B, caller
u, err := users.FindUser("123")
if err == ErrNotFound { // FALSE, because err is now a wrapped error
    // ...
}
Enter fullscreen mode Exit fullscreen mode

The wrapped error's concrete value is a *fmt.wrapError, not ErrNotFound. == compares pointers. The pointer isn't the same.

Fix is errors.Is:

if errors.Is(err, ErrNotFound) {
    // ...
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: the original code with == worked fine before somebody added the wrap. Your tests might still use the unwrapped version. Then production uses the wrapped one and your conditional silently flips.

Linter note: errorlint catches this. It's not part of go vet, but if you use golangci-lint, turn it on. It's the single most valuable lint rule for Go error handling.

6. json.Unmarshal into map[string]any losing number precision

This one bites teams dealing with user IDs, timestamps in milliseconds, or anything else that needs precise integers.

raw := []byte(`{"id": 9007199254740993}`)

var m map[string]any
_ = json.Unmarshal(raw, &m)

fmt.Println(m["id"]) // 9.007199254740992e+15
Enter fullscreen mode Exit fullscreen mode

The ID in the JSON was 9007199254740993. What came out of Unmarshal was 9007199254740992, because json.Unmarshal decodes JSON numbers into float64 by default, and float64 can only represent integers exactly up to 2^53 = 9007199254740992. One bit past that, you're in approximation territory.

Fix 1: if you know the shape, use a typed struct:

type payload struct {
    ID int64 `json:"id"`
}
var p payload
_ = json.Unmarshal(raw, &p)
// p.ID == 9007199254740993, exact
Enter fullscreen mode Exit fullscreen mode

Fix 2: if you really need the dynamic map, use a json.Decoder with UseNumber:

dec := json.NewDecoder(bytes.NewReader(raw))
dec.UseNumber()
var m map[string]any
_ = dec.Decode(&m)

id, _ := m["id"].(json.Number).Int64()
// id == 9007199254740993, exact
Enter fullscreen mode Exit fullscreen mode

Why it slips through: the bug is silent. No error, no warning, no panic. The number just ends up slightly wrong, and if the thing it's identifying doesn't exist in your DB, the lookup fails with "not found" and nobody connects the two.

7. Forgetting to call the stop func from context.AfterFunc

context.AfterFunc was added in Go 1.21. It runs a callback when the context is done, and returns a stop func you can call to cancel the callback if you don't need it anymore.

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// schedule a cleanup
stop := context.AfterFunc(ctx, func() {
    log.Println("context done, cleaning up")
})

// if things work out, we want to NOT run the cleanup
if ok := doSomething(); ok {
    stop() // cancel the AfterFunc
    return
}
Enter fullscreen mode Exit fullscreen mode

The subtle bug: if doSomething() succeeds and you don't call stop, the callback is still armed. When the context is eventually canceled (which it will be, because of the defer cancel()), the callback fires, even though you logically wanted to cancel it.

Fix: always call stop when you want to cancel the scheduled callback, and always at least consider whether you need it.

stop := context.AfterFunc(ctx, cleanup)
defer stop() // if you want "cancel cleanup on early return"
Enter fullscreen mode Exit fullscreen mode

Why it slips through: context.AfterFunc is newer than most people's mental model of context. The API looks like "fire and forget," but it isn't. The returned stop is not optional if you care about controlling the callback's lifetime.

8. Comparing interfaces holding different concrete types (panic)

Interface equality in Go compares both the type and the value. If the types are the same, it compares values. If the types are different, the result is false. If the types are uncomparable (like []int or map[string]int), the comparison panics at runtime.

var a any = []int{1, 2, 3}
var b any = []int{1, 2, 3}

fmt.Println(a == b) // panic: runtime error: comparing uncomparable type []int
Enter fullscreen mode Exit fullscreen mode

The classic setup: you have a map[string]any and you're deduplicating by comparing values. The first time somebody puts a slice or a map in that map as a value, your dedup code panics in production.

Fix: don't use == on any if you don't know the concrete type. Use reflect (reflect.DeepEqual), a typed comparator, or design the data so the values are always comparable types.

if reflect.DeepEqual(a, b) {
    // works for slices, maps, structs containing slices, etc.
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: the code compiles. go vet doesn't flag it. Unit tests with scalar values pass. The panic happens the first time a slice shows up in production data.

9. defer in a long-running function holding resources until return

defer runs at function return, not at the end of the current loop iteration. Put a defer in a long-running function and you can hold file handles, locks, or connections for the entire life of the function.

func processFiles(paths []string) error {
    for _, p := range paths {
        f, err := os.Open(p)
        if err != nil {
            return err
        }
        defer f.Close() // runs at function return, NOT end of iteration

        if err := handleFile(f); err != nil {
            return err
        }
    }
    return nil
}
Enter fullscreen mode Exit fullscreen mode

If paths has 10,000 entries, you end up with 10,000 open file handles, all waiting to be closed when processFiles returns. The OS usually caps your process file descriptors somewhere around 1024 by default. You blow through that limit, and the error message is too many open files with no indication that defer is the culprit.

Fix: wrap the per-iteration work in a helper function so defer scopes to a single file:

func processFiles(paths []string) error {
    for _, p := range paths {
        if err := processOne(p); err != nil {
            return err
        }
    }
    return nil
}

func processOne(p string) error {
    f, err := os.Open(p)
    if err != nil {
        return err
    }
    defer f.Close() // scopes to processOne, runs at end of each iteration
    return handleFile(f)
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: defer reads as "close this when I'm done with it," and for a single-shot function that's correct. The "when I'm done" is ambiguous only when the function itself is long-running.

10. Struct embedding with colliding method names

Embed two structs that both have a method with the same name, and the resulting type compiles fine. You can declare it, construct values of it, pass it around. The bug doesn't fire until somebody tries to call the colliding method:

type Clock struct{}

func (Clock) String() string { return "12:34" }

type Logger struct{}

func (Logger) String() string { return "[logger]" }

type App struct {
    Clock
    Logger
}

func main() {
    var a App
    fmt.Println(a.String()) // compile error: ambiguous selector a.String
}
Enter fullscreen mode Exit fullscreen mode

App embeds both Clock and Logger. Both have String() string. The Go spec says method promotion fails when there's a conflict at the same depth, which means App doesn't have a promoted String() method at all. Calling a.String() is a compile error. Assigning App{} to a fmt.Stringer variable is also a compile error. But the struct declaration itself compiles.

The nastier version: fmt.Println(a) will compile and run, because fmt takes any. It just prints the struct fields, because reflection can't find an unambiguous String() method. So App silently fails to satisfy fmt.Stringer while looking like it should.

Fix: don't embed two types that share a method name. Pick one, or promote explicitly:

type App struct {
    clock  Clock
    logger Logger
}

func (a App) String() string {
    return a.clock.String() + " " + a.logger.String()
}
Enter fullscreen mode Exit fullscreen mode

Why it slips through: Go's embedding model looks like multiple inheritance but isn't. Readers assume the method collision is caught at declaration time, the way Java would flag it. Go defers the error to the call site, and if no caller calls the colliding method, the bug sleeps in the codebase. Then somebody adds a log.Printf("%s", a) and the whole thing falls over.

Meta: the shape of these bugs

Most of the mistakes above come from Go taking one design decision seriously and you taking a second decision for granted.

The language is happy to let you:

  • Copy a mutex by value, because mutexes are values.
  • Wrap a typed nil in an interface, because interfaces are type+value pairs.
  • Mutate a parent slice through a child slice, because slices share backing arrays.
  • Shadow a variable in an inner scope, because := is designed to.
  • Compare errors with ==, because errors are values.
  • Lose precision in JSON numbers, because float64 is the default.
  • Compare uncomparable interfaces at runtime, because interface equality is dynamic.
  • Defer 10,000 closures in one function, because defer is function-scoped.

None of those are bugs in Go. They're all design choices the language made explicit. What turns them into bugs is your code making a second assumption that silently conflicts with the first. The fix is always the same: read the language spec's version of what you're doing, not your intuition's version.

Bonus: go fix in Go 1.26 catches some of these

Go 1.26 rewrote go fix as a "modernizer" framework built on the same analysis engine as go vet. It's not a silver bullet, but it will flag some of the patterns above (particularly the copylocks for Mistake 1 and the err shadowing for Mistake 4 if you enable the right analyzer). Run go fix ./... periodically and read what it wants to change. The hit rate on real codebases is better than the 1.25 version.

For the others, code review is the safety net. Take this list, paste it into your team's review checklist, and grep for the patterns in your codebase. You'll find at least three of them.

Next step

Open one package in your codebase. Search it for:

  1. sync.Mutex or sync.RWMutex embedded in a struct with non-pointer receivers.
  2. Functions that return an interface type and could return a typed nil.
  3. err == anywhere near an error that might get wrapped.
  4. json.Unmarshal into a map[string]any.
  5. defer inside a for loop.

Each of those is a 5-minute grep. Each of those can find a bug that's already shipping. Start there.

Question for the comments: which one of these ten has burned you personally, and how did you catch it? Drop the story, or add an eleventh if I missed one.


The books

๐Ÿ“– The Complete Guide to Go Programming โ€” Book 1. The language from the ground up, including the memory model, interface internals, and the quirks that cause the bugs in this post.

๐Ÿ“– Hexagonal Architecture in Go โ€” Book 2. How to design Go services where half of these bugs can't happen because the architecture doesn't let them. 22 chapters + a companion repo.

๐Ÿ“š Or the full collection: Thinking in Go on Amazon as Kindle or Paperback.

Top comments (0)