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

x/tools/gopls: references doesn't work properly. #60676

Closed
pavlelee opened this issue Jun 8, 2023 · 12 comments
Closed

x/tools/gopls: references doesn't work properly. #60676

pavlelee opened this issue Jun 8, 2023 · 12 comments
Assignees
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@pavlelee
Copy link

pavlelee commented Jun 8, 2023

gopls version

Build info

golang.org/x/tools/gopls master
golang.org/x/tools/gopls@(devel)
github.com/BurntSushi/[email protected] h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
github.com/google/[email protected] h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
github.com/sergi/[email protected] h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
golang.org/x/[email protected] h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
golang.org/x/exp/[email protected] h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
golang.org/x/[email protected] h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk=
golang.org/x/[email protected] h1:PUR+T4wwASmuSTYdKjYHI5TD22Wy5ogLU5qZCOLxBrI=
golang.org/x/[email protected] h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
golang.org/x/[email protected] h1:2sjJmO8cDvYveuX97RDLsxlyUxLl+GHoLxBiRdHllBE=
golang.org/x/tools@(devel)
golang.org/x/[email protected] h1:A9kONVi4+AnuOr1dopsibH6hLi1Huy54cbeJxnq4vmU=
honnef.co/go/[email protected] h1:6qXr+R5w+ktL5UkwEbPp+fEvfyoMPche6GkOpGHZcLc=
mvdan.cc/[email protected] h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
mvdan.cc/xurls/[email protected] 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

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jun 8, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jun 8, 2023
@pavlelee
Copy link
Author

pavlelee commented Jun 8, 2023

Through the debug of gopls found the objects is missing data when try many times
image

@pavlelee
Copy link
Author

pavlelee commented Jun 8, 2023

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.:
image
image

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

image

@pavlelee pavlelee changed the title x/tools/gopls: references occasionally it doesn't work properly. x/tools/gopls: [Very serious impact] references occasionally it doesn't work properly. Jun 9, 2023
@pavlelee pavlelee changed the title x/tools/gopls: [Very serious impact] references occasionally it doesn't work properly. x/tools/gopls: [Very serious impact] references doesn't work properly. Jun 9, 2023
@pavlelee pavlelee changed the title x/tools/gopls: [Very serious impact] references doesn't work properly. x/tools/gopls: [hope to reply as soon as possible] references doesn't work properly. Jun 9, 2023
@pavlelee pavlelee changed the title x/tools/gopls: [hope to reply as soon as possible] references doesn't work properly. x/tools/gopls: references doesn't work properly. Jun 12, 2023
@findleyr
Copy link
Member

Thank you for the report, and the repro and investigation. We will look into this with priority.

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.12.3 Jun 12, 2023
@findleyr
Copy link
Member

CC @adonovan

@adonovan adonovan self-assigned this Jun 12, 2023
@pavlelee
Copy link
Author

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

@findleyr
Copy link
Member

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?

@findleyr
Copy link
Member

findleyr commented Jun 15, 2023

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.

@findleyr
Copy link
Member

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!

@findleyr findleyr assigned findleyr and unassigned adonovan Jun 15, 2023
@pavlelee
Copy link
Author

pavlelee commented Jun 15, 2023

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

You can set runtime.GOMAXPROCS (1) to make it easier to observe this problem.

@findleyr
Copy link
Member

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

@findleyr
Copy link
Member

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.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/503438 mentions this issue: internal/gcimporter: supporting encoding objects from different packages

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants