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: Consider reverting tag conflict for embedded fields #30465

Closed
pschultz opened this issue Feb 28, 2019 · 16 comments
Closed

cmd/vet: Consider reverting tag conflict for embedded fields #30465

pschultz opened this issue Feb 28, 2019 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@pschultz
Copy link

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 4b439e4.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 28, 2019
@ALTree ALTree added this to the Go1.13 milestone Feb 28, 2019
@mvdan
Copy link
Member

mvdan commented Feb 28, 2019

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.

@powerman
Copy link

Well, we had same bug in our old implementation (small test helper function based on reflect). It was easy enough to fix it: iterate over struct fields from last to first, add seen map to skip already seen field names, pass current seen map to same (recursive) function in case of Anonymous struct field or pass empty map in case of non-Anonymous struct field.

I don't know exact Go policy about such non-security fixes in patch releases - if policy forbid this then such a fix should go in 1.13 while 1.12.1 should revert added check. But fix is fairly obvious (I suppose go vet should require similar change in algorithm), so maybe it makes sense to allow it for 1.12.1.

@alandonovan
Copy link
Contributor

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.

@mvdan
Copy link
Member

mvdan commented Feb 28, 2019

@alandonovan I think there are some true positives which we do want to keep as part of the check. In particular, when the duplicates happen at the same level, so neither "wins" and overrides the other. For example:

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

In that program, neither field is encoded, even though both are promoted normally. Both start showing up if either json tag is changed. I don't think this could ever be a false positive; if one really wants to skip nested fields, they can add json:"-" to the anonymous struct fields, or they can override both with a third field in the parent which isn't encoded.

I don't think the fix would be trivial, though. And given that the check isn't super important, and we've gotten one false positive wrong already, I think we should revert in 1.12.1. I can submit the non-revert fix to master for 1.13.

@alandonovan what would be the simplest way to apply the revert? The original change was in cmd/vet, and the code has been refactored and moved to another repo, so a clean revert won't work. In order of preference, I would:

  • Add a return right after if field.Anonymous() and remove the relevant tests, disabling the enhancement
  • Manually port the revert to cmd/vendor/x/tools
  • Revert in x/tools, pull only that change to cmd/vendor, and un-revert in x/tools

@mvdan mvdan modified the milestones: Go1.13, Go1.12.1 Feb 28, 2019
@mvdan mvdan added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 28, 2019
@mvdan
Copy link
Member

mvdan commented Mar 1, 2019

I forgot that x/tools has a 1.12 branch; we can revert the commit(s) there, and pull the latest 1.12 branch changes into Go's 1.12 branch.

@gopherbot
Copy link

Change https://golang.org/cl/164659 mentions this issue: [release-branch.go1.12] go/analysis: disable embedded struct tag check

@nightlyone
Copy link
Contributor

nightlyone commented Mar 1, 2019

Is this behaviour described in the bug an supported use case that should continue to work or a hack that happens to work?

Because if this behaviour is supported, then it seems subtle enough to warrant an example. This would lock down this behaviour both in vet as well as in the encoding/json package.

@gopherbot
Copy link

Closed by merging aa82965741a9fecd12b026fbb3d3c6ed3231b8f8 to release-branch.go1.12.

gopherbot pushed a commit to golang/tools that referenced this issue Mar 13, 2019
This disables the enhancement added in golang.org/cl/115677. The
original change was done in the old cmd/vet location, so it would be
non-trivial to port a full revert of all the code changes. Simply
skipping anonymous struct fields is a simpler way to undo the effects of
the CL.

The reason we want to disable this enhancement of the check in the Go
1.12 release branch is because a false positive was uncovered, which we
want fixed for Go 1.12.1. It's possible that the check will instead be
tweaked for 1.13, but for 1.12.1 we want to play it safe.

Updates golang/go#30465.

Change-Id: I379b4547a560723b8023dad45ab26556b10ee308
Reviewed-on: https://go-review.googlesource.com/c/tools/+/164659
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mvdan
Copy link
Member

mvdan commented Mar 18, 2019

@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.

@pschultz
Copy link
Author

@mvdan, go vet is still failing for the same reason in 1.12.1. Looks like the cmd vendor tree hasn't been updated.

@mvdan
Copy link
Member

mvdan commented Mar 19, 2019

@pschultz you're absolutely right. Why was this issue closed? Nothing about this CL was merged into Go's 1.12 release branch. And https://go-review.googlesource.com/c/tools/+/164659/ simply updated this issue, it didn't close it.

Well... let's try again for 1.12.2.

@mvdan mvdan reopened this Mar 19, 2019
@mvdan mvdan modified the milestones: Go1.12.1, Go1.12.2 Mar 19, 2019
@mvdan mvdan reopened this Mar 19, 2019
@andybons andybons modified the milestones: Go1.12.2, Go1.12.3, Go1.12.4 Apr 5, 2019
@andybons andybons modified the milestones: Go1.12.4, Go1.12.5 Apr 11, 2019
@pschultz
Copy link
Author

@mvdan, is this still on your radar?

@mvdan
Copy link
Member

mvdan commented Apr 23, 2019

See #30399 (comment); I did the relevant backports in the tools repo over a month ago. This just needs someone to update the vendored copy of x/tools before doing a 1.12.x release. I'm not needed for that, nor do I know when Go 1.12.5 will happen. I honestly don't know why this trivial amount of work has been getting pushed back since Go 1.12.2.

@gopherbot
Copy link

Change https://golang.org/cl/174519 mentions this issue: [release-branch.go1.12] cmd/vendor/golang.org/x/tools/go/analysis: update from release-branch.go1.12

@gopherbot
Copy link

Change https://golang.org/cl/174520 mentions this issue: [release-branch.go1.12] cmd/vet: add tests for point-release issues

gopherbot pushed a commit that referenced this issue May 1, 2019
…date from release-branch.go1.12

$ ./update-xtools.sh
Copied /Users/rsc/src/golang.org/x/tools@aa829657 to .
$ cd ~/src/golang.org/x/tools
$ git log -n1 aa829657
commit aa82965741a9fecd12b026fbb3d3c6ed3231b8f8 (HEAD -> release-branch.go1.12, origin/release-branch.go1.12)
Author:     Daniel Martí <mvdan@mvdan.cc>
AuthorDate: Fri Mar 1 11:00:19 2019 +0000
Commit:     Brad Fitzpatrick <bradfitz@golang.org>
CommitDate: Wed Mar 13 21:06:03 2019 +0000
...
$

Picks up cmd/vet fixes that have been inadvertently missed in point releases so far.

Fixes #30399.
Fixes #30465.

Change-Id: Ibcfaac51d134205b986b32f857d54006b19c896a
Reviewed-on: https://go-review.googlesource.com/c/go/+/174519
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue May 1, 2019
Add explicit tests for:

 #30465	cmd/vet: Consider reverting tag conflict for embedded fields
 #30399	cmd/vet: possible to get a printf false positive with big.Int

because we have managed not to fix them in the last
couple point releases, and it will be too embarrassing
to do that yet again.

Change-Id: Ib1da5df870348b6eb9bfc8a87c507ecc6d44b8dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/174520
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@andybons
Copy link
Member

andybons commented May 6, 2019

Closing as the cherry-picks have landed.

@andybons andybons closed this as completed May 6, 2019
@golang golang locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants