x/tools/cmd/vet: check JSON fields have the same struct tag

[albrow]

I'm on go version 1.5.0 on Mac OS X. The same problem occurs in the playground, which right now uses go 1.5.1. json.Marshal does not appear to encode any fields that have conflicting json struct tags. For example, the following type will get marshaled to an empty json object ("{}"):

type MyStruct struct {
    A string `json:"a"`
    B int    `json:"a"`
}

See this playground example. This just bit me in a real world application with a fairly large codebase and was hard to track down.

After re-reading the documentation for json.Marshal, it appears that this behavior is consistent with the documentation. However, I would consider this a silent failure, and I would like to consider having json.Marshal return an error for this specific case.

This in contrast to embedded structs with conflicting field names or struct tags. Here's a playground example with embedded structs. In this case, json.Marshal chooses the field in the parent struct. I believe that behavior is correct and consistent. I can imagine many cases where an embedded struct will have field names which "conflict" with the parent struct. In fact, it's a pattern I have used on occasion to share common behavior and data structures.

The case I am presenting is different, because I cannot imagine sane code which would assign conflicting struct tags to two different fields in the same struct. This was clearly an error on my part and can be easily fixed. The json package should help me, and other developers who make the same mistake, discover it and fix it as quickly as possible.

[minux]

I think the behavior is primarily for consistent with conflicting struct tags at the same level in different embedded structs. http://play.golang.org/p/syh3z4m_OX

[albrow]

@minux, thanks for your reply. I have looked at your example and also took the time to read through https://github.com/golang/go/issues/11006, which is more similar to this issue than I originally thought.

I understand now that both the example you provided and the original example I provided match the documented behavior for json.Marshal. I don't necessarily agree with that behavior, but I understand that it cannot be changed without breaking backwards compatibility. For reference, the relevant part of the documentation is rule 3) concerning the visibility of struct fields.

3) Otherwise there are multiple fields, and all are ignored; no error occurs.

If possible, I think we should consider changing this in go 2.0. It is relatively common to have conflicting field names or struct tags in embedded struct types, and I am okay with leaving behavior as is for those cases. However, having two fields with conflicting struct tags in the same struct type, as in my original example, is a blatant mistake. There is no sane reason a developer should ever do this, and the json package should help developers by returning an error when it sees it. This would require amending the documentation, but the rule is simple enough to express in one sentence. E.g, "The key name cannot conflict with any other fields in the same struct type."

In the meantime, since this is probably something we don't want to change in go 1.x, I would like to see go vet report an error if you have a single struct type with conflicting struct tags, as in this example:

type MyStruct struct {
    A string `json:"a"`
    B int    `json:"a"`
}

Is that something we can consider? If so, I would be more than happy to contribute. I can also open a separate issue for this if you believe it is appropriate.

[stemar94]

For go vet you maybe want @robpike also on board. I think it would be a good and easy thing to add.

[minux]

For a new vet check, I think we need to get some statistics first. How many times does this problem occur in the wild?

Perhaps this problem is not as frequent as one expects.

[adg]

Paging @josharian who has a corpus of open source Go code. Is there an easy way to check if this error appears anywhere else?

Each vet check we add has a cost; they shouldn't be added for insignificant issues.

[albrow]

I just want to point out that while scanning open source go code is a good idea, it is not a one-to-one correspondence with the number of times this mistake has been made in the wild. Oftentimes, I imagine developers making the mistake, finding it (maybe after scratching their heads for a while like I did), and then fixing it without ever committing the broken code to version control. go vet can still save them trouble in this case.

I also understand the desire to not make go vet too bloated, so getting some kind of metric for how often this happens makes sense. Would you be open to labelling this as question or something similar and gauging responses from the community as an additional metric for how useful this might be?

[adg]

Sure, more input sounds good. We don't have a "Question" tag, but if you want to ask around on mailing lists etc the please do.

On 1 October 2015 at 10:41, Alex Browne notifications@github.com wrote:

I just want to point out that while scanning open source go code is a good idea, it is not a one-to-one correspondence with the number of times this mistake has been made in the wild. Oftentimes, I imagine developers making the mistake, finding it (maybe after scratching their heads for a while like I did), and then fixing it without ever committing the broken code to version control.

I also understand the desire to not make go vet too bloated, so getting some kind of metric for how often this happens makes sense. Would you be open to labelling this as question or something similar and gauging responses from the community as an additional metric for how useful this might be?

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/12791#issuecomment-144583659.

[josharian]

My corpus is no more, and I haven't built a new one yet.

FWIW, this seems prima facie to me like a reasonable vet check—catches a copy/paste error whose manifestation is quiet and delayed. And (without having tried), I think the implementation would be efficient and simple.

[albrow]

Thank you all for your responses. It's unclear to me what the consensus is. Can I go ahead and create a CL or do we want to collect more data first?

[adg]

Go ahead and create the cl; happy to review.

[albrow]

Great. I'll work on the CL as soon as I get a chance, most likely this weekend.

[albrow]

Took a little longer than expected to find time for this, but the CL is out! https://go-review.googlesource.com/#/c/16704/

[gopherbot]

CL https://golang.org/cl/16704 mentions this issue.