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: should report json tag conflict for embedded struct #25593

Closed
powerman opened this issue May 27, 2018 · 12 comments
Closed

cmd/vet: should report json tag conflict for embedded struct #25593

powerman opened this issue May 27, 2018 · 12 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@powerman
Copy link

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

go version go1.10.2 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/powerman/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/powerman/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="x86_64-pc-linux-gnu-gcc"
CXX="x86_64-pc-linux-gnu-g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build251037373=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/zvagdPQx1Gf

What did you expect to see?

go vet should warn about using same json tag name in embedded struct.
It already warn about using same json tag name in same struct, so making it detect embedded fields looks like obvious enhancement.
May be related to #17609

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 27, 2018
@agnivade agnivade added this to the Unplanned milestone May 27, 2018
@odeke-em
Copy link
Member

odeke-em commented Jun 1, 2018

/cc @robpike @mvdan @josharian

@mvdan
Copy link
Member

mvdan commented Jun 1, 2018

I'd also wonder if this should be encoding/json's job, which I presume is #17609. But I guess it's too late to make it error on these cases in Go1.

Agreed that this vet enhancement makes sense.

@robpike
Copy link
Contributor

robpike commented Jun 1, 2018

Offhand this seems a minor addition to the struct tags check if it can be done efficiently. Is it actually a problem that shows up in practice?

@mvdan
Copy link
Member

mvdan commented Jun 1, 2018

@robpike I'm working on a patch now, which will let us see how many new vet warnings it would see in existing Go code.

@powerman
Copy link
Author

powerman commented Jun 1, 2018

Is it actually a problem that shows up in practice?

It depends on what you mean. I didn't had such a bug yet in my code. But I do use embedded structs a lot, I do use custom 1-3 char json names (saves about 50% of json length and it's important for us because we store 300KB json in mysql and transfer/decode 600KB or 300KB is a noticeable difference) and I do spend a lot of time and attention to manually check tags to ensure they won't conflict. Actually I've asked one of our developers to write a unit test using reflect to automate this check - and before creating that task I've checked how good metalinter's checkers already handle this case, found vet handle it partially and opened this issue.

@gopherbot
Copy link

Change https://golang.org/cl/115676 mentions this issue: cmd/vet: rewrite structtag using go/types

@gopherbot
Copy link

Change https://golang.org/cl/115677 mentions this issue: cmd/vet: check embedded field tags too

@mvdan
Copy link
Member

mvdan commented Jun 1, 2018

A quick fix is implemented above, on top of a small refactor to use go/types. I quickly checked the Go tree, and it found no new cases.

I'm currently downloading https://github.com/rsc/corpus, which should be a large enough corpus of packages to make a decision. GitHub is throttling me to about 100KB/s, so it's going to take a few hours.

@mvdan
Copy link
Member

mvdan commented Jun 1, 2018

I failed to use https://github.com/rsc/corpus after nearly an hour of trying. Even when using the tarball and recursively downloading deps, I always got stuck with some packages not typechecking properly. If anyone knows how to use it, any help will be appreciated.

I settled for some ad-hoc testing on packages that I regularly use and their dependencies. I found that a certain popular package has lots of newly found warnings thanks to this check:

$ go vet github.com/nlopes/slack
# github.com/nlopes/slack
./im.go:25: struct field at ./im.go:27 repeats json tag "is_im" at ./conversation.go:22
./search.go:54: struct field at ./pagination.go:15 repeats json tag "page" at ./pagination.go:7
./search.go:54: struct field at ./search.go:58 repeats json tag "total" at ./pagination.go:6
./search.go:61: struct field at ./pagination.go:15 repeats json tag "page" at ./pagination.go:7
./search.go:61: struct field at ./search.go:65 repeats json tag "total" at ./pagination.go:6
./search.go:68: struct field at ./pagination.go:15 repeats json tag "page" at ./pagination.go:7
./search.go:68: struct field at ./search.go:58 repeats json tag "total" at ./pagination.go:6
./search.go:68: struct field at ./search.go:62 repeats json tag "matches" at ./search.go:55
./search.go:68: struct field at ./pagination.go:5 repeats json tag "count" at ./pagination.go:5
./search.go:68: struct field at ./pagination.go:6 repeats json tag "total" at ./pagination.go:6
./search.go:68: struct field at ./pagination.go:7 repeats json tag "page" at ./pagination.go:7
./search.go:68: struct field at ./pagination.go:8 repeats json tag "pages" at ./pagination.go:8
./search.go:68: struct field at ./search.go:63 repeats json tag "paging" at ./search.go:56
./search.go:68: struct field at ./pagination.go:14 repeats json tag "total_count" at ./pagination.go:14
./search.go:68: struct field at ./pagination.go:15 repeats json tag "page" at ./pagination.go:7
./search.go:68: struct field at ./pagination.go:16 repeats json tag "per_page" at ./pagination.go:16
./search.go:68: struct field at ./pagination.go:17 repeats json tag "page_count" at ./pagination.go:17
./search.go:68: struct field at ./pagination.go:18 repeats json tag "first" at ./pagination.go:18
./search.go:68: struct field at ./pagination.go:19 repeats json tag "last" at ./pagination.go:19
./search.go:68: struct field at ./search.go:64 repeats json tag "pagination" at ./search.go:57
./search.go:68: struct field at ./search.go:65 repeats json tag "total" at ./pagination.go:6
./users.go:171: struct field at ./users.go:130 repeats json tag "presence" at ./users.go:124

gopherbot pushed a commit that referenced this issue Aug 20, 2018
This lets us simplify the code considerably. For example, unquoting the
tag is no longer necessary, and we can get the field name with a single
method call.

While at it, fix a typechecking error in testdata/structtag.go, which
hadn't been caught since vet still skips past go/types errors in most
cases.

Using go/types will also let us expand the structtag check more easily
if we want to, for example to allow it to check for duplicates in
embedded fields.

Finally, update one of the test cases to check for regressions when we
output invalid tag strings. We also checked that these two changes to
testdata/structtag.go didn't fail with the old structtag check.

For #25593.

Change-Id: Iea4906d0f30a67f36b28c21d8aa96251aae653f5
Reviewed-on: https://go-review.googlesource.com/115676
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Rob Pike <r@golang.org>
@mvdan
Copy link
Member

mvdan commented Aug 20, 2018

I'm pretty confident that this is worth doing - it's very little extra work for vet now that it can rely on go/types, and the check is useful - see for example how I found buggy code in a fairly popular library above.

Ping @alandonovan @josharian (as per dev.golang.org/owners) for a decision. I've got a working CL that you can try; I just want to be sure we want the change before cleaning it up and adding tests.

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 20, 2018
@mvdan mvdan modified the milestones: Unplanned, Go1.12 Aug 20, 2018
@mvdan mvdan self-assigned this Aug 20, 2018
@alandonovan
Copy link
Contributor

Seems reasonable to me, for go1.12.

@mvdan
Copy link
Member

mvdan commented Aug 22, 2018

Thanks for the feedback - the CL is now ready to review at https://go-review.googlesource.com/c/go/+/115677.

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

7 participants