staticcheck: flag impossible branches

[josharian]

False negative on this (real) code:

    switch {
    case tp == dns.TypeA || tp == dns.TypeALL:
        if !addr.Is4() {
            return netaddr.IP{}, dns.RCodeSuccess, nil
        }
        return addr, dns.RCodeSuccess, nil
    case tp == dns.TypeAAAA || tp == dns.TypeALL:
        if !addr.Is6() {
            return netaddr.IP{}, dns.RCodeSuccess, nil
        }
        return addr, dns.RCodeSuccess, nil
    default:
        return netaddr.IP{}, dns.RCodeNotImplemented, errNotImplemented
    }

Rewriting it to the equivalent, simpler form makes the bug obvious, and also enables staticcheck to catch it:

    switch tp {
    case dns.TypeA, dns.TypeALL:
        if !addr.Is4() {
            return netaddr.IP{}, dns.RCodeSuccess, nil
        }
        return addr, dns.RCodeSuccess, nil
    case dns.TypeAAAA, dns.TypeALL:
        if !addr.Is6() {
            return netaddr.IP{}, dns.RCodeSuccess, nil
        }
        return addr, dns.RCodeSuccess, nil
    default:
        return netaddr.IP{}, dns.RCodeNotImplemented, errNotImplemented
    }

Output:

$ staticcheck ./...
wgengine/tsdns/tsdns.go:215:18:     previous case (compile)
wgengine/tsdns/tsdns.go:220:21: duplicate case dns.TypeALL (constant 255 of type dnsmessage.Type) in expression switch (compile)

System details:

$ staticcheck -version
staticcheck 2020.2 (v0.1.0)
$ staticcheck -debug.version
staticcheck 2020.2 (v0.1.0)

Compiled with Go version: go1.15.5
Main module:
    honnef.co/go/tools@v0.1.0 (sum: h1:AWNL1W1i7f0wNZ8VwOKNJ0sliKvOF/adn0EHenfUh+c=)
Dependencies:
    github.com/BurntSushi/toml@v0.3.1 (sum: h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=)
    golang.org/x/mod@v0.4.0 (sum: h1:8pl+sMODzuvGJkmj2W4kZihvVb5mKm8pB/X44PIQHv8=)
    golang.org/x/tools@v0.0.0-20201211185031-d93e913c1a58 (sum: h1:1Bs6RVeBFtLZ8Yi1Hk07DiOqzvwLD/4hln4iahvFlag=)
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 (sum: h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=)
$ go version
go version go1.15.5 darwin/amd64
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/josh/bin"
GOCACHE="/Users/josh/Library/Caches/go-build"
GOENV="/Users/josh/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/josh/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/josh"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/josh/go/1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/josh/go/1.15/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/josh/t/corp/oss/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/go-build981923805=/tmp/go-build -gno-record-gcc-switches -fno-common"

[dominikh]

Actually, the rewritten form allows the Go compiler to detect the issue; Staticcheck didn't contribute to that.

This might be worth adding a check for. At the same time, I want to add a check that transforms from the first to the second form, in which case we'd get the compile error for free, but even then we might want to flag differently shaped boolean expressions that are impossible.

[josharian]

flag differently shaped boolean expressions that are impossible.

One note on that, taken from a comment I wrote in the compiler (swt.go):

            // Don't check for duplicate bools. Although the spec allows it,
            // (1) the compiler hasn't checked it in the past, so compatibility mandates it, and
            // (2) it would disallow useful things like
            //       case GOARCH == "arm" && GOARM == "5":
            //       case GOARCH == "arm":
            //     which would both evaluate to false for non-ARM compiles.

So there are some legit cases there.

[dominikh]

Yeah, we could only check this symbolically, otherwise build tags will ruin everything.