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

Jump to bottom

x/tools/gopls: GOPACKAGESDRIVER calls happen at intervals that make the results incorrect #59625

Open
JamyDev opened this issue Apr 13, 2023 · 3 comments
Open

x/tools/gopls: GOPACKAGESDRIVER calls happen at intervals that make the results incorrect #59625

JamyDev opened this issue Apr 13, 2023 · 3 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/unplanned

Comments

@JamyDev
Copy link

JamyDev commented Apr 13, 2023

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

$ go version
1.20.3

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

GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT="nocoverageredesign,nounified"
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go-code/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go-code"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/.cache/bazel/_bazel_jamy/b97476d719d716accead0f2d5b93104f/external/go_sdk_linux_amd64"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/.cache/bazel/_bazel_jamy/b97476d719d716accead0f2d5b93104f/external/go_sdk_linux_amd64/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.3 X:nounified,nocoverageredesign"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOWORK=""
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-build2135155060=/tmp/go-build -gno-record-gcc-switches"

What did you do?

With GOPACKAGESDRIVER enabled, adding an import (whether by inline writing code and accepting an import suggestion; or manually adding it to the imports section) will trigger a query to the packages driver. However, this trigger will happen prior to the file being saved, yet the packagesdriver will only get a filename to load packages for.

This triggers a bit of a race condition in build systems like Bazel:

  1. Current imports in file.go are a, b, c
  2. I add import d
  3. GoPLS immediately tries to look up package info from the driver using the filename ($ pkgdrvr file=file.go)
  4. Since the file isn't saved at this point in time, the packagesdriver will only see imports a, b, c and thus only return info for those 3
  5. GoPLS gets incomplete results back and flags import d as bad.
  6. Even after save, there is no re-trigger of the packagesdriver, which means this bad imports stays bad until after the imports are changed again; or the language server is restarted.

What did you expect to see?

Since the packagesdriver works with files on disk (in the general case) we should not query the packagesdriver until the IDE has committed the changes on disk.

Alternatively we can extend the packages driver call to query directly for the information of the imported package, rather than indirectly querying for the file the import is added to.

What did you see instead?

Package import resolution fails until language server restart.

I will try to get a project where we can repro this situation in.
@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 Apr 13, 2023
@gopherbot gopherbot added this to the Unreleased milestone Apr 13, 2023
@hyangah
Copy link
Contributor

hyangah commented Apr 14, 2023

Can you share which gopackagesdriver you are using.
The default gopackage driver go list handles this case by using overlay.
https://pkg.go.dev/golang.org/x/tools/go/packages#Config

Is it possible to make your gopackagesdriver recognize the overlay info?

@golang/tools-team I think we need documentation on gopackagesdriver #34341

@JamyDev
Copy link
Author

JamyDev commented Apr 14, 2023 •
edited

We use the rules_go one. I'll have a look at overlay info and see how to support it in the rules_go driver

@suzmue suzmue modified the milestones: Unreleased, gopls/unplanned Apr 20, 2023
@findleyr
Copy link
Contributor

We discussed this a bit more in our team meeting, and decided it would probably be very difficult to support this in the bazel driver (based on our understanding of bazel).

So we'll leave this open: it would be nice to fix, but unlikely to get prioritized any time soon. The work to implement this would be approximately:

  • accumulate invalidations in cache.snapshot.clone while the user types.
  • only invalidate and reload packages when the user saves all files

...but this will be very tricky to implement, and even after implemented may have to a subtly confusing UX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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
Milestone
gopls/unplanned
Development

No branches or pull requests
5 participants
@JamyDev @hyangah @suzmue @gopherbot @findleyr