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?

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?

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.

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 :)

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.

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

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

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.

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.

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

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

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