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

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


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.

Comment From: hyangah

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 https://github.com/golang/go/issues/34341

Comment From: JamyDev

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

Comment From: findleyr

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.

Comment From: li-jin-gou

@findleyr Hello, What is the status of this support programme? We had a similar problem when we implemented a packagedriver ourselves and he would dynamically generate code to return package information. but because of caching issues, there is no way to get the latest generated code. Is there a recommended solution? Apart from rebooting

Comment From: findleyr

@li-jin-gou I think this issue may have been superseded by #68002. I plan to implement #68002 this year. It is currently targetting the v0.17.0 release, which is tentatively planned for October.

Comment From: ViolaPioggia

@findleyr Hello, What is the status of this support programme? We had a similar problem when we implemented a packagedriver ourselves and he would dynamically generate code to return package information. but because of caching issues, there is no way to get the latest generated code. Is there a recommended solution? Apart from rebooting

I have a similar problem to him. After my local dependencies using gopackagesdriver are changed, I hope gopls can get the latest data in time. Is there any gopls command that can achieve this? Or is there any other better way?

Comment From: li-jin-gou

Thanks, until then we can move forward with the programme of trying to restart gopls. @findleyr

Comment From: JamyDev

FWIW at Uber we did 2 things to fix this: 1. Support overlays on our internal driver. The overlay content contains the newly added import which fixes the initial issue (assuming you have some way of resolving said import without your dependency manager). 2. Added a "reload Imports" codelens above the imports section. Clicking this invalidates the target in our cache, and then slightly modifies the imports (adds a space at the front of the first import and removes it right after). The imports modification triggers gopls to request the imports from the packages driver. @li-jin-gou @ViolaPioggia

Comment From: li-jin-gou

FWIW at Uber we did 2 things to fix this:

1. Support overlays on our internal driver. The overlay content contains the newly added import which fixes the initial issue (assuming you have some way of resolving said import without your dependency manager).
2. Added a "reload Imports" codelens above the imports section. Clicking this invalidates the target in our cache, and then slightly modifies the imports (adds a space at the front of the first import and removes it right after). The imports modification triggers gopls to request the imports from the packages driver.

thanks

Comment From: Skyenought

FWIW at Uber we did 2 things to fix this:

1. Support overlays on our internal driver. The overlay content contains the newly added import which fixes the initial issue (assuming you have some way of resolving said import without your dependency manager).
2. Added a "reload Imports" codelens above the imports section. Clicking this invalidates the target in our cache, and then slightly modifies the imports (adds a space at the front of the first import and removes it right after). The imports modification triggers gopls to request the imports from the packages driver. @li-jin-gou @ViolaPioggia

sorry sir. In our case, the content of *packages.DriverResponse is unchanged, but the content of the file it maps to has changed. So the IDE is not aware of the fact that the prompts are no longer correct due to a change in the file

  {
    "ID": "service1/hello",
    "Name": "hello",
    "PkgPath": "rgo/service1/kitex_gen/hello",
    "GoFiles": [
      "/root/cache/service1/hello/hello.go"
    ],
    "CompiledGoFiles": [
      "/root/cache/service1/hello/hello.go"
    ],
    "Imports": {
      "context": "context",
      "fmt": "fmt"
    }
  },

In fact it really doesn't describe whether or not the specifics of the file change. Have you ever had this problem? @JamyDev

Comment From: li-jin-gou

Dependencies may have been added or the original dependency code may have changed.

Comment From: rv32ima

I'm able to reproduce this. We use Bazel as our build system and this has been one of the more annoying bits. I've recorded what this looks like, and how you can get around it by deleting another import, saving, and then putting it back. That triggers packages.Load to be called again on the file with the updated metadata.

Video is too big to upload to GitHub: https://share.cleanshot.com/YSjRRv3D

Comment From: rv32ima

I delved into this, and made some minor adjustments to gopls in order to have it reload the "state of the world" whenever a Bazel-related file was being touched. It's quite brute-force & dumb, but it seemingly works for us: https://github.com/devzero-inc/tools

Comment From: findleyr

@ellie-idb thanks for looking into this.

If it works, I'd be totally fine with configuration that customizes the set of 'workspace file' patterns, with the default value of []string{"go.mod", "go.work"}. That's a good idea. We could perhaps define it as follows:

// (in gopls/internal/settings.BuildOptions)

// BuildFiles configures the set of regular expressions that match files defining the logical build of the current workspace.
// Any on-disk change to a file matching these expressions causes the workspace to be reloaded.
//
// This setting need only be customized in environments that set GOPACKAGESDRIVER.
BuildFiles []string

One of our major concerns with extending explicit support for bazel is the lack of test coverage (and we have ~zero budget to add such coverage). If your proposed configuration allows us to address this issue without adding any explicit logic for bazel, that largely mitigates this concern.

I can help you land this in gopls. To start, could you update your patchset with proposed changes that avoid bazel-specific logic? Then I can help you with testing; we have extremely limited coverage for GOPACKAGESDRIVER, but should be able to exercise this logic using the fake driver that actually calls go list.

Comment From: rv32ima

@findleyr I took the liberty of calling it WorkspaceFiles rather than BuildFiles, but I've gone ahead and implemented it as you said -- avoiding any specific Bazel-related logic and making it a parameter that takes in LSP-y globs without modifying the API surface too much as well as trying to match any existing behavior. I had to re-arrange some code here and there, but otherwise, there shouldn't be anything too objectionable in the PR.

One note is that I didn't want to introduce any new dependencies as part of this work, so I implemented the file matching using the golang.org/x/tools/gopls/internal/test/integration/fake/glob package... which, I'm not satisfied about, but nevertheless, seemed like the best option here. I've tested this locally on our monorepo and it works just fine once configured properly with Bazel like so:

// in .vscode/settings.json
    "gopls": {
        "build.workspaceFiles": [
            "**/*.{work,mod}",
            "**/BUILD",
            "**/WORKSPACE",
            "**/*.{bzl,bazel}",
        ],
    },

. Let me know what you think! Happy to hop on a call or something to get this landed.

Comment From: findleyr

I took the liberty of calling it WorkspaceFiles rather than BuildFiles

Sounds good, I was actually undecided between those names.

Are you able to send your patch as a CL in Gerrit, so that we can continue discussion in CL comments? (see https://go.dev/doc/contribute if you are new to contributing).

Comment From: rv32ima

@findleyr Done! https://go-review.googlesource.com/c/tools/+/640076

Comment From: gopherbot

Change https://go.dev/cl/640076 mentions this issue: gopls: add WorkspaceFiles option

Comment From: gopherbot

Change https://go.dev/cl/647295 mentions this issue: gopls/doc/release: document the new workspaceFiles option

Comment From: rv32ima

hey folks -- it looks like my fix for this change has made it out in v0.18.0 (see: https://github.com/golang/tools/releases/tag/gopls%2Fv0.18.0). to enable, you'll need to modify your LSP settings to add the following lines: https://github.com/ellie-idb/bazel-go-starter-repo/blob/533cf015072f75a1369981aec21675fdd9b7b29a/.vscode/settings.json#L20-L26. please let me know if you still have issues with out of date metadata after this!

cc @JamyDev @li-jin-gou

Comment From: podtserkovskiy

@rv32ima Thank you for the fix, it resolves the issue we had with the Buck2 driver.

However, we still face a similar problem with stdlib imports due to our lack of support for overlay. - When a new stdlib import is added, the packages.Load call occurs before the user saves the file to disk. - As a result, the driver and build system access the old version of the file and return outdated package metadata. - Gopls doesn't perform another packages.Load after the file is saved, causing the user to see stale diagnostic errors.

The WorkspaceFiles option doesn't address this issue (unless we add a *.go pattern), since we don't include stdlib packages as dependencies in BUCK files.