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: incorrect file and line number in repeated json tag warning #28995

Closed
mark-rushakoff opened this Issue Nov 29, 2018 · 12 comments

Comments

Projects
None yet
6 participants
@mark-rushakoff
Copy link
Contributor

mark-rushakoff commented Nov 29, 2018

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

$ go version
go version devel +96d41786c5 Wed Nov 28 18:53:53 2018 +0000 darwin/amd64

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mr/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mr/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/mr/gotip/src/github.com/golang/go"
GOTMPDIR=""
GOTOOLDIR="/Users/mr/gotip/src/github.com/golang/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/mr/gomod/platform/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ct/bl4_z3g51ks8239_r2k07v_40000gn/T/go-build416189829=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran go vet ./... against github.com/influxdata/platform at revision 8279ba65c37c86586a8185afceb3dd73597a1e4b (master at the moment).

I received the warning:

http/dashboard_service.go:76:2: struct field Cells repeats json tag "cells" also at dashboard_service.go:184

The phrase struct field Cells repeats json tag "cells" is understandable, but line 184 is a comment: https://github.com/influxdata/platform/blob/8279ba65c37c86586a8185afceb3dd73597a1e4b/http/dashboard_service.go#L184

Line 76 is correct, however: https://github.com/influxdata/platform/blob/8279ba65c37c86586a8185afceb3dd73597a1e4b/http/dashboard_service.go#L76

Apparently it's because the struct that vet is flagging:

type dashboardResponse struct {
	platform.Dashboard
	Cells []dashboardCellResponse `json:"cells"` // line 76 of dashboard_service.go
	Links dashboardLinks          `json:"links"`
}

embeds platform.Dashboard which also has a field with json tag cells:

// package platform
type Dashboard struct {
	ID          ID            `json:"id,omitempty"`
	Name        string        `json:"name"`
	Description string        `json:"description"`
	Cells       []*Cell       `json:"cells"` // line 53 of dashboard.go
	Meta        DashboardMeta `json:"meta"`
}

But the line number and file don't match dashboard_service.go:184:
https://github.com/influxdata/platform/blob/8279ba65c37c86586a8185afceb3dd73597a1e4b/dashboard.go#L49-L55

/cc @alandonovan

@agnivade agnivade added this to the Go1.12 milestone Nov 29, 2018

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 2, 2018

Thanks for reporting. I think I know what the issue is. From the docs, we can see:

The Fset, Files, Pkg, and TypesInfo fields provide the syntax trees, type information, and source positions for a single package of Go code.

However, we do pass.Fset.Position(pos) to point at the "also at" field. If that file is in a different package, its position information may not be in pass.Fset (as per the comment above), so we get mangled position information.

I can't see a quick fix here, other than having pass.Fset also including the *token.Files for each package's dependencies. I presume that's out of the question, since the documentation says exactly the opposite.

A perhaps simpler fix would be to simply name the other field with a qualified identifier, like github.com/influxdata/platform.Dashboard.Cells, without providing position information.

@alandonovan any thoughts?

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 2, 2018

Something interesting is that go vet and go/analysis/cmd/vet report different things:

$ go vet .
# github.com/influxdata/platform/http
./dashboard_service.go:86:2: struct field Cells repeats json tag "cells" also at dashboard_service.go:158
$ vet .
/home/mvdan/git/platform/http/dashboard_service.go:86:2: struct field Cells repeats json tag "cells" also at dashboard.go:53

@alandonovan I presume I should be using unitchecker/main.go to debug issues in go vet quickly? If so, what's the purpose of cmd/vet?

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Dec 3, 2018

I think I know what the issue is. From the docs, we can see:

The Fset, Files, Pkg, and TypesInfo fields provide the syntax trees, type information, and source positions for a single package of Go code.

However, we do pass.Fset.Position(pos) to point at the "also at" field. If that file is in a different package, its position information may not be in pass.Fset (as per the comment above), so we get mangled position information.

I can't see a quick fix here, other than having pass.Fset also including the *token.Files for each package's dependencies. I presume that's out of the question, since the documentation says exactly the opposite.

The position information should be correct for every types.Object relevant to the current package, which is a subset of its transitive imports. (The problem you are alluding to could occur if an Analyzer squirreled away Objects while analyzing one package and used them while analyzing in another package, but you would have to go out of your way to make that mistake.)

The Analyzer code is correct. It looks to me like a bug in the handling of compiler export data, which is why it shows up in go vet (which used it) but not in x/tools/.../vet which ues only source. Unfortunately I can't reproduce it because the influx build appears to be broken. Could you provide me with a sequence of commands to check out a working influx build on which I can run vet?

I assume I should be using unitchecker/main.go to debug issues in go vet quickly? If so, what's the purpose of cmd/vet?

cmd/vet designates the suite of analyzers and invokes unitchecker. I'm not sure I understand what you're asking.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 3, 2018

It looks to me like a bug in the handling of compiler export data, which is why it shows up in go vet (which used it) but not in x/tools/.../vet which ues only source.

Ah, that makes sense.

Could you provide me with a sequence of commands to check out a working influx build on which I can run vet?

$ # outside of GOPATH to not need GO111MODULE=on
$ go version
go version devel +624e197c71 Sat Dec 1 15:03:28 2018 +0000 linux/amd64
$ git clone https://github.com/influxdata/platform
$ cd platform/http
$ git checkout 873aae3fad0980b583ed3dfaed724242a0d31e25
$ go vet .
# github.com/influxdata/platform/http
./dashboard_service.go:76:2: struct field Cells repeats json tag "cells" also at dashboard_service.go:194
$ vet .
/home/mvdan/git/platform/http/dashboard_service.go:76:2: struct field Cells repeats json tag "cells" also at dashboard.go:53

The build worked for me the other day, and the latest HEAD today (see the commit hash above) still works fine. Perhaps you're not on Linux?

I'm not sure I understand what you're asking.

Having read your comment, what I'm asking is for a way to run vet in exactly the same way that go vet does - with cmd/go's package loader that shows this peculiar bug, and not x/tools/cmd/vet, which I presume uses go/packages.Load.

I can of course add a debug println to x/tools, re-vendor vet in the Go tree, and run go install cmd/vet. But that's a bit cumbersome :)

@mark-rushakoff

This comment has been minimized.

Copy link
Contributor

mark-rushakoff commented Dec 3, 2018

Unfortunately I can't reproduce it because the influx build appears to be broken

Of course I don't want to derail this thread into a separate build problem... but we had been running into a module cache issue related to a kubernetes dependency and something with .gitattributes in the repository that was fixed in go1.11.2. If you're seeing output like

go: verifying k8s.io/client-go@v0.0.0-20180806134042-1f13a808da65: checksum mismatch
    downloaded: h1:wQUEIVcXYxsDE8RXfUufo1nfnkeH/BEPhT175YIzea4=
    go.sum:     h1:kQX7jEIMYrWV9XqFN4usRaBLzCu7fd/qsCXxbgf3+9g=

And you're on go1.11.2, you can go clean -modcache or sudo rm -rf $GOPATH/pkg/mod/cache/download/k8s.io $GOPATH/pkg/mod/k8s.io if you don't want to blow away everything.

(Related to kubernetes/kubernetes#69040 and #27925.)

Or feel free to check out an even earlier revision - I believe this same issue was reproducible on master of influxdata/platform when the new vet switchover happened.

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Dec 3, 2018

Thanks Daniel, I can reproduce the problem now. (Apparently running from within GOPATH was my problem. Also I had to install bzr tool. First time I've ever encountered it in practice.)

The root cause is that go/importer doesn't have a token.FileSet parameter even though logically it should, so it creates its own temporary FileSets then throws them away. The types.Package that go/importer returns have object positions that implicitly refer to to these inaccessible FileSets; when vet uses its own FileSet it gets the wrong answer.

The solution is for go/importer to take an additional parameter.

@griesemer

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 3, 2018

It's probably too late for this to go into 1.12.

@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 3, 2018

@mvdan mvdan removed their assignment Dec 3, 2018

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Dec 3, 2018

It's probably too late for this to go into 1.12.

It's an API-compatible, low-risk change that fixes a clear bug.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 3, 2018

Thanks all. Doesn't look like it's a bug in the vet code I touched, so I'm unassigning myself for now.

@alandonovan unless you would prefer that we work around this bug in Go 1.12's vet? It seems like giving wrong position information is worse than giving none at all.

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 3, 2018

@alandonovan We are just before the beta. We're not going to change APIs at this point.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 3, 2018

Change https://golang.org/cl/152258 mentions this issue: go/importer: add ForCompiler, which accepts a token.FileSet

@gopherbot gopherbot closed this in 159797a Dec 4, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 4, 2018

Change https://golang.org/cl/152578 mentions this issue: go/analysis/unitchecker: use importer.ForCompiler under go1.12

gopherbot pushed a commit to golang/tools that referenced this issue Dec 4, 2018

go/analysis/unitchecker: use importer.ForCompiler under go1.12
importer.For does not populate the caller's token.FileSet
leading to spurious position information in diagnostics.

This change is the upstream fix for
https://go-review.googlesource.com/c/go/+/152258.

Fixes golang/go#28995

Change-Id: I9307d4f1f25c2b0877558426d4d71b3f1df99505
Reviewed-on: https://go-review.googlesource.com/c/152578
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment