SA2003: false negatives

[peterbourgon]

$ staticcheck -version
staticcheck 2019.2.3

$ staticcheck -debug.version
staticcheck 2019.2.3

Compiled with Go version: go1.13
Main module:
        honnef.co/go/tools@v0.0.1-2019.2.3 (sum: h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=)
Dependencies:
        github.com/BurntSushi/toml@v0.3.1 (sum: h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=)
        golang.org/x/tools@v0.0.0-20190621195816-6e04913cbbac (sum: h1:MQEvx39qSf8vyrx3XRaOe+j1UDIzKwkYOVObRgGPVqI=)

$ go version
go version go1.13 darwin/amd64

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/peter/Library/Caches/go-build"
GOENV="/Users/peter/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/peter"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/8s/kvkb2dzx37n057vk54y9b_ym0000gn/T/go-build681978744=/tmp/go-build -gno-record-gcc-switches -fno-common"

Following on from this Twitter thread. Given this Go file:

     1 package bleh
     2 
     3 import "sync"
     4 
     5 type T struct {
     6      rwmtx sync.RWMutex
     7      mtx   sync.Mutex
     8 }
     9 
    10 func (t *T) M() {
    11     t.rwmtx.RLock()
    12     defer t.rwmtx.RLock()
    13     t.rwmtx.Lock()
    14     defer t.rwmtx.Lock()
    15     t.mtx.Lock()
    16     defer t.mtx.Lock()
    17 
    18     var rwmtx sync.RWMutex
    19     var mtx sync.Mutex
    20     rwmtx.RLock()
    21     defer rwmtx.RLock()
    22     rwmtx.Lock()
    23     defer rwmtx.Lock()
    24     mtx.Lock()
    25     defer mtx.Lock()
    26 }

staticcheck SA2003 has several false negatives.

$ staticcheck
bleh.go:21:2: deferring RLock right after having locked already; did you mean to defer RUnlock? (SA2003)
bleh.go:25:2: deferring Lock right after having locked already; did you mean to defer Unlock? (SA2003)

I would expect warnings on lines 12, 14, 16, 21, 23, and 25. I only get warnings on lines 21 and 25.

(Hat tip to @athomason for tracking this down.)

[dominikh]

There are two separate issues here: mutexes that aren't stored in variables, and a combination of RLock + defer Lock.

Addressing the second issue first, I don't think it is something we should fix. The likelihood of someone making this mistake seems low to me (feel free to prove me wrong, though), and it seems more likely to cause a false positive in some bizarrely written code.

Addressing the first issue, what follows is the technical explanation of why it happens, and how we'll fix it. Feel free to skip over it.

The underlying cause of the first issue is the current, SSA-based implementation of the check. We're checking for a sequence of two instructions, a call to Lock/RLock, immediately followed by a defer of the same. This does not work for mutexes that aren't stored in (SSA-registerized) variables, because additional instructions for loading the value will be emitted.

In the case of struct fields, we'd see a sequence like this:

t1 = &t.rwmtx [#0]                                        *sync.RWMutex
t3 = (*sync.RWMutex).RLock(t1)                                       ()
t5 = &t.rwmtx [#0]                                        *sync.RWMutex
defer (*sync.RWMutex).RLock(t5)

while for a local variable, it'd instead look like this:

t1 = new sync.RWMutex (rwmtx)                             *sync.RWMutex
t2 = (*sync.RWMutex).RLock(t1)                                       ()
defer (*sync.RWMutex).RLock(t1)

There are other possible sequences for mutexes stored in maps, slices, when using new for allocating variables and likely at least one other scenario I'm forgetting about. Rather than handling all of these sequences, we may be much better off implementing this check by looking at the AST instead, where the call and the defer would truly be two consecutive statements¹.

It's unfortunate that we chose to use SSA for this, as it greatly diminished the value of the check for a long time. In fact, we haven't always used SSA for it. Before commit 0156b8217b6e76d483ff843f93b909483db7d6bb, it was implemented by looking at the AST. We were too eager to use SSA, and our test cases weren't exhaustive enough, or they would've caught this regression.

¹: That is, unless we're concerned about code like this:

mu.Lock()
{
  defer mu.Lock()
}

Personally, I am not. Though it would be possible to handle this, too.

[peterbourgon]

. . . a combination of RLock + defer Lock . . , I don't think it is something we should fix. The likelihood of someone making this mistake seems low to me (feel free to prove me wrong, though), and it seems more likely to cause a false positive in some bizarrely written code.

I'm not sure I understand, can you clarify which case(s) this refers to?

As for the rest, it follows and I'm 👍

[dominikh]

I'm not sure I understand, can you clarify which case(s) this refers to?

I'm not surprised, because I misread the code… For completeness sake, I was thinking of the following code:

var rwmtx sync.RWMutex
rwmtx.RLock()
defer rwmtx.Lock()

but apparently you never wrote that.

I see now that you wrote this instead:

var rwmtx sync.RWMutex
rwmtx.Lock()
defer rwmtx.Lock()

which should be flagged by staticcheck, but isn't. The reason for that is another bug in the check, which only looks for RLock on RWMutex, not Lock.