-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: panic with patch version in go.mod file #66195
Comments
Thank you for attaching the panicking stack. We should try to fix this for our next patch release (scheduled for Monday). |
(I should note, this may be fixed by upgrading to Go 1.22 and reinstalling gopls). |
go/types at go1.20 insisted on a version string of the form "go%d.%d", and would otherwise panic. gopls (since my https://go.dev/cl/557215 in January) permits strings of the form "go%d.%d.%d" as well, since later versions of go/types use the go/version.Lang operation to sanitize/normalize. I'll change gopls to respect the stricter semantics when compiled with go1.20. I will be so happy when we can drop support for these old toolchains. |
Indeed, that day can't come soon enough, particularly now that we've agreed on it! Thanks for investigating. |
@liumingmin do you have an idea of why gopls was compiled with go1.20.13, while your ambient go version is 1.21.6? gopls@v0.15.1 came out last week -- have you upgraded Go in the meantime? |
Ah, the problem occurs only when a gopls built with go1.20 runs a go list at go1.21+. Otherwise the go1.20 list would report the go.mod file as containing a syntax error. This is not going to fit easily into our test framework. |
Hmm, in general I don't think we can have forward Go command compatibility. We should probably fail loudly in this scenario. Originally I thought this was gopls compiled with go1.21.6. Given our new understanding of this problem, I think it would be OK not to fix this for gopls@v0.15.2: gopls needs to be recompiled. |
Change https://go.dev/cl/570135 mentions this issue: |
Change https://go.dev/cl/569879 mentions this issue: |
…c on version "go1.2.3" This change fixes a gopls panic caused by giving go/types@go.1.20 a Go version string with three components, e.g. go1.2.3. Unfortunately this is hard to write a test for, since it requires building gopls with go1.20 and running it with a go1.21 toolchain. Fixes golang/go#66195 Change-Id: I09257e6ded69568812b367ee80cafea30add93d3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/570135 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> (cherry picked from commit 9b64301) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569879 Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Robert Findley <rfindley@google.com>
This should be fixed by the next gopls prerelease (though reinstalling gopls with a more recent Go would also fix it anyway):
|
Change https://go.dev/cl/576135 mentions this issue: |
Fix the same crash as golang/go#66195, this time in Analyze: don't set invalid Go versions on the types.Config. The largest part of this change was writing a realistic test, since the lack of a test for golang/go#66195 caused us to miss this additional location. It was possible to create a test that worked, by using a flag to select a go1.21 binary location. For this test, it was required to move a couple additional integration test preconditions into integration.Main (otherwise, the test would be skipped due to the modified environment). Fixes golang/go#66636 Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135 Auto-Submit: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change https://go.dev/cl/577301 mentions this issue: |
…ot.Analyze with patch versions Fix the same crash as golang/go#66195, this time in Analyze: don't set invalid Go versions on the types.Config. The largest part of this change was writing a realistic test, since the lack of a test for golang/go#66195 caused us to miss this additional location. It was possible to create a test that worked, by using a flag to select a go1.21 binary location. For this test, it was required to move a couple additional integration test preconditions into integration.Main (otherwise, the test would be skipped due to the modified environment). Updates golang/go#66636 Updates golang/go#66730 Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135 Auto-Submit: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> (cherry picked from commit c623a28) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577301
gopls version: v0.15.1 (go1.20.13)
gopls flags:
update flags: proxy
extension version: 0.39.1
go version: 1.21.6
environment: Visual Studio Code win32
initialization error: undefined
issue timestamp: Fri, 08 Mar 2024 06:09:49 GMT
restart history:
Fri, 08 Mar 2024 02:16:19 GMT: activation (enabled: true)
ATTENTION: PLEASE PROVIDE THE DETAILS REQUESTED BELOW.
Describe what you observed.
OPTIONAL: If you would like to share more information, you can attach your complete gopls logs.
NOTE: THESE MAY CONTAIN SENSITIVE INFORMATION ABOUT YOUR CODEBASE.
DO NOT SHARE LOGS IF YOU ARE WORKING IN A PRIVATE REPOSITORY.
<OPTIONAL: ATTACH LOGS HERE>
The text was updated successfully, but these errors were encountered: