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: stale diagnostic, possibly from an orphaned workspace package #43347

Closed
findleyr opened this issue Dec 23, 2020 · 2 comments
Closed
Assignees
Labels
FrozenDueToAge gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Dec 23, 2020

I encountered a strange and concerning bug in gopls yesterday (using gopls/v0.6.1), which presented as follows:

While working on the regtests, I typed e.Sandbox.RunGoCommand (a function), at which point I got a go/types diagnostic for the unused ExprStmt. I then kept typing, but the "is unused" diagnostic stuck around. Subsequently editing and saving the file did not eliminate or move the diagnostic -- it was republished for each snapshot. My first thought is that this is a bug in diagnostic caching, but I don't yet see how that could be possible and the following evidence suggests otherwise: I have a diagnostic delay configured and the initial publish after editing (which only diagnoses the PackagesForFile) cleared the erroneous diagnosic, then it came back on the subsequent delayed diagnostics pass, which diagnoses all workspace packages.

We (@stamblerre and I) therefore suspected that I somehow had an orphaned workspace package which is no longer associated with the file I was editing. If this were possible we would not invalidate this typechecked package upon edits to the file, its stale diagnostics would persist across snapshots, and it would not be part of the PackagesForFile package set and therefore wouldn't be present in the initial diagnostics pass. In other words, the symptoms would be exactly what I observed. On the other hand I don't yet see how a package could be orphaned in such a way, and nothing I did while editing the file would have triggered abnormal code paths (in my RPC logs there are no nearby packages.Load calls that would have affected package associations).

So after staring at this for a bit, I'm currently at a loss. While the bug persisted in my gopls session, I unfortunately did not have the debug server enabled and was unsurprisingly not able to reproduce in a new session. Eventually, after an ~hour in the broken session, the stale diagnostic just went away.

This is likely a rare enough bug that it is relatively low impact, but its nature demonstrates that some invariant of the gopls server is being broken. I therefore think we should proceed by instrumenting gopls to assert loudly on the invariants we expect: logging errors in production builds and crashing in development builds.

CC @stamblerre @heschik @pjweinbgo

@findleyr findleyr added this to the gopls/v0.6.2 milestone Dec 23, 2020
@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 Dec 23, 2020
@stamblerre stamblerre added this to Needs Triage in vscode-go: gopls by default via automation Dec 23, 2020
@gopherbot
Copy link

Change https://golang.org/cl/280698 mentions this issue: internal/lsp/cache: add a check for snapshot invariants

gopherbot pushed a commit to golang/tools that referenced this issue Dec 30, 2020
Check that some invariants are met when cloning the snapshot, in an
attempt to catch golang/go#43347

For golang/go#43347

Change-Id: I7404509027a1b0b0085133cba4d21d1006a52a57
Reviewed-on: https://go-review.googlesource.com/c/tools/+/280698
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre moved this from Needs Triage to Non-critical in vscode-go: gopls by default Jan 6, 2021
@stamblerre stamblerre removed this from Non-critical in vscode-go: gopls by default Jan 6, 2021
@stamblerre stamblerre added this to To Do in gopls on-deck Feb 28, 2021
@findleyr findleyr added Testing An issue that has been verified to require only test changes, not just a test failure. gopls/metadata Issues related to metadata loading in gopls and removed gopls/testing labels May 10, 2022
@findleyr findleyr modified the milestones: gopls/later, gopls/v0.9.2 Aug 3, 2022
@findleyr
Copy link
Contributor Author

findleyr commented Aug 3, 2022

I believe this is fixed now, because we derive workspace packages directly from metadata. Or at least if it still exists, the mechanism by which it is produced is entirely new.

@findleyr findleyr closed this as completed Aug 3, 2022
@findleyr findleyr self-assigned this Aug 8, 2022
@golang golang locked and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

3 participants