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: improve error message for string(int) warning #39151

Open
josharian opened this issue May 19, 2020 · 9 comments
Open

cmd/vet: improve error message for string(int) warning #39151

josharian opened this issue May 19, 2020 · 9 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented May 19, 2020

Migrated here from #32479 (comment).

The error message is:

x/x.go:9:14: conversion from int to string yields a string of one rune

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

@cespare
Copy link
Contributor

@cespare cespare commented May 20, 2020

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.

@smasher164
Copy link
Member

@smasher164 smasher164 commented May 23, 2020

@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:

"conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprintf("%d", x)?)"
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 27, 2020

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 cmd module until 1.16 development starts.

@josharian
Copy link
Contributor Author

@josharian josharian commented May 27, 2020

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.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 27, 2020

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 std and cmd should be frozen, but I'm not confident that's actually the case, and there may be some risk here.

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

@cespare
Copy link
Contributor

@cespare cespare commented May 27, 2020

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.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 27, 2020

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.

@gopherbot
Copy link

@gopherbot gopherbot commented May 30, 2020

Change https://golang.org/cl/235797 mentions this issue: go/analysis: improve error message for string(int) warning

@smasher164
Copy link
Member

@smasher164 smasher164 commented May 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
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.