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: degraded behavior on repositories containing multiple modules #59176

Closed
AaronFriel opened this issue Mar 22, 2023 · 8 comments
Closed
Assignees
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@AaronFriel
Copy link

AaronFriel commented Mar 22, 2023

gopls version

gopls -v version Build info ---------- golang.org/x/tools/gopls v0.11.0 golang.org/x/tools/[email protected] h1:/nvKHdTtePQmrv9XN3gIUN9MOdUrKzO/dcqgbG6x8EY= 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:QfTh0HpN6hlw6D3vu8DAwC8pBIwikq0AI1evdm+FksE= golang.org/x/exp/[email protected] h1:fl8k2zg28yA23264d82M4dp+YlJ3ngDcpuB1bewkQi4= golang.org/x/[email protected] h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA= golang.org/x/[email protected] h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= golang.org/x/[email protected] h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A= golang.org/x/[email protected] h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg= golang.org/x/[email protected] h1:7/HkGkN/2ktghBCSRRgp31wAww4syfsW52tj7yirjWk= golang.org/x/[email protected] h1:qptQiQwEpETwDiz85LKtChqif9xhVkAm8Nhxs0xnTww= honnef.co/go/[email protected] h1:oDx7VAwstgpYpb3wv0oxiZlxY+foCpRAwY7Vk6XpAgA= mvdan.cc/[email protected] h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM= mvdan.cc/xurls/[email protected] h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc= go: go1.20.2

go env

GO111MODULE="" GOARCH="amd64" GOBIN="/home/friel/go/bin" GOCACHE="/home/friel/.cache/go-build" GOENV="/home/friel/.config/go/env" GOEXE="" GOEXPERIMENT="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOINSECURE="" GOMODCACHE="/home/friel/go/pkg/mod" GOOS="linux" GOPATH="/home/friel/go" GOPROXY="https://proxy.golang.org,direct" GOROOT="/home/friel/.go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/home/friel/.go/pkg/tool/linux_amd64" GOVCS="" GOVERSION="go1.20.2" GCCGO="gccgo" GOAMD64="v1" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/dev/null" 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-build903227594=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Cloned a multi-module repository, such as https://github.com/pulumi/pulumi-kubernetes into the correct location in GOPATH. (Or outside GOPATH, the behavior is the same either way.)

I then opened this directory in VS Code.

What did you expect to see?

I expected to see the Go modules for /examples, /provider, and /sdk/go "light up", with valid code completion and import information.

What did you see instead?

All non-built-in imports yield broken import errors, e.g.:

could not import github.com/pkg/errors (no required module provides package "github.com/pkg/errors")

Screenshot of "could not import ..." errors in Visual Studio Code affecting all imports that are not shipped with Go.

Neither go mod download nor go get nor any other command seems to rectify these.

Editor and settings

Visual Studio Code 1.76.2
Go extension golang.go

Additional information

This experience is materially worse than previous versions of Go, a serious regression for new users and even onboarding engineers into Go. Go workspaces appear to now be required for these use cases, however committing go.work files would result in our repositories overriding advanced users' workspace configurations.

Would it be possible to revert removal of experimentalWorkspaceModule until the work in #57979 lands? There is a serious gap in usability. Any would-be contributor cloning a repository containing multiple Go modules and using gopls would encounter this, and the error message does not put users onto a golden path.

Impact

This impacts approximately two hundred thousand modules on GitHub alone, per a GitHub Code Search showing over 200,000 go.mod files in subdirectories:

Screenshot of Code Search showing over 200,000 go.mod files in subdirectories.

@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 Mar 22, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 22, 2023
@findleyr
Copy link
Contributor

Thanks for this thoughtful issue. It sounds like you've correctly analyzed the problem: a go.work file is required until #57979 lands.

Unfortunately, we can't revert experimentalWorkspaceModule at this point, nor do I think that is the correct course of action. If users want to work on multiple modules the same build, they should use a go.work file. If they want to work on multiple disconnected modules, gopls should do the right thing (which is #57979). We have undertaken a major rewrite of gopls' internals since reverting experimentalWorkspaceModule, to fix the scalability problems that are blocking #57979. At this point, the path forward is faster than the path backward.

That error message is unfortunately poor, which is my fault. We should have at least fixed the error message before reverting experimentalWorkspaceModule. (FWIW, we have no data on how many users are using the experimentalWorkspaceModule setting). Given that v0.12.0 is still ~a month out, perhaps we should do a v0.11.1 release improving the error message. I will investigate if that would be possible.

@findleyr
Copy link
Contributor

Sorry, just to be clear, in my previous comment I was assuming that the experimentalWorkspaceModule feature had been removed in [email protected], but my memory was incorrect. In fact that removal happened after v0.11.0 (https://go.dev/cl/458116). This was intentional, because we did not have time to otherwise improve the multi-module experience for v0.11.0.

So, are you saying that the experience with gopls@master is degraded, relative to the experience with v0.11.0+experimentalWorkspaceModule? Or are you saying that multi-module support is degraded relative to working in GOPATH?

I'm not trying to say there isn't a problem here: there is and it is our top priority, but fixing it is rather more complicated than one might guess. Our plan to do so is to fix the incremental performance cost of each workspace (#57987), and then have gopls automatically create new workspaces as files are opened (#57979).

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.12.0 Mar 22, 2023
@findleyr findleyr self-assigned this Mar 22, 2023
@AaronFriel
Copy link
Author

AaronFriel commented Mar 23, 2023

@findleyr This could have been a regression in the Go extension or gopls.

Reverting my local environment back to Go 1.17 and [email protected] and an older extension v0.32.0 results fine results with experimentalWorkspaceModule, in this screenshot taken outside GOPATH, it appears to work!

Screenshot of VS Code showing correct IDE hinting.

Trying to bisect this back to current day - and wiping local go bin and other paths - I was unable to repro. Perhaps my IDE was using gopls at the tip - likely from a local build of Go/gopls. If I manually install gopls unstable, the behavior I saw comes back.

It's a serious regression, but the impact in my issue description is wrong barring another confounding variable. I've reset my Go environment twice now, and I cannot repro as long as I have experimentalWorkspaceModule enabled.

@AaronFriel
Copy link
Author

AaronFriel commented Apr 4, 2023

@findleyr I did a survey of how users could experience gopls in one of the repositories my team owns. While I think we can fix issues with make commands in some cases, in others, the introduction of Workspaces and deprecation of experimentalWorkspaceModule has significantly degraded developer UX. Each row effectively appears in triplicate, as I wanted to be thorough in checking Go 1.18 to 1.20. The only significant behavior differences occur due to location of the repository on disk, gopls version, and experimentalWorkspaceModule flag status.

What I think is markedly worse though, is that the errors users see don't direct users to a golden path.

I would be happy to chat more or provide test cases. In the mean time, we're exploring how we can modify our make targets to initialize a workspace on first run, knowing this default won't be right for all developers. This introduces complexity to our workflow that didn't exist prior to Go workspaces, so it feels like there have been some meaningful regressions both in usability and simplicity.

Clone location Go
version
gopls
version
gopls.EWM
Enabled?
Makefile
make tfgen
VS Code
Silent Start
VS Code
Problems Found
gopls LSP
Code Completion
GOPATH 1.18.10 0.11.0 FALSE >100
GOPATH 1.19.7 0.11.0 FALSE >100
GOPATH 1.20.2 0.11.0 FALSE >100
GOPATH 1.18.10 0.11.0 TRUE ❌[1] 0
GOPATH 1.19.7 0.11.0 TRUE ❌[1] 0
GOPATH 1.20.2 0.11.0 TRUE ❌[1] 0
GOPATH 1.18.10 HEAD FALSE >100
GOPATH 1.19.7 HEAD FALSE >100
GOPATH 1.20.2 HEAD FALSE >100
GOPATH 1.18.10 HEAD TRUE ❌[2] >100
GOPATH 1.19.7 HEAD TRUE ❌[2] >100
GOPATH 1.20.2 HEAD TRUE ❌[2] >100
Not in workspace 1.18.10 0.11.0 FALSE ❌[3] ~10
Not in workspace 1.19.7 0.11.0 FALSE ❌[3] ~10
Not in workspace 1.20.2 0.11.0 FALSE ❌[3] ~10
Not in workspace 1.18.10 0.11.0 TRUE ❌[1] 0
Not in workspace 1.19.7 0.11.0 TRUE ❌[1] 0
Not in workspace 1.20.2 0.11.0 TRUE ❌[1] 0
Not in workspace 1.18.10 HEAD FALSE ❌[3] ~10
Not in workspace 1.19.7 HEAD FALSE ❌[3] ~10
Not in workspace 1.20.2 HEAD FALSE ❌[3] ~10
Not in workspace 1.18.10 HEAD TRUE ❌[2][3] ~10
Not in workspace 1.19.7 HEAD TRUE ❌[2][3] ~10
Not in workspace 1.20.2 HEAD TRUE ❌[2][3] ~10
In workspace 1.18.10 0.11.0 FALSE ❌[4] ~10 ❌[5]
In workspace 1.19.7 0.11.0 FALSE ❌[4] ~10 ❌[5]
In workspace 1.20.2 0.11.0 FALSE ❌[4] ~10 ❌[5]
In workspace 1.18.10 0.11.0 TRUE ❌[4] ❌[1] ~10 ❌[5]
In workspace 1.19.7 0.11.0 TRUE ❌[4] ❌[1] ~10 ❌[5]
In workspace 1.20.2 0.11.0 TRUE ❌[4] ❌[1] ~10 ❌[5]
In workspace 1.18.10 HEAD FALSE ❌[4] ~10 ❌[5]
In workspace 1.19.7 HEAD FALSE ❌[4] ~10 ❌[5]
In workspace 1.20.2 HEAD FALSE ❌[4] ~10 ❌[5]
In workspace 1.18.10 HEAD TRUE ❌[4] ❌[2] ~10 ❌[5]
In workspace 1.19.7 HEAD TRUE ❌[4] ❌[2] ~10 ❌[5]
In workspace 1.20.2 HEAD TRUE ❌[4] ❌[2] ~10 ❌[5]
Notes Description
❌[1] Go LSP pops a toast notification that EWM has been replaced, told to use Go workspaces. Note, this will not help the user unless the workspace is correctly configured.
❌[2] Go LSP pops a toast notification that EWM has been deprecated. No other information is provided.
❌[3] Go status bar indicates that there is an error loading the workspace, the repository has been cloned outside of GOPATH or a workspace.
❌[4] Make fails with an error that "no required module provides ...", tells the user that they must run "go get...", but this is not accurate. That go get command will fail.
❌[5] In these situations, imported packages succeeded if they were present in my Go module cache. Any import not in my go module cache failed, and code completion or go to definition for imports within the repository failed.

@findleyr
Copy link
Contributor

findleyr commented Apr 4, 2023

@AaronFriel thank you for that detailed analysis. We should check back in with this use-case as we improve error messages and implement #57979.

At present we're focused on scalability improvements that we also view as critical for monorepos and/or multi-module workspaces. Removing experimentalWorkspaceModule support was an important simplification to allow us to make progress on this goal. Furthermore, experimentalWorkspaceModule was deemed not to be the correct solution for multi-module workspaces, because it hides the fact that users may be working with different dependencies than are deployed in production. go.work files make that explicit.

That's not to say that gopls shouldn't just do the right thing when the user opens a Go file. It should, but it should use the same build list as the Go command run from that directory; if the user wants to work on multiple modules in the same logical build, they can use a go.work file. This is the motivation behind #57979.

v0.12.0 should include much better error messages, with quick-fixes to add and/or update your go.work. This is #53880. True "zero-config" multi-module workspaces as described in #57979 are slated for v0.13.0 over the summer. I understand your point that this is a degraded experience from v0.11.0+experimentalWorkspaceModule. All I can say is that we had little evidence that anyone was actually using experimentalWorkspaceModule outside of power users that would be comfortable adding a go.work file. I am sorry that this impacts your organization. I hope that we can work with you to ensure that you are able to make it easy for your developers to correctly configure their workspace.

[5] just looks like a bug, or some sort of misconfiguration. The initial workspace load should populate the module cache for any missing imports. Go to definition for imports within the repository should succeed for any module in your go.work file. Can you say more about this setup? You have many modules in nested directories, and a go.work file that uses them all?

@AaronFriel
Copy link
Author

AaronFriel commented Apr 4, 2023

Appreciate the status update and info. Would be very happy to contribute anything back here that would be helpful.

Re: [5]

No, this is for the use case that a user has inadvertently cloned the repository into a Go workspace but the go.work file does not reference the modules.

The 0.12 quick fix should improve that use case by providing them with a solution.

@findleyr findleyr added release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Apr 17, 2023
@adonovan
Copy link
Member

@findleyr This sounds like it could be related to either of the problems we identified yesterday:
(a) VSCode's slow processing of glob patterns consisting of very long comma-separated lists of directories (see #60089 and CL 496186);
(b) sequential execution of go mod tidy, and the fact that this is done even when the metadata hasn't changed.

@findleyr
Copy link
Contributor

We have added quick-fixes to add missing modules to a go.work file (creating one if necessary). Furthermore, we have fixed several performance issues related to working with a go.work file containing many modules.

For the [email protected] release, #57979 is the major feature.

@golang golang locked and limited conversation to collaborators May 24, 2024
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. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker 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