cmd/vet: should check unreachable code after t.FailNow or related functions

[Abirdcfly]

What version of Go are you using (go version)?

$ go version
go version go1.18.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN="/usr/local/opt/go/libexec/bin"
GOCACHE="/Users/xxx/Library/Caches/go-build"
GOENV="/Users/xxx/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/fupeng/go/pkg/mod"
GONOPROXY="xxx"
GONOSUMDB="xxx"
GOOS="darwin"
GOPATH="/Users/fupeng/go"
GOPRIVATE="xxx"
GOPROXY="https://goproxy.cn"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/xxx/go/src/revive/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3v/mkwyck4x0f5bxv2gdv4_l8v00000gn/T/go-build2714321280=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

should check unreachable code after t.SkipNow or t.FailNow or related functions.
I'm not quite sure if this is a bug or if it's designed that way.
If it's a bug, I'd be happy to fix it and make it my first commit to the golang language.
example:

func TestA(t *testing.T) {
	tests := make([]int, 100)
	for i := range tests {
		println("i: ", i)
		if i == 0 {
			t.Fatal("i == 0") // MATCH /unreachable code after this statement/
			println("unreachable")
			continue
		}
		if i == 1 {
			t.Fatalf("i:%d", i) // MATCH /unreachable code after this statement/
			println("unreachable")
			continue
		}
		if i == 2 {
			t.FailNow() // MATCH /unreachable code after this statement/
			println("unreachable")
			continue
		}
		/*  Thanks @dominikh 
                    Skip is used to temporarily skip tests. The code following it is intentionally unreachable.
			if i == 3 {
				t.Skip("i == 3") // MATCH /unreachable code after this statement/
				println("unreachable")
				continue
			}
			if i == 4 {
				t.Skipf("i:%d", i) // MATCH /unreachable code after this statement/
				println("unreachable")
				continue
			}
			if i == 5 {
				t.SkipNow() // MATCH /unreachable code after this statement/
				println("unreachable")
				continue
			}
		*/
	}
}

A practical example comes from the kubernetes tests. kubernetes project has used govet and golangci-lint for static detection.

https://github.com/kubernetes/kubernetes/blob/8b16a4a4df64d13c0e01015820de0245f493e90c/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go#L2841-L2848

What did you expect to see?

$ go vet -tests -unreachable ./...
# unreachable
./main_test.go:10:2: unreachable code  # detect it

What did you see instead?

$ go vet -tests -unreachable ./...

[mvdan]

Worth noting that I believe @dominikh's staticcheck has done this for a while.

[dominikh]

Worth noting that I believe @dominikh's staticcheck has done this for a while.

Only as a side-effect in other checks, and in the case of Skip, much to my users' dismay.

Doing this for SkipNow is definitely a bad idea. Skip is used to temporarily skip tests. The code following it is intentionally unreachable.

[Abirdcfly]

Only as a side-effect in other checks, and in the case of Skip, much to my users' dismay.

You mean SA2002 Called testing.T.FailNow or SkipNow in a goroutine, which isn’t allowed?

Doing this for SkipNow is definitely a bad idea. Skip is used to temporarily skip tests. The code following it is intentionally unreachable.

Thanks @dominikh You are right, I just updated the description above. Skip should be ignored and the point of discussion should be FailNow.

A practical example comes from the kubernetes tests. kubernetes project has used govet and golangci-lint for static detection.

https://github.com/kubernetes/kubernetes/blob/8b16a4a4df64d13c0e01015820de0245f493e90c/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go#L2841-L2848

[timothy-king]

The unreachable checker needs to know the "noreturned"-ness of function/method calls. To have this the checker either needs to special case all of the std library cases (os.Exit, t.Fatal, etc.) or summarize each function's "noreturned"-ness from a very small number of exits and the panic builtin (os.Exit). Lack of analyzing the std library in some environments (bazel? might be misremembering) might make this a bit moot anyways.

If we do summarize, we should consider rewriting "unreachable" using ctrlflow. There is an old TODO to do this.

[dominikh]

Analyzing functions runs the risk of causing false positives in the presence of build tags. For example when a function is implemented as a panicking stub only under some build tags. This has been a real source of false positives in Staticcheck, which uses the noreturned-ness when building the IR for functions to do what you are suggesting for ctrlflow. (It has also made some checks more powerful, but vet's standards for not causing false positives are even higher than Staticcheck's.)

There are ways to work around it, but they all introduce additional complexity.

[timothy-king]

Interesting. I would have guessed these are true positives for the current build tag though (with false negatives for other build tags.) Maybe I am just not putting the pieces together. Do you have any examples?

[dominikh]

true positives for the current build tag though (with false negatives for other build tags.)

In the strictest sense they are true positives, yes. But I'd argue that code shouldn't be flagged as unreachable unless it's unreachable for all build tags.

If you have

foo()
panicsOnSomeTags()
bar()

then telling the user that bar is unreachable isn't actionable advice. It's reachable on other build tags and we cannot remove it.

And if we make use of the information in ctrlflow or SSA then we can get follow-on effects. Consider a check that flags unnecessary variable assignments and the following code:

...
x = foo()
panicsOnSomeTags()
println(x)

the assignment is useless for some build tags, but not for all build tags, and we can again not remove it. That last example is derived from dominikh/go-tools#787.

When it comes to flagging dead/ineffective code, we IMO have to determine liveness by considering the union of all possible builds.