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: design UX around version skew resulting from the forward compatibility proposal #61185

Open
findleyr opened this issue Jul 5, 2023 · 9 comments
Assignees
Labels
gopls/metadata Issues related to metadata loading in gopls 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

@findleyr
Copy link
Contributor

findleyr commented Jul 5, 2023

This issue follows up on discussion in https://go.dev/cl/507975, where go/types is being updated to fail type-checking if the required Go version (as configured by types.Config.GoVersion) is greater than the Go version of the standard library (e.g. the version used to compile gopls).

This is the right change for go/types, because an older version of the type checker cannot type check code that was written for a newer version of Go.

However, we aim for an invariant in gopls that "if the Go command works, gopls works" (see also #57979). Previously, we didn't have to worry about the impact of version skew on this invariant, because the version of go in the user's environment is usually the same as the version of go used to compile gopls. So if gopls was producing confusing errors, the go command would also produce confusing errors.

But with forward compatibility (#57001), I think it will frequently be the case that the go command will work correctly, because it was automatically upgraded, yet gopls will fail.

We need to design and test an appropriate UX when users encounter this skew. Some questions to consider:

  • Should we endeavor to release a new version of gopls with go 1.N in its go.mod file, whenever version 1.N of go is released, so that go install golang.org/x/tools/gopls@latest will always have a version of gopls compatible with all published versions of Go?
  • Should we pop-up a message prompting the user to reinstall gopls, whenever a new Go version is published?
  • Should we pop-up a message prompting the user to reinstall gopls, whenever they work on code that is intended for a newer go version?
  • Should we allow gopls to still partially function, by artificially decreasing the version we pass as types.Config.GoVersion?
  • Should gopls always re-install itself with the latest toolchain, whenever a newer toolchain is available?

CC @golang/tools-team

@findleyr findleyr added this to the gopls/v0.13.0 milestone Jul 5, 2023
@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 Jul 5, 2023
@hyangah
Copy link
Contributor

hyangah commented Jul 5, 2023

In case of VS Code Go, the extension reads the build info of gopls and if the go version used to build the gopls is too old to handle the go version used in the project (i.e. minor version comparison), it shows a popup message and asks users to rebuild gopls. This is based on the assumption that gopls built with go1.N can work with go1.N and a few older versions of go. We didn't hear much about version skew issues since we implemented this feature. If gopls implements a similar logic (and vscode go should remove this check to avoid double popups), I think that would mostly work.

@findleyr
Copy link
Contributor Author

findleyr commented Jul 6, 2023

@hyangah does VS Code actually read the go.mod file to get its go version?

@rsc
Copy link
Contributor

rsc commented Jul 6, 2023

I don't see in the list above the option of gopls rebuilding and reinvoking itself as needed. It could do something like

GOBIN=/some/gopls/cache/go1.N GOTOOLCHAIN=go1.N go install gopls@latest

and then reinvoke /some/gopls/cache/go1.N/gopls.

@findleyr
Copy link
Contributor Author

findleyr commented Jul 6, 2023

@rsc I considered that, but there's a chicken and egg problem which would require careful treatment: we can't know the required Go version until we've resolved the user's workspace and environment, which happens during the LSP handshake. It's doable, but would require a fair bit of work to splice in a different gopls version during a session.

However, we could just say that "gopls is always built with the latest toolchain", and upgrade whenever there is a newer toolchain available. Added that as an option.

I'm a bit concerned about having gopls download and install new binaries (even if those binaries are go/gopls itself). At least with the forward compatibility proposal there is an explicit act of running the Go command on a module that requires a newer toolchain. In this case, the upgrade would be triggered by the presence of a newer version, which feels different. Of course, some IDEs auto-upgrade binaries, but that is thus far not a bridge we have crossed with gopls, which should generally be more conservative than any individual IDE. It may have to be opt-in.

@hyangah
Copy link
Contributor

hyangah commented Jul 6, 2023

@findleyr

does VS Code actually read the go.mod file to get its go version?

Extension runs go version from the workspace root. My understanding is that with the extended forward compatibility work, go version should do the right thing (download/find the right tool chains depending on the GOTOOLCHAIN setup) and report the actually used version.

Complicated cases include, however, multi-module workspaces, nested module, multi-root (vscode's) workspaces, zero-config gopls handling multiple modules, etc. And, in case, users update go.mod file (either the 'go' line, or by updating the dependencies) in the middle of a session. In that case, gopls will be in a better position to detect that and notify editors/users.

@hyangah
Copy link
Contributor

hyangah commented Jul 6, 2023

Re: gopls self-updating. Another aspect we need to keep in mind is the compatibility between the editor and the gopls. LSP sometimes makes incompatible changes and we also sometimes have custom gopls commands. Editors and users (can) have all the information about the compatibility. I am afraid go and even gopls may not want to know about editors' versions.

@findleyr
Copy link
Contributor Author

findleyr commented Jul 6, 2023

@hyangah to be clear I think gopls should only rebuild itself, not upgrade itself. For example, right now I am working with a user who is stuck on v0.11.0, because they are running into performance edge cases of v0.12.x. It's already frustrating enough when we break users, and we would never want to re-break them when they intentionally downgrade!

@hyangah
Copy link
Contributor

hyangah commented Jul 7, 2023

Note that in many editors, the editor (or its plugin) manages the lifetime of the language server and expects the connection with the language server (over stdio) to be stable after the initial hand-shake. And the initial hand-shake needs to be done within a certain deadline.

We observed a similar pattern with aqua (golang/vscode-go#2229) and it turned out to require pretty delicate handling of processes. (aquaproj/aqua#710)

@adonovan adonovan added the gopls/metadata Issues related to metadata loading in gopls label Aug 31, 2023
@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.15.0 Oct 9, 2023
@stapelberg
Copy link
Contributor

I think I’m hitting this very issue.

I have go1.21.5 installed, but decided to try a Go 1.22 feature (the servemux change) for a specific project.

This is my transcript:

% git clone https://github.com/robustirc/robustirc
% cd robustirc
% gopls version
golang.org/x/tools/gopls v0.14.2
    golang.org/x/tools/gopls@v0.14.2 h1:sIw6vjZiuQ9S7s0auUUkHlWgsCkKZFWDHmrge8LYsnc=
% emacs robustirc.go # jump-to-definition for api.NewHTTP(…) works

% echo 'toolchain go1.22rc1' >> go.mod
% grep '^go ' go.mod
go 1.20
% emacs robustirc.go # jump-to-definition still works

% sed -i 's,go 1.20,go 1.21,g' go.mod
% grep '^go ' go.mod                 
go 1.21
% emacs robustirc.go # jump-to-definition still works

% sed -i 's,go 1.21,go 1.22,g' go.mod  
% grep '^go ' go.mod                 
go 1.22
% emacs robustirc.go # jump-to-definition for api.NewHTTP(…) now broken

So the triggering condition seems to be that the language version and toolchain directive both are on Go 1.22, while gopls was built with Go 1.21.5.

Re-installing gopls with Go 1.22 fixes the problem, but it was not obvious that version skew was what broke gopls (there was some time between switching to Go 1.22 and the next time I needed a gopls feature).

Is there anything we can do to at least produce a good error message?

(PS: If this is a separate issue after all, feel free to split it into a new one or let me know and I can file a separate one.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/metadata Issues related to metadata loading in gopls 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

6 participants