cmd/vet: warn about unconditional return in a loop

[vhosakot]

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

$ go version
go version go1.9.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

MacOS Seirra.

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/vhosakot/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.9.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.9.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/83/1jxkgzw91vg810dqdlvqg9x80000gn/T/go-build352630876=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Ran go tool vet on the following code that has an unconditional return inside a for loop:

func test() error {
	for i := 0; i < 10; i++ {
		return nil
	}
	return nil
}

What did you expect to see?

An error or a warning that the loop exits unconditionally after just one iteration similar to the error SA4004 The loop exits unconditionally after one iteration in the Go staticcheck tool in https://staticcheck.io/docs/staticcheck.

What did you see instead?

No error or warning.

[ianlancetaylor]

The guidelines for vet checks (see cmd/vet/README) are frequency, correctness, and precision. This test is precise, but I doubt it is frequent. It is also not always correct; some code chooses a random key from a map by writing

    for k := range m {
        return k
    }

So I don't think this meets the criteria for adding to cmd/vet.

[dominikh]

For correctness, the check can easily be improved to recognize that pattern, which is what staticcheck does. However, this still leaves the possibility for contrived code that is correct but strange.

As for frequency, you're correct. It does occur in real code, but not very often.

(Pedantry: it's an arbitrary key, not a random one)

[mvdan]

(Pedantry: it's an arbitrary key, not a random one)

Care to explain? I know map ranges aren't truly random, but you should still get a semi-random key from the set of keys.

[dominikh]

@mvdan The spec leaves the order undefined. The order could well be insertion order or alphabetical order or some other order. And even in this implementation, the distribution of random iteration isn't very good.

IOW, if you want to get some key (i.e. arbitrary), the one-iteration loop is fine. If you want a random key – one that is chosen fairly – it is not fine.

[cespare]

(The most common place where I've seen this kind of code is where you know the map has 1 element and you want to get it out.)

[vhosakot]

@ianlancetaylor The code below is perfect and meant to run only once and returns some key of the map m. It is not a loop and there is only one iteration. No warning/error is needed in this case.

 for k := range m {
        return k
    }

I opened this issue about the case below where the code is supposed to run multiple times in the loop but does not due to the unconditional return. A warning or an error for this case would be good I think.

func test() error {
	for i := 0; i < 10; i++ {
		return nil
	}
	return nil
}

Yes, this code may not be common, is mostly a mistake, but could be caught by go tool vet IMO. If missed, such code can lead to dangerous bugs.

[ianlancetaylor]

@vhosakot If the problem is not common, it shouldn't go into vet. There are other static checking tools that have different criteria.