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: reject x < 0 if x is unsigned #8601

Closed
davecheney opened this issue Aug 27, 2014 · 10 comments

Comments

Projects
None yet
8 participants
@davecheney
Copy link
Contributor

commented Aug 27, 2014

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

This comment has been minimized.

Copy link
Member

commented Aug 27, 2014

Comment 1:

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2014

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 rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc removed release-none labels Apr 10, 2015

@rsc rsc changed the title cmd/gc: compiler should reject x < 0 if x is unsigned cmd/compile: compiler should reject x < 0 if x is unsigned Jun 8, 2015

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2016

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.

@rsc rsc changed the title cmd/compile: compiler should reject x < 0 if x is unsigned cmd/vet: reject x < 0 if x is unsigned Sep 26, 2016

@rsc rsc added NeedsFix and removed LanguageChange Thinking labels Sep 26, 2016

dhananjay92 added a commit to dhananjay92/go that referenced this issue Dec 25, 2016

cmd/vet: warn for (unsigned) <= 0
Fixes golang#8601.

Change-Id: I019c70ef2b4eefb153a7f299c68fb226da59fa5c
@dhananjay92

This comment has been minimized.

Copy link
Member

commented Dec 25, 2016

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

@gopherbot

This comment has been minimized.

Copy link

commented Dec 25, 2016

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

@robpike

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2016

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.

@dhananjay92

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2016

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

@gopherbot

This comment has been minimized.

Copy link

commented Nov 10, 2017

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

@gopherbot gopherbot closed this Nov 10, 2017

@golang golang locked and limited conversation to collaborators Nov 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.