x/tools/gopls: references doesn't work properly.
pavlelee opened this issue · 12 comments
gopls version
Build info
golang.org/x/tools/gopls master
golang.org/x/tools/gopls@(devel)
github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
golang.org/x/exp@v0.0.0-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
golang.org/x/mod@v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk=
golang.org/x/sync@v0.2.0 h1:PUR+T4wwASmuSTYdKjYHI5TD22Wy5ogLU5qZCOLxBrI=
golang.org/x/sys@v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
golang.org/x/text@v0.9.0 h1:2sjJmO8cDvYveuX97RDLsxlyUxLl+GHoLxBiRdHllBE=
golang.org/x/tools@(devel)
golang.org/x/vuln@v0.0.0-20230110180137-6ad3e3d07815 h1:A9kONVi4+AnuOr1dopsibH6hLi1Huy54cbeJxnq4vmU=
honnef.co/go/tools@v0.4.2 h1:6qXr+R5w+ktL5UkwEbPp+fEvfyoMPche6GkOpGHZcLc=
mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.20.4
go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/data/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/data/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/data/go"
GOPRIVATE=""
GOPROXY=""
GOROOT="/usr/local/go"
GOSUMDB=""
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/data/go/src/dashboard/go.mod"
GOWORK="/data/go/src/dashboard/go.work"
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3198834238=/tmp/go-build -gno-record-gcc-switches"
What did you do?
go work file:
go 1.20
use (
.
./staging/src/skipper
)
Correct:
gopls references -d /data/go/src/dashboard/staging/src/skipper/pkg/models/vm.go:130:2
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:255:15-23
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:257:13-21
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:263:12-20
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:263:236-244
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:282:12-20
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:282:207-215
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:297:14-22
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:298:12-20
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:305:11-19
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:305:206-214
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:523:11-19
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:523:206-214
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:531:11-19
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:531:206-214
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:560:11-19
/data/go/src/dashboard/cmd/nightsWatch/tools/initNode.go:560:206-214
/data/go/src/dashboard/dao/vminstance.go:614:3-11
/data/go/src/dashboard/service/progress.go:498:20-28
/data/go/src/dashboard/service/progress.go:504:49-57
/data/go/src/dashboard/staging/src/skipper/pkg/models/vm.go:130:2-10
The second time, wrong:
gopls references -d /data/go/src/dashboard/staging/src/skipper/pkg/models/vm.go:130:2
/data/go/src/dashboard/dao/vminstance.go:614:3-11
/data/go/src/dashboard/staging/src/skipper/pkg/models/vm.go:130:2-10
If I execute rm-rf / root/.cache/ gopls, I can restore the correct results
What did you expect to see?
What did you see instead?
Editor and settings
Logs
Through the debug of gopls found the objects is missing data when try many times
This repo can reproduce the problem https://github.com/pavlelee/gopls-references
https://github.com/pavlelee/gopls-references/blob/5709ea5feb28e5be021238f562d3f8a6425cb6df/models/vm.go#L5
The correct citation should be
/data/go/src/github.com/pavlelee/gopls-references/controller/references.go:11:3-5
/data/go/src/github.com/pavlelee/gopls-references/staging/src/submodule/models/vm.go:4:2-4
The problem I identified was that there were set two times filecache, first the data length of 519 was set, but last the data length of 465 was set, and 519 contained the correct object, but 465 did not. The strange thing is why the same key sets two caches of different length.
The logs for troubleshooting are as follows, the first time cache couldn't find it, so pre return true then execution post func to get correct data. the next time retry code found cache, but the cache data length was not correct(lenght 465), so couldn't find the reference correctly.：
Continue to debug, I found err: = objectpathFor (obj) code is occasionally returned this error can't find path for field Name string in github.com/pavlelee/submodule/models
Continue to investigate and find that the two objects are occasionally not equal.
Thank you for the report, and the repro and investigation. We will look into this with priority.
Thank you for the report, and the repro and investigation. We will look into this with priority.
@findleyr Thanks, it has a big impact on our project, and I really want to fix it, but I haven't figured out why the pos is different
Hmm, I can't yet reproduce with that repo. @pavlelee are you just running references repeatedly to encounter this?
In your screenshots, you have some interesting instrumentation. What does "type check data is ..." mean? When/where is that log statement added? Could you perhaps share a CL with your WIP investigation, so that we can see the instrumentation?
Ok, I can reproduce. About 1 out of every 4 attempts I can get the correct references, by clearing the cache, restarting gopls, and immediately requesting references on the ID or Name field use in controller/references.go. The timing seems to matter, which may be a clue as it could relate to making the request before the initial workspace diagnostics have completed.
I'll note that references to models.VM still do the right thing. It is only the fields that are wrong.
As I write this, I am starting to suspect that it is related to whether the import is via export data, or type-checking source. Of course the imported package loses the correct location of the forwarded field. In fact, that is almost certainly it.
Yes, that is 100% it. I just hacked up gopls to never import from export data, and the broken references is no longer reproducible.
So this is an exporter/importer bug. Unless I'm missing some complexity, we'll fix this tomorrow!
Ok, I can reproduce. About 1 out of every 4 attempts I can get the correct references, by clearing the cache, restarting gopls, and immediately requesting references on the ID or Name field use in
controller/references.go. The timing seems to matter, which may be a clue as it could relate to making the request before the initial workspace diagnostics have completed.I'll note that references to models.VM still do the right thing. It is only the fields that are wrong.
As I write this, I am starting to suspect that it is related to whether the import is via export data, or type-checking source. Of course the imported package loses the correct location of the forwarded field. In fact, that is almost certainly it.
No, the timing not to matter. The same file is set filecache twice, if the correct data (data lenght 519) is overwritten by the wrong data(data lenght 465), then there will be a problem. I don't quite understand why I set it twice.
You can set runtime.GOMAXPROCS (1) to make it easier to observe this problem.
@pavlelee the timing doesn't matter for the incorrect behavior, but to get the correct behavior you must get references before the dependent package is serialized.
I'm going to fix this in a somewhat hacky way, by changing how we compare fields in the objectpath algorithm. That should work for references, but not for other places where we rely on the canonical identity of fields.
I'll file a separate issue for the larger fix.
Change https://go.dev/cl/503438 mentions this issue: internal/gcimporter: supporting encoding objects from different packages