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

proposal: cmd/vet: flag accidental integer division inside float casts #36681

Closed
jktomer opened this issue Jan 21, 2020 · 8 comments
Closed

proposal: cmd/vet: flag accidental integer division inside float casts #36681

jktomer opened this issue Jan 21, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@jktomer
Copy link

@jktomer jktomer commented Jan 21, 2020

We have caught bugs of the form seen in kubernetes/kubernetes#83132 (review) a number of times in Kubernetes — namely, code that writes float64(some_int / another_int) when what is clearly meant is float64(some_int) / another_int.

It strikes me that the likelihood that anyone would ever intentionally lose precision with an integer divison and then cast the known-to-be-integral result to float64 is not necessarily exactly zero, but very low. In the rare cases where that is what's desired, it's easy to replace f := float64(a/b) with t := a/b; f := float64(t) and doing so more clearly communicates the intentional truncation.

The proposal is to have go vet flag expressions of the form float32(x / y) or float64(x / y) where x and y are expressions yielding a result of integral type.

@gopherbot gopherbot added this to the Proposal milestone Jan 21, 2020
@gopherbot gopherbot added the Proposal label Jan 21, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 22, 2020

I'm not convinced that float64(a / b) is always an error for integer a and b. It would be interesting to hear how many times such a check fired on the whole Kubernetes repo.

Basically I'm not sure this rises to the level of a vet check, which is a fairly high level. It might be more appropriate for some other checker.

@josharian
Copy link
Contributor

@josharian josharian commented Jan 22, 2020

@rsc rsc added this to Incoming in Proposals Jan 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jan 22, 2020

Adopting this check would mean forcing people to write float64(int(a/b)) for integer a, b to express "no I really mean what the code says". That's pretty unfortunate. In general we do assume that people write what they mean. Vet flags obvious mistakes, but this is not an obvious mistake. There is a way to write an integer divide and a way to write a float divide. Both are reasonable in different circumstances.

Note cmd/vet/README's "Precision" requirement, in particular that a check must not have a significant number of false positives. To move forward with this, we'd need evidence that essentially all instances of float64(a/b) are bugs.

@rsc rsc moved this from Incoming to Active in Proposals Jan 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Feb 5, 2020

We need evidence to move forward with this, and we don't have any evidence. This seems like a likely decline.

I also note that staticcheck doesn't check this either.

@rsc rsc moved this from Active to Likely Decline in Proposals Feb 5, 2020
@jktomer
Copy link
Author

@jktomer jktomer commented Feb 5, 2020

Sorry for the delay.

I mocked up a check to run over the k8s codebase, and came back with five hits, of which four are copies of each other (and harmless, if silly, since in context it amounts to float64((2 * some int) / 2)) and the other is definitely a bug.

This is possibly a slight underestimate since I was only looking for literal float{32,64} casts and there could be some other type with an underlying float type, but I'll accept that this doesn't meet the quality bar for a go vet check. Thanks for the review, though!

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 5, 2020

Even time.Second / time.Nanosecond is OK, since the time package ensures that there is an integral number of nanoseconds in a second.

@dominikh
Copy link
Member

@dominikh dominikh commented Feb 6, 2020

I also note that staticcheck doesn't check this either.

Correct, I share your concern about false positives.

Staticcheck has a related check, however, which flags math.Ceil(float64(<integer expression>)) (and identically for math.Floor). This catches one of the common instances of the mistake, with no real false positives to my knowledge. However, that check likely doesn't meet vet's frequency criterion.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 12, 2020

Thanks for taking the time to dig up numbers.
No change in consensus, so declining.

@rsc rsc closed this Feb 12, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.