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/vet: potential false positive in the "suspect or" check #28446

Open
cznic opened this issue Oct 28, 2018 · 14 comments
Open

cmd/vet: potential false positive in the "suspect or" check #28446

cznic opened this issue Oct 28, 2018 · 14 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cznic
Copy link
Contributor

cznic commented Oct 28, 2018

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

go version go1.11.1 linux/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jnml/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jnml"
GOPROXY=""
GORACE=""
GOROOT="/home/jnml/go"
GOTMPDIR=""
GOTOOLDIR="/home/jnml/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build777349074=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Using github.com/cznic/ql, branch file2, checked out at 3497607bfaba518eab540d317a6c168396b8002f.

==== jnml@4670:~/src/github.com/cznic/ql> go test
# github.com/cznic/ql
./file2.go:37: suspect or: szKey != szVal || szKey != szBuf
FAIL	github.com/cznic/ql [build failed]
==== jnml@4670:~/src/github.com/cznic/ql> 

What did you expect to see?

Build succeeds, tests are executed and failures reported.

What did you see instead?

Build fails.

Additional info

The line go vet does not like is here

func init() {
	if al := cfile.AllocAllign; al != 16 || al <= binary.MaxVarintLen64 {
		panic("internal error")
	}

	if szKey != szVal || szKey != szBuf { // <-- line 37
		panic("internal error")
	}
}

const (
	magic2      = "\x61\xdbql"
	szBuf       = szKey
	szKey       = cfile.AllocAllign // BTree
	szVal       = szKey             // BTree
	wal2PageLog = 12                //TODO tune pagelog
)

Line 37 tests for equality of the three values szKey, szValue, szBuf which other code depends on as it would have to be more complicated for the general case and the equality allow simplifying it. I see nothing wrong about the line, I donť think vet should reject it.

The vet test is located here

// checkSuspect checks for expressions of the form
//   x != c1 || x != c2
//   x == c1 && x == c2
// where c1 and c2 are constant expressions.
// If c1 and c2 are the same then it's redundant;
// if c1 and c2 are different then it's always true or always false.
// Exprs must contain only side effect free expressions.
func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) {
...
}

The comment if c1 and c2 are different then it's always true or always false. is correct, but that does not imply something is wrong.

There's nothing wrong in implementing

a == b == c

in Go as

a == b && a == c

Where the negation, rejected by vet, is

a != b || a != c

Workaround: vet is happy if the expression is rewritten as

szKey != szVal || szVal != szBuf

even though the two versions are logical identities.

@dominikh
Copy link
Member

dominikh commented Oct 28, 2018

a != b || a != c is always true, a cannot equal both b and c, so one of the two branches has to be true.

The negation you're looking for is !(a == b && a == c) or a != b && a != c

Edit: never mind, tired eyes should not review code.

@mvdan
Copy link
Member

mvdan commented Oct 28, 2018

I'm not sure I follow this bug report. All three names are constants, and they're equal, so I presume that vet is seeing the condition as a != b || a != b, or even as a != a. That is, the condition will always be false.

It's not that vet doesn't like conditions that are always true or false. I believe this check is to spot human error when writing repetitive boolean expressions.

You're right that a != b || a != c in general is a valid boolean, but I don't think vet would flag that condition if the names involved were variables with unknown values.

@mvdan mvdan changed the title go test(vet): suspect or: false positive cmd/vet: potential false positive in the "suspect or" check Oct 28, 2018
@mvdan mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 28, 2018
@cznic
Copy link
Contributor Author

cznic commented Oct 28, 2018

All three names are constants, and they're equal, so I presume that vet is seeing the condition as a != b || a != b, or even as a != a. That is, the condition will always be false.

... will be always false iff the three constants are equal. And that's the very point of the test.

If anyone later changes any of the {szKey,szVal,szBuf} constants, each of which tunes an independent thing, in a way such that they become not equal, the condition will become true and the init() will panic as desired because the code simplifications rely on that invariant. So the constants must remain the same or the code must be adjusted to handle them not being equal.

@josharian
Copy link
Contributor

My quick 2c: This is a false positive. But it’s the first one like we have ever had reported (and it has been years), and I don’t see a clean way to fix it, and there’s an easy workaround available. So let’s just take the workaround for now.

@mvdan
Copy link
Member

mvdan commented Oct 29, 2018

... will be always false iff the three constants are equal. And that's the very point of the test.

Yes - that's why I started the paragraph with "all three names are constants" :)

I don't think I've ever seen an init check to see if constants have been tampered with. Is that something that you expect your users to do? I think this is why this false positive has never been brought up before, like Josh says.

@cznic
Copy link
Contributor Author

cznic commented Oct 29, 2018

Is that something that you expect your users to do?

I'm the prime suspect any time in the future. Been there, done that ;-)

@mvdan
Copy link
Member

mvdan commented Oct 31, 2018

My previous comments weren't completely correct. The check documentation reads:

checkSuspect checks for expressions of the form
  x != c1 || x != c2
  x == c1 && x == c2
where c1 and c2 are constant expressions.
If c1 and c2 are the same then it's redundant;
if c1 and c2 are different then it's always true or always false.
Exprs must contain only side effect free expressions.

So this kind of expression is exactly what the vet check is supposed to flag. I mostly agree with Josh, that it should be easy enough for now to avoid this pattern and see if there are any other false positives reported in the future.

I was going to pull in the boolean check author for some second thoughts, but he's ahead of me :)

commit 7da5f193b77edcfcf26ac513864432001693735a
Author: Josh Bleecher Snyder <josharian@gmail.com>
Date:   Wed Jul 2 10:39:57 2014 -0700

    go.tools/cmd/vet: detect stupid boolean conditions

@cznic
Copy link
Contributor Author

cznic commented Oct 31, 2018

I still don't get where from comes the idea that there's anything wrong with the above quoted expressions.

Please ELI5 why, for example, a test for violating invariant a == b && a == c, written as if a != b || a != c { ... } should be rejected by vet, given a, b and c are constants, considering constants may change during code development/debugging etc.

If some/all of a, b, c would be constant values (literals), ie. not constant names, then the rejection would be obviously correct. (Like in a == 42 && a == 278)

@mvdan
Copy link
Member

mvdan commented Oct 31, 2018

I think it doesn't distinguish named constants from unnamed ones. Perhaps it should - that would cover this false positive, but it might introduce some false negatives. It looks like none of the current tests use user-defined constants, so that might work. nil should still trigger the check, of course.

@josharian thoughts?

@josharian
Copy link
Contributor

I think it doesn't distinguish named constants from unnamed ones.

That's correct.

I think it'd be useful to gather some data from a corpus about the nature of the constant expressions when this check fires.

@cbeuw
Copy link

cbeuw commented Dec 25, 2018

I think the ultimate solution is to make this a warning. Along with unused import/variables, short edits in code for debugging is painful in Go.

@dfang

This comment has been minimized.

@ALTree
Copy link
Member

ALTree commented Jun 21, 2019

@dfang This is not related to the issue reported in this thread, assuming authType is a variable.

This:

authType != "scan" || authType != "web"

is equivalent to true for all possible values of authType, so vet is right: that condition is suspicious. It's not checking anything. It's always true.

Moreover, this:

!(authType == "scan" || authType == "web")

is not equivalent to

authType != "scan" || authType != "web"

so your change is not correct. For example, if authType is "scan" the first condition if false, but the second is true.

@michaelcourcy
Copy link

michaelcourcy commented Dec 4, 2019

x x!= a x!=b x!= a or x!=b x!= a and x!=b
a 0 1 1 0
b 1 0 1 0
c 1 1 1 1
a == b 0 0 0 0

Falling in the last line is weird because I don't see why you would implement x == a == b but @cznic is right the column x!= a or x!=b is not always true.

bbrks added a commit to couchbase/sync_gateway that referenced this issue Jun 11, 2021
adamcfraser pushed a commit to couchbase/sync_gateway that referenced this issue Jun 14, 2021
* Combine all gocb loggers into a shared method

* Work around vet false-positive for const equality checks.

See github.com/golang/go/issues/28446 for detail

* Move gocb log level equality checking to a test
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

10 participants