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: improve error message for string(int) warning #39151
Comments
FWIW I'd be fine with @alandonovan's suggestion in #32479 (comment). The key problem with the error message is if you interpret it from the viewpoint of an experienced Go user who is very aware of what string(int) does and has used it intentionally a few times, the response to being told "conversion from int to string yields a string of one rune" is probably "yes...and?" Alan's message which mentions "not a string of digits" makes it clear what the unintentional mistake with this kind of code would be. |
@cespare On further thought, your justification for how a Go user might see it makes sense. A user who knows that string(int) produces a string of one rune would know to change it to a string(rune(int)). OTOH, a user who uses it to produce a string of digits would be informed that their usage is incorrect. Quoting @alandonovan's comment, I think the following message is appropriate:
|
Let's push this back to 1.16. The release team has asked us to be stricter about what goes in after the freeze, so we're holding back most changes except for documentation and significant bug fixes. I think it's probably okay (please correct me if I'm wrong) for a fix to land in x/tools right now, but I wouldn't expect x/tools to be upgraded in the |
We’re talking about changing the text of an error message, if I understand correctly. And an error message in a vet check that is new to 1.15, to try to prevent confusion. That strikes me as about as close to documentation as you can get. |
Changing documentation is pretty much zero risk. Changing this message requires updating and vendoring x/tools and fixing any tests that expect the old message. In theory, the packages in x/tools depended on by My interpretation of Go Release Cycle and Planning Go 1.15 is that unless this is approved by the release team, this has to wait for 1.16. (Personally, I would like to see changes like this allowed during the freeze. It's frustrating to keep track of a lot of small pending changes for months, and it would be great to fix things sooner. That said, the goal of the stricter freeze policy is to avoid delaying the next development cycle, and I'm very much in favor of that.) |
The value of improving this error is to help the people who will be momentarily confused when this new vet check starts flagging their code, which is going to happen as soon as Go 1.15 lands. If we can't change the error until 1.16, we might as well not bother; by that point, people will have fixed their code. Fixing this in 1.16 only helps the tiny number of people who skip over the 1.15 release entirely. |
Ah, I missed that this is a new pass in 1.15. I just saw the date that #32479 was opened and assumed it was old. Fixing features introduced in this cycle is definitely in scope for the freeze. I'll change the milestone back. Sorry for the noise. |
Change https://golang.org/cl/235797 mentions this issue: |
I sent CL 235797 to update the error message in go/analysis with the one I mentioned above. I'll send a follow-up vendoring CL after it's merged. |
To an experienced Go user, the previous error message doesn't describe the error--it merely states a fact. The new error message asks if the user meant fmt.Sprint(x). If they used the conversion correctly, the suggested fix to replace string(int) with string(rune(int)) is understandable and valid. On the other hand, if they used it to print a digit representation, asking that in the error message further emphasizes the mistake. Updates golang/go#39151. Change-Id: Iab9cdcaf53e9ca134285246fad0546d6f24c3983 Reviewed-on: https://go-review.googlesource.com/c/tools/+/235797 Run-TryBot: Akhil Indurti <aindurti@gmail.com> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Change https://golang.org/cl/238157 mentions this issue: |
Migrated here from #32479 (comment).
The error message is:
But we can offer much more helpful information. See the discussion near the end of #32479.
I took a guess at milestoning and labels.
cc @ianlancetaylor @alandonovan @cespare @smasher164
The text was updated successfully, but these errors were encountered: