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

[josharian]

Migrated here from https://github.com/golang/go/issues/32479#issuecomment-628818192.

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]

FWIW I'd be fine with @alandonovan's suggestion in https://github.com/golang/go/issues/32479#issuecomment-628821401.

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]

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

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]

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]

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]

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]

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]

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

[smasher164]

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.

[gopherbot]

Change https://golang.org/cl/238157 mentions this issue: cmd: update golang.org/x/tools