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

Closed
josharian opened this issue May 19, 2020 · 10 comments
Closed

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

josharian opened this issue May 19, 2020 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@josharian
Copy link
Contributor

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

@josharian josharian added release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels May 19, 2020
@josharian josharian added this to the Go1.15 milestone May 19, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 19, 2020
@cespare
Copy link
Contributor

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

@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

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.

@jayconrod jayconrod added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels May 27, 2020
@jayconrod jayconrod modified the milestones: Go1.15, Go1.16 May 27, 2020
@josharian
Copy link
Contributor Author

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

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

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.

@jayconrod jayconrod added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels May 27, 2020
@jayconrod jayconrod modified the milestones: Go1.16, Go1.15 May 27, 2020
@gopherbot
Copy link

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

@smasher164
Copy link
Member

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.

@toothrot toothrot removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 10, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jun 16, 2020
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>
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants