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: consider adding check for incorrect bitSize on strconv.ParseFloat (and similar functions) #42389

Closed
benhoyt opened this issue Nov 5, 2020 · 5 comments
Labels
Projects
Milestone

Comments

@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Nov 5, 2020

There is a non-trivial amount of code that calls strconv.ParseFloat with a bitSize of 10 (or even 0) instead of 32 or 64, presumably due to a copy-n-paste error from ParseInt, or thinking the second argument was a base instead of a bit size. We were going to return an error in these cases, but doing that would probably break existing code, so we went back on that (issue #42297 has more context and code links).

Instead of returning an error, I've put up CL 267600 with a go/analysis pass (intended for "go vet") that checks for this. My questions (copied from #42297) are:

  1. This just checks ParseFloat and ParseComplex. Do we want to add checks for other functions that take a bitSize as well? It'll be easy to add: AppendFloat (32, 64), FormatComplex (64, 128), FormatFloat (32, 64), ParseInt, ParseUint (0, 8, 16, 32, 64).
  2. I'm pretty sure this satisfies the correctness and precision criteria here ... but does it satisfy the frequency criteria? Is the cost of running this check (I don't have a good sense of that at all, but presumably it's small) worth the benefit? This is flagging incorrect code, but it's probably not code that will actually have bugs in it (they may have used 10 instead of 64, but it'll still parse the number correctly and return a float64).
@ianlancetaylor ianlancetaylor changed the title cmd/vet: consider adding check for incorrect bitSize on strconv.ParseFloat (and similar functions) proposal: cmd/vet: consider adding check for incorrect bitSize on strconv.ParseFloat (and similar functions) Nov 5, 2020
@gopherbot gopherbot added this to the Proposal milestone Nov 5, 2020
@gopherbot gopherbot added the Proposal label Nov 5, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 5, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2020

@robpike
Copy link
Contributor

@robpike robpike commented Nov 6, 2020

Strikes me as possibly above the bar but marginally at best, and over time I expect the frequency to drop. Instead I might advocate a single push to clean up all the public instances. Should be easy to find.

@benhoyt
Copy link
Contributor Author

@benhoyt benhoyt commented Nov 6, 2020

@benhoyt I'm not opposed to "a single push to clean up all the public instances" (though that can be a very time-consuming process to drive: N pull requests across N repositories with N different styles of contributing). However, why do you expect the frequency of this to drop over time? I'd expect it to be constant as this seems to be a copy-n-paste type of mistake (or even grow, given with increased Go usage more newbies are copying-and-pasting stuff).

That said, I don't mind scrapping the vet check, either. With the ParseFloat(s, 10) behavior now forever immortalized with regression tests :-), this code -- while incorrect -- isn't really going to cause issues.

@robpike
Copy link
Contributor

@robpike robpike commented Nov 6, 2020

I don't see much evidence for it being a copy-and-paste error, but if you say it is I have no reason to doubt you. In any case, since passing 10 instead of 64 has no effect, is it even a bug?

@benhoyt
Copy link
Contributor Author

@benhoyt benhoyt commented Nov 6, 2020

It being a copy and paste error was an educated guess, based (so to speak) on the fact that 10 is usually the second argument in a ParseInt call, so likely copied from there. In any case, yeah, I think now that the dust has settled and because of the tests we've added, this is a mistake but not a bug. Closing.

Was fun to write a vet check though. :-)

@benhoyt benhoyt closed this Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants