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

Comments

Projects
None yet
7 participants
@powerman
Copy link

commented May 27, 2018

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 this to the Unplanned milestone May 27, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

@mvdan

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link

commented Jun 1, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Jun 1, 2018

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

@mvdan

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

cmd/vet: rewrite structtag using go/types
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

This comment has been minimized.

Copy link
Member

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 modified the milestones: Unplanned, Go1.12 Aug 20, 2018

@mvdan mvdan self-assigned this Aug 20, 2018

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Seems reasonable to me, for go1.12.

@mvdan

This comment has been minimized.

Copy link
Member

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 join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.