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

proposal: cmd/vet: flag invalid json struct tags #41769

Closed
andig opened this issue Oct 3, 2020 · 7 comments
Closed

proposal: cmd/vet: flag invalid json struct tags #41769

andig opened this issue Oct 3, 2020 · 7 comments
Labels
Projects
Milestone

Comments

@andig
Copy link
Contributor

@andig andig commented Oct 3, 2020

I've recently written codes for json marshaling, containing:

`json:"omitempty"`

as tag. The error only became obvious when I've added another field with the same tag. It has to be:

`json:",omitempty"`

I was wondering if vet could/should detect those (and potentially more invalid tags)?

@ianlancetaylor ianlancetaylor changed the title cmd/vet: proposal to flag invalid struct tags cmd/vet: proposal to flag invalid json struct tags Oct 3, 2020
@ianlancetaylor ianlancetaylor changed the title cmd/vet: proposal to flag invalid json struct tags proposal: cmd/vet: flag invalid json struct tags Oct 3, 2020
@gopherbot gopherbot added this to the Proposal milestone Oct 3, 2020
@gopherbot gopherbot added the Proposal label Oct 3, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 3, 2020

cmd/vet does already flag certain kinds of invalid struct tags. This would extend that to look specifically for invalid json struct tags. And, logically, xml, etc.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Oct 3, 2020
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Oct 4, 2020

One catch here will be tooling outside the Go toolchain that uses these tags. For example, easyjson adds an intern directive to the json tag to enable interning for fields on unmarshal. If go vet disallows this it might have a negative effect, though I don't know the pervasiveness of adding to the standard library's tags versus say, other tooling using their own tags like easyjson:"intern".

@dominikh
Copy link
Member

@dominikh dominikh commented Oct 4, 2020

I'm not sure how much there is to do here? Vet already flags duplicate tags: ./bar.go:5:2: struct field State repeats json tag "omitempty" also at bar.go:4, and I don't think vet can flag a field simply for being called omitempty, as that would be a false positive if I actually wanted the field to be named that.

@andig
Copy link
Contributor Author

@andig andig commented Oct 4, 2020

I don't think vet can flag a field simply for being called omitempty, as that would be a false positive if I actually wanted the field to be named that.

That's the question imho. Might be the usefulness is outweighed by this drawback.

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 4, 2020

cc @mvdan

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 4, 2020

I agree with @dominikh, vet has a high standard for no false positives, so I don't think it can be added. The buggy example you shared originally would be very quickly spotted with a test or by running the code, I presume.

I'm also not sure I get the title of the issue. json:"omitempty" is perhaps confusing, but it's not an invalid tag.

@andig
Copy link
Contributor Author

@andig andig commented Oct 4, 2020

Sorry for the confusion, the title was still on the notion of omitempty affecting decoding. Closing this one on the basis of not generating false positives as vet standard. Thanks for the discussion!

@andig andig closed this Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants