proposal: cmd/vet: warn about structs marked json omitempty #51261
Comments
|
Scanning all modules, I found ~73k fields where the type was either That's pretty significant evidence that this is a highly frequent problem. |
|
Is there any reason to limit this to time.Time instead of applying to all structs? |
|
I advocate expanding it to all structs. |
|
Yes this shall be extending any non-primitive and non-reference types, mainly struct. Interestingly, the official document specifies:
which actually indicates that a conflicting check that is supposed to report usage of |
|
I don't write much JSON code, so take my thoughts with a grain of salt, but this problem seems to indicate that there should be some form of [1] Admittedly this is still a problem for |
I agree there should be some type of |
It is a little bit confusing what "empty" means. Currently:
Regarding a composite object, what does "empty" or "zero" mean? Consider these cases:
Users seems to assume that (2) is "empty" when using "omitempty". |
|
I think this issue getting off topic. There's a lot of valid questions with regard to |
|
One concern I have is how to educate users. It seems like the most likely cause is the user misunderstanding of how structs interact with encoding/json with omitempty being a symptom. Vet's message have historically been quite terse. I am hesitant to give suggestions like "rewrite time.Time to *time.Time here" without giving an explanation as to why (and can cause other bugs during the transition). Similarly a message like "struct field %s is not emitted by encoding/json so the omitempty tag is redundant" seems like it would often [usually?] result in the omitempty tag being removed which does not help the underlying confusion. Do folks have a suggestion for what to tell the user in the diagnostic? What I really want to do is point users at a longer discussion of the issue (like 1-2 paragraphs describing the issue and possible solutions), but to the best of my knowledge this has not done this before in vet. |
|
Change https://go.dev/cl/388574 mentions this issue: |
adonovan
added
the
Analysis
I've proposed an implementation for vetting on omitempty in the general case where this is currently a no-op: #63541
If this issue needs more consideration I'd appreciate if we could reopen #63539 with it's narrower focus. |
|
Change https://go.dev/cl/535416 mentions this issue: |
|
Change https://go.dev/cl/535515 mentions this issue: |
… types Fixes golang/go#51261 Change-Id: Ic71f614acd3a78a9db73483ccefe613cb7f66e5d
… types Fixes golang/go#51261 Change-Id: I99c3a0afc5171777b39f8288e2159f8554151e43
|
I ran face first into this very bug in the x/oauth2 package: https://pkg.go.dev/golang.org/x/oauth2#Token |
|
@rakyll @shinfan @codyoss per https://dev.golang.org/owners - would it be OK to send a patch to make |
|
@mvdan I think even though it is v0 The whole TokenSource interface is quite pervasive in the oauth space. I think we should not make a breaking change here. With how this struct is used I don't think data is actually serialized all that much -- it is much more common to deserialize into it. Does that seem worth the ripple effects imo. |
Has this issue been expanded to include flagging any inline-struct field that is (uselessly) tagged We just fixed a bug that bit us in production because our product spent years making API calls, sending JSON that included an extra property with a |
|
@mvdan you're not the first one ;) I'm absolutely puzzled why it seems so hard to move this issue forward? |
|
I ran a candidate implementation on a large selection of Go packages with >=10 imports. I looked at an earlier sample and all of the reports looked accurate. We may want to treat generated code specially though. The candidate implementation is now gated to only warn when it knows that ast.IsGenerated is false. The reports in hand written code all look like warnings someone should fix. The diagnostics in generated code are correct, but I am less confident users will want to take action by changing the generated code. That code might be written over or the generator is hard to fix, etc. The fix is to change the generator, which is a bit of a slower process and not very CICD friendly. Hence the additional IsGenerated requirement in the prototype I added. Examples this would exclude: 1, 2. Some diagnostics are in generated code that do not yet satisfy IsGenerated (example), but we do need to pick some standard and that is ast.IsGenerated. I think it would be okay to release the omitempty check in go version N, and remove the IsGenerated check in go version N+1. So I don't think this has to be a requirement forever. Here are 100 randomly selected diagnostics out of 29697 (no ast.IsGenerated files): |
|
Here are 100 randomly selected diagnostics out of 35104 (allows ast.IsGenerated files): |
|
It looks like #11939 was just closed in favor of a new tag, ETA: ...which, I believe, is exactly what the "candidate implementation" / https://go-review.googlesource.com/c/tools/+/388574 was supposed to do. That review is marked as "abandoned" -- any chance of getting the ball rolling again? It looks like this proposal here has to be resolved first... |
ianlancetaylor
changed the title
|
This proposal has been added to the active column of the proposals project |
|
Awesome! The fact that even Go standard library likes to promote this mistake speaks for itself. Follow-up proposal would be if "fixing" go standard library goes against compatibility promise ( |
My instinct is that, practically speaking, this is far more of a problem for struct fields than anything else, but I would love to see some data on that. @timothy-king , since you have the pipeline for checking for |
|
I should note that the v2 experiment alters the definition of To be specific:
Under the v2 definition,
Under the v2 definition, |
|
So.. if we add this vet check now, it'll mess up json/v2 in the future? That's unfortunate. I believe vet can see all of the conditions for |
|
We have 4 conditions:
I believe we should initially implement this vet check in such a way that it only detects case 4. More concisely,
In the future (after 2 releases), we can expand it to detect case 2, where:
|
|
Thanks. That logic all seems fine. |
Thinking a little more about this, I believe this should be written in terms of underlying types and exclude named types with |
|
Sorry, another thought: In encoding/json v1, there's nothing wrong with using |
|
Discussing this with the proposal committee, we're skeptical of de facto deprecating the current behavior of When and if |
|
Partly just because I did the work of writing it down, if we were to include the Vet would report a meaningless use of encoding/json's
In case 2c, vet would suggest using |
|
Have all remaining concerns about this proposal been addressed? The proposal is to add a vet check that reports meaningless uses of encoding/json's
|
Marking a struct field of type
time.Timeas json omitempty has no effect. You must use a*time.Timefor it to work (which is wire compatible). Vet should detect this situation and perhaps suggest*time.Timeinstead.This might be reasonably extended to any struct type; see the linked omitzero proposals for more discussion. If an omitzero proposal is accepted, vet could switch to suggesting the use of omitzero rather than suggesting a pointer type.
cc @dsnet @robpike @dominikh
The text was updated successfully, but these errors were encountered: