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

Abirdcfly · 2022-07-20T09:37:37Z

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 ./...

The text was updated successfully, but these errors were encountered:

mvdan · 2022-07-20T12:20:21Z

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

dominikh · 2022-07-20T12:25:43Z

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 · 2022-07-20T13:23:22Z

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 · 2022-07-20T20:30:48Z

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 · 2022-07-20T20:53:08Z

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 · 2022-07-20T21:25:30Z

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 · 2022-07-21T06:45:32Z

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.

Abirdcfly mentioned this issue Jul 20, 2022

add testing.FailNow and related function to unreachable check mgechev/revive#710

Closed

Abirdcfly changed the title ~~cmd/vet: should check unreachable code after t.SkipNow or t.FailNow or related functions~~ cmd/vet: should check unreachable code after t.FailNow or related functions Jul 20, 2022

toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 20, 2022

toothrot added this to the Backlog milestone Jul 20, 2022

timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jul 20, 2022

Abirdcfly mentioned this issue Jul 21, 2022

clean unreachable code kubernetes/kubernetes#111228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Abirdcfly commented Jul 20, 2022 •

edited

mvdan commented Jul 20, 2022

dominikh commented Jul 20, 2022

Abirdcfly commented Jul 20, 2022

timothy-king commented Jul 20, 2022

dominikh commented Jul 20, 2022

timothy-king commented Jul 20, 2022

dominikh commented Jul 21, 2022

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

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

Comments

Abirdcfly commented Jul 20, 2022 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Jul 20, 2022

dominikh commented Jul 20, 2022

Abirdcfly commented Jul 20, 2022

timothy-king commented Jul 20, 2022

dominikh commented Jul 20, 2022

timothy-king commented Jul 20, 2022

dominikh commented Jul 21, 2022

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

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

Abirdcfly commented Jul 20, 2022 •

edited

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

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