Enhancements to interface comparison checks

I recently ran into a few bugs in some of our projects around interface comparison (i.e. directly checking if an interface var == nil or two different interface vars are == to each other where one is nil and the other is a concrete nil pointer). I was surprised our CI linter didn't catch these so I wrote a little linter to help prevent these types of issues in the future.

The golangci-lint team rejected the PR because they believed this behavior was already caught by staticcheck SA4023. However, after some further research (and refining the examples to be a bit closer to real world instances of this bug) I found that staticcheck isn't always detecting this type of bug.

Here is a simplified example that I believe should be considered a bug but isn't currently being reported by staticcheck:

package main

import (
    "fmt"
    "time"
)

type Greeter interface {
    SayHi() string
}

type CostcoGreeter struct {}

func (c *CostcoGreeter) SayHi() string {
    return "Welcome to Costco, I love you."
}

func main() {
    var greeter Greeter = getCachedGreeter()

    if greeter == nil {
        panic("greeter is nil") // we might expect this to panic, but it doesn't!
    }

    fmt.Println("ok")
}

// getCachedGreeter simulates cache miss at runtime and will always return nil *CostcoGreeter
func getCachedGreeter() Greeter {
    now := time.Now().Unix()

    switch {
    case now == 0:
        return new(CostcoGreeter)
    case now == 1:
        return nil
    default:
        var c *CostcoGreeter
        return c
    }
}

And here is an example of comparing two interface vars:

package main

import (
    "fmt"
    "time"
)

type Greeter interface {
    SayHi() string
}

type WallyWorldGreeter struct {}

func (w *WallyWorldGreeter) SayHi() string {
    return "Sorry, folks! We're closed for two weeks to clean and repair America's favorite family fun park. Sorry, uh-huh, uh-huh, uh-huh!"
}

func main() {
    var greeterA Greeter
    var greeterB Greeter = getCachedGreeter()

    if greeterA == greeterB {
        panic("greeterA == greeterB") // we might expect this to panic since both are nil, but it doesn't!
    }

    fmt.Println("ok")
}

// getCachedGreeter simulates cache miss at runtime and will always return nil *WallyWorldGreeter
func getCachedGreeter() Greeter {
    now := time.Now().Unix()

    switch {
    case now == 0:
        return new(WallyWorldGreeter)
    case now == 1:
        return nil
    default:
        var w *WallyWorldGreeter
        return w
    }
}

Would you be interested in a PR that reports these types of issues?

dominikh / go-tools

Enhancements to interface comparison checks #1596