search

dominikh / go-tools

Staticcheck - The advanced Go linter
https://staticcheck.dev
MIT License
6.24k stars 377 forks source link

staticcheck: SA4006 false-negative when a defer is used #685

Open ainar-g opened 4 years ago

ainar-g commented 4 years ago
Formalities 
$ staticcheck --version
staticcheck (devel, v0.0.0-20200121224137-2774002210e8)
$ staticcheck --debug.version
staticcheck (devel, v0.0.0-20200121224137-2774002210e8)

Compiled with Go version: go1.13.7
Main module:
    honnef.co/go/tools@v0.0.0-20200121224137-2774002210e8 (sum: h1:LDZNqNckGrHSM2hO0hPWJKIqdodYH0Tov7n86ikeGJE=)
Dependencies:
    github.com/BurntSushi/toml@v0.3.1 (sum: h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=)
    golang.org/x/tools@v0.0.0-20191130070609-6e064ea0cf2d (sum: h1:/iIZNFGxc/a7C3yWjGcnboV+Tkc7mxr+p6fDztwoxuM=)
$ go version
go version go1.13.7 linux/amd64
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ainar/.cache/go-build"
GOENV="/home/ainar/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ainar/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ainar/go/go1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ainar/go/go1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build783553461=/tmp/go-build -gno-record-gcc-switches"

The code itself: https://play.golang.org/p/97Vr6raQX-i.

On line 21, an error check is missing, but because of the following defer, staticcheck doesn't complain: 

$ staticcheck ./go.tmp.go
$

I remember a similar bug, but I couldn't find the issue.

dominikh commented 4 years ago

From an implementation point of view: the anonymous function refers to err, which means that the variable can't be lifted into registers; it'll be an allocation. The way we check for unread assignments is by checking whether the register gets read.

For allocations, this isn't nearly as trivial (nor solvable for all cases.) For example, err could be aliased and read via the alias, or g could panic, leading to the deferred function reading the value. We could handle some of these cases (e.g. handle allocations that can't be aliased), but for your concrete example, we still couldn't claim that err is unread, due to the possibility of g panicking and thus reading that value.