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: don't report duplicate json tags when field shadowing is used #30846

Closed
flimzy opened this issue Mar 14, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@flimzy
Copy link

commented Mar 14, 2019

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

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jonhall/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jonhall/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build643199218=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Given this simple code:

package test

type Foo struct {
	Foo string `json:"foo"`
	Bar string `json:"bar"`
}

type Bar struct {
	Foo
	Bar int `json:"bar"`
}

As of Go 1.12, go vet complains:

./test.go:10:2: struct field Bar repeats json tag "bar" also at test.go:5

What did you expect to see?

I expect to see no complaint from go vet, as I did in Go 1.11 and earlier, since the field defined at line 5 is hidden, so is disregarded by the json package.

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

This is #30465; the enhancement to the vet check was temporarily reverted in 1.12.1. I plan to simply fix the check for 1.13 in master, as opposed to disabling it.

I wanted to file a separate issue to track that, but you beat me to it :) I'll use this one, if that's okay.

@mvdan mvdan changed the title cmd/vet: Improperly reports duplicate json tag for hidden fields cmd/vet: don't report duplicate json tags when field shadowing is used Mar 18, 2019

@mvdan mvdan added this to the Go1.13 milestone Mar 18, 2019

@mvdan mvdan added the NeedsFix label Mar 18, 2019

@mvdan mvdan self-assigned this Mar 18, 2019

@dstrelau

This comment has been minimized.

Copy link

commented May 30, 2019

Hi 👋. What is the status of putting a proper fix for this into x/tools master?

Right now, golangci/golangci-lint is in a bit of a spot where it's reporting false positives for this case, even when run on 1.12.5 because it brings in x/tools@685fecacd0a0 (master) as an explicit dependency. However, it looks like the temporary fix put in to revert this behavior was put onto the x/tools 1.12 release branch and never pulled back into master. This means that golangci-lint doesn't have the fix and can't get it — it can't simply switch to the 1.12 release branch because there are 300+ other commits in master, many of which presumably fix other issues.

Would it be possible to pull the temporary fix from the 1.12 release branch into master for the time being, until a proper fix can be made for 1.13? I'm not sure the best course of action here.

@mvdan

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Thanks for the ping @dstrelau. I wasn't in a rush to fix this, other than making it to the 1.13 release, as I didn't know anyone was running vet from x/tools master. Is there a particular reason for that, other than getting the latest features? I think that's confusing, as most developers will run vet from the latest Go version, not x/tools master.

In any case, I'll look at a fix today, as there's no reason to delay it further.

@dstrelau

This comment has been minimized.

Copy link

commented May 30, 2019

Thanks @mvdan! I don't really know why golangci-lint chose to vendor master — I'm not involved in development there, just a user — but it seems intentional as they've pulled in newer versions from master several times. I opened an issue over there, so we'll see what the maintainers say.

Either way, thanks for the speedy response and looking into this. 🥇

@gopherbot

This comment has been minimized.

Copy link

commented May 31, 2019

Change https://golang.org/cl/179360 mentions this issue: go/analysis/passes/structtag: allow field tag shadowing

gopherbot pushed a commit to golang/tools that referenced this issue Jun 2, 2019

go/analysis/passes/structtag: allow field tag shadowing
In the following piece of code:

	type T1 struct {
		Shadowed string `json:"foo"`
	}
	type T2 struct {
		T1
		Shadowing int `json:"foo"`
	}

encoding/json will encode T2 using T2.Shadowing, ignoring T2.Shadowed
entirely. This can be a useful feature to replace some of T1's fields
when encoding it. Moreover, this feature is already in use in the wild,
even though it's probably never been documented.

This started being a problem, as the structtag pass started walking
through embedded fields a few months ago. To keep it from complaining
about these useful shadowing cases, make it only see duplicate field tag
names if they are at the same embedding level, in which case no
shadowing is happening.

The old code indexed these tags by encoding key and name, using a
[2]string. The new code needs to add a level integer, so start declaring
named types for the map, and use methods to simplify the code further
below. We still use a map pointer, to avoid allocating on every single
struct definition.

Updates golang/go#30846.

Change-Id: Iae53228d4f8bd91584c59dcc982cb1300970bc8f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/179360
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jun 2, 2019

Change https://golang.org/cl/179999 mentions this issue: cmd/vendor: go get -u golang.org/x/tools && go mod vendor

@gopherbot gopherbot closed this in 8969b05 Jun 7, 2019

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.