cmd/vet: don't report duplicate json tags when field shadowing is used #30846
Comments
|
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
changed the title
mvdan
added
the
NeedsFix
|
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. |
|
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. |
|
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. 🥇 |
|
Change https://golang.org/cl/179360 mentions this issue: |
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>
|
Change https://golang.org/cl/179999 mentions this issue: |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Given this simple code:
As of Go 1.12,
go vetcomplains: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 thejsonpackage.The text was updated successfully, but these errors were encountered: