Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: go vet complains about redundant and/or #46046

Closed
fishy opened this issue May 7, 2021 · 3 comments
Closed

cmd/go: go vet complains about redundant and/or #46046

fishy opened this issue May 7, 2021 · 3 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@fishy
Copy link

fishy commented May 7, 2021

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

$ go version
go version go1.16.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

Note: I removed GOPRIVATE line

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fishy/.cache/go-build"
GOENV="/home/fishy/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/fishy/.gopath/pkg/mod"
GONOPROXY="github.snooguts.net"
GONOSUMDB="github.snooguts.net"
GOOS="linux"
GOPATH="/home/fishy/.gopath"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/fishy/demo/go.mod"
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-build868923192=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Minimal reproducible code:

package demo

func Return(cond1, cond2 int) bool {
        return true &&
                cond1 == 0 &&
                cond2 == 0 &&
                true
}

func If(cond1, cond2 int) {
        if false ||
                cond1 == 0 ||
                cond2 == 0 ||
                false {

                // TODO: do something
        }
}

run go vet against it complains:

$ go vet foo.go
# command-line-arguments
./foo.go:4:9: redundant and: true && true
./foo.go:11:5: redundant or: false || false

What did you expect to see?

We use this style because this allows us to generate better diff if we need to add/remove conditions in the future, it's in the same spirit of always allow trailing comma on arg list, even for the last arg. So we think we should allow this style through go vet.

We also have some old, similar code that go vet does not complain about. I cannot create a minimal reproducible example for it that go vet does not complains, but the pseudo code is something like this (this is the implementation of our health check handler):

func (s *service) IsHealthy(_ context.Context, probe baseplate.IsHealthyProbe) (bool, error) {
	switch probe {
	default:
		// For anything unknown, fallback to READINESS
		fallthrough
	case baseplate.IsHealthyProbe_READINESS:
		// Have false at beginning and end to make future diffs easier.
		return false ||
			!s.dependency1.IsHealthy() ||
			!s.dependency2.IsHealthy() ||
			false, nil

	case baseplate.IsHealthyProbe_STARTUP:
		// We don't use startup probes right now,
		// but if we started to use them they should fallback to liveness.
		fallthrough
	case baseplate.IsHealthyProbe_LIVENESS:
		return true &&
			s.dependency3.IsHealthy() &&
			true, nil
	}
}

What did you see instead?

@mvdan
Copy link
Member

mvdan commented May 7, 2021

That's an interesting approach, and I generally agree that simpler diffs are better.

I'm not sure I'm convinced this is a good idea and something that tools like vet should encourage, though. You're almost fighting against Go's syntax here, adding no-op expressions as if they were opening and closing tokens. And the vet warning is right, after all. If we simply disabled this check if the two true expressions are on different lines, we could introduce false negatives.

If you really want to work around the vet warning, I'm sure there are workarounds you could try on your end, like using a variable var _true = true or whichever name you like best.

@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 7, 2021
@fishy
Copy link
Author

fishy commented May 7, 2021

Yea currently changing one of the true into !false is enough to fool go vet, but we also have some old code didn't use that trick and go vet didn't complain about that either (we added some new code and go vet starts to complain, that's how we realized there's such a rule in go vet). We haven't figured out what's the reason the old code passed go vet yet.

I think my question is more on, what kind of bugs does this go vet rule prevent? Is it really some serious bug, or is it more like "you are using true && true, maybe you have a typo somewhere" kind of warning? I guess I just need to understand more about the background of this rule and what bad things can happen if we remove it.

@ianlancetaylor
Copy link
Contributor

The vet check was introduced for #7622. It specifically looks for redundant conditional expressions, which is what you have. It's not really intended for true && true, it's intended for a && a, but because of that it picks up true && true.

This is intended behavior so I'm going to close this issue.

@golang golang locked and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants