go: cmd/vet: Consider reverting tag conflict for embedded fields
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/pschultz/.cache/go-build" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOOS="linux" GOPATH="/home/pschultz/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-build788821326=/tmp/go-build -gno-record-gcc-switches"
What did you do?
Run go vet
on existing code base, containing code such as this: https://play.golang.org/p/HnkPcNUu84J
What did you expect to see?
go vet
to either succeed, or to report only real problems.
What did you see instead?
go vet
fails due to intended shadowing of embedded fields with json tags.
#25593 changed vet to also report repeated tags from embedded types.
We use this technique intentionally to create slightly varying JSON representation for struct types, typically when returning large lists of a particular type. Here is a simplified example (playground):
package main
import (
"encoding/json"
"fmt"
)
type Book struct {
Author Author `json:"author"`
Title string `json:"title"`
}
type Author struct {
Name string `json:"name"`
Biography string `json:"biography"`
}
func main() {
b := Book{
Title: "Foo",
Author: Author{
Name: "John Doe",
Biography: "Lorem Ipsum",
},
}
// When returning a list of books, we don't want to include the whole
// Author struct for each book.
type BookSummary struct {
Book // Include all Book fields by default,
Author struct { // but override the "author" field with a smaller model
Name string `json:"name"`
// All other fields omitted
} `json:"author"`
}
var s BookSummary
s.Book = b
s.Author.Name = b.Author.Name
j, _ := json.MarshalIndent(s, "", " ")
fmt.Println(string(j))
}
With go vet
now rejecting this code, the only viable alternative I can see is to redefine the Book type completely:
type BookSummary struct {
Title string `json:"title"`
Author struct {
Name string `json:"name"`
} `json:"author"`
// Many more fields
}
This way, however, the JSON representation of BookSummary will not be updated automatically when Book fields are added or changed. We would also have to assign each field individually, since Book and BookSummary don’t have the same underlying type. Realistically, this forces us to do some kind of code generation, which is clearly much more trouble than what worked with Go 1.11.
Other possible fixes are:
- Not run
go vet
anymore - Marshal the Book value, then unmarshal into a map, then modify the map, and then marshal again.
- Add the MarshalJSON method and some fields that control how that method behaves.
None of these are appealing.
Please consider this as a valid use-case for repeated json tags on embedded fields and revert 4b439e41e2cdd78e0eeed05942c93364c5d99b6b.
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 4
- Comments: 16 (9 by maintainers)
Commits related to this issue
- [release-branch.go1.12] go/analysis: disable embedded struct tag check This disables the enhancement added in golang.org/cl/115677. The original change was done in the old cmd/vet location, so it wou... — committed to golang/tools by mvdan 5 years ago
- [release-branch.go1.12] cmd/vendor/golang.org/x/tools/go/analysis: update from release-branch.go1.12 $ ./update-xtools.sh Copied /Users/rsc/src/golang.org/x/tools@aa829657 to . $ cd ~/src/golang.org/... — committed to golang/go by rsc 5 years ago
- [release-branch.go1.12] cmd/vet: add tests for point-release issues Add explicit tests for: #30465 cmd/vet: Consider reverting tag conflict for embedded fields #30399 cmd/vet: possible to get a pr... — committed to golang/go by rsc 5 years ago
@nightlyone I’m not sure if this is a documented and intended feature. But like @alandonovan mentioned, it seems like a useful feature to me. And I imagine some users already depend on it, as pointed out by the bugs filed against vet.
Let’s use #30846 to keep track of the proper fix for Go 1.13.
This does seem like a valid use of field shadowing with no easy alternative. Daniel, do you have any real-world examples of true-positive reports of this checker that we could use to try to identify a better heuristic? Otherwise we may have to disable the field shadowing check for now.
There are many ways in which adding an annotation or comment mechanism would simplify the task of writing checkers with high precision and recall, but we have historically avoided doing so.
This is why we ask that you try out betas and RCs - this change to vet has been shipped in all 1.12 releases, going all the way back to beta1 😃
I admit I haven’t considered this kind of false positive, but it would be a shame to simply throw away the added check. @alandonovan @robpike @powerman any thoughts?
Another option is to revert this particular vet enhancement in 1.12.1, and keep it in master to decide what to do before the 1.13 release.