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: json.Number is easy to misuse (vet check?) #24001

Closed
bsiegert opened this issue Feb 21, 2018 · 2 comments
Closed

cmd/vet: json.Number is easy to misuse (vet check?) #24001

bsiegert opened this issue Feb 21, 2018 · 2 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bsiegert
Copy link
Contributor

What version of Go are you using (go version)?

1.10

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

linux-amd64, but any architecture really

What did you do?

I reviewed some code that contained an erroneous instantiation of a json.Number. See https://play.golang.org/p/w6cy7I1A--Q.

What did you expect to see?

The author of that code was trying to store the value 1 in a json.Number by using json.Number(1).

What did you see instead?

The type is defined as

type Number string

Thus, the following happens: the 1 is interpreted as a rune, and the string is initialized from the rune. Thus, instead of storing a 1, it stores an ASCII 0x1 character. The correct way of initializing this constant would be json.Number("1").

This seems like a good fit for a vet check, as it probably is never what was intended: catch casting a number (constant? int? rune?) into a json.Number and flag it.

I looked through our internal repo and found no uses of Number instances being initialized from runes or numbers.

Would such a vet check be accepted?

@bradfitz bradfitz changed the title json.Number is easy to misuse (vet check?) cmd/vet: json.Number is easy to misuse (vet check?) Feb 21, 2018
@bradfitz
Copy link
Contributor

New vet checks are usually denied for a variety of reasons.

It might be accepted in @dominikh's https://github.com/dominikh/go-tools/tree/master/cmd/staticcheck though.

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 21, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Feb 21, 2018
@rsc
Copy link
Contributor

rsc commented Mar 26, 2018

Really a dup of #3939.

@rsc rsc closed this as completed Mar 26, 2018
@golang golang locked and limited conversation to collaborators Mar 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants