cmd/vet: reject x < 0 if x is unsigned

[davecheney]

What steps will reproduce the problem?

In the following code fragment

func f(x uint) int {
    if x < 0 {
        return 9000
    }
    return -9000
}

func main() {
    fmt.Println(f(10))
}

The x < 0 branch is unreachable because x is unsigned. 

What is the expected output? What do you see instead?

Expected, the compiler should refuse to compile this statement.

Actual, the compiler happily accepts it and probably eliminates the branch as
unreachable.

Please use labels and text to provide additional information.

See discussion: https://groups.google.com/d/topic/golang-dev/k3TQaEeGwyY/discussion

[minux]

Comment 1:

should the compiler also reject:
if false {}
?
i think this could make a cmd/vet warning, but
not a compiler error.

[davecheney]

Comment 2:

Thanks Minux,
I raised this issue in response to r's comment. I don't mind if this is tackled in the
compiler or in go vet.

[griesemer]

Comment 3:

Making this a language error is a very slippery slope.
We do have precedent for disallowing expressions that are guaranteed to fail (panic) at
run-time, such as divisions by (constant) 0, negative constant indices in index/slice
expressions, etc.
We don't have precedent for disallowing expressions that simply always produce the same
result (false in this case). For instance, should x*0 be disallowed? Probably not. And
even if we just consider this special case mentioned here, I can easily imagine
perfectly sensible code of the form
var shiftCount uint = ...
if shiftCount < WordSizeInBits - 32 { ...
where WordSizeInBits is a platform-specific constant. On a 32bit platform, the rhs would
evaluate to the constant 0. Should it still compile? I'd think so.
What about the transformation?
if shiftCount + 32 < WordSizeInBits { ...
etc.
Maybe the restriction should just be applied to expressions of this exact form: x < 0
where the rhs must be the literal 0. But then it should also (probably) apply to x >=
0 (again, for x uint). It's unclear where to stop.
Most importantly, there's a clear principle missing that would guide decision making
across the language.
I agree with Minux that a vet check might be in order instead (there's plenty of space
to explore with that alone...).

Labels changed: added release-none, repo-main, languagechange.

Status changed to Thinking.

[rsc]

I have seen errors like this from vet in the past. It seems OK to add something like this to vet assuming it passes the 3 rules in vet/README. It's not a backwards compatible language change though.

[JayNakrani]

golang.org/cl/34715 adds this to vet.

[gopherbot]

CL https://golang.org/cl/34715 mentions this issue.

[robpike]

You need to establish the three rules in the README, of which one seem clear (correctness).

Precision is tricky. I can see cases where due to a constant value being different on different architectures that this would be fine.

Also frequency remains to be demonstrated.

To me, this smells more like a lint issue.

[JayNakrani]

I can see cases where due to a constant value being different on different architectures that this would be fine.

Does this mean that the x < 0 branch (where x is unsigned), may not be dead/unreachable depending on a constant's representation in some architecture?

[robpike]

Yes. For example, consider the case where 0 is not a numeral but an architecture-dependent constant.

[gopherbot]

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)