Skip to content

Commit

Permalink
gopls/internal/lsp: clear vuln diagnostics on config changes
Browse files Browse the repository at this point in the history
Clear vuln diagnostics before diagnosing views, during handling of
config changes. This should mostly avoid the symptoms of
golang/go#60465, but as noted in the code is not a proper fix.

Unfortunately there is no way to write a test for this behavior that is
not flaky, because the operation itself is flaky. I have scheduled a
proper fix for [email protected].

Fixes golang/go#60465

Change-Id: If41f5420c24dfa15a7d83e89988488619a2dd857
Reviewed-on: https://go-review.googlesource.com/c/tools/+/498560
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
  • Loading branch information
findleyr committed May 26, 2023
1 parent f3faea1 commit 5974258
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
4 changes: 4 additions & 0 deletions gopls/internal/lsp/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -2156,6 +2156,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
if !change.exists {
result.files.Delete(uri)
} else {
// TODO(golang/go#57558): the line below is strictly necessary to ensure
// that snapshots have each overlay, but it is problematic that we must
// set any content in snapshot.clone: if the file has changed, let it be
// re-read.
result.files.Set(uri, change.fileHandle)
}

Expand Down
22 changes: 21 additions & 1 deletion gopls/internal/lsp/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,30 @@ func (s *Server) didChangeConfiguration(ctx context.Context, _ *protocol.DidChan
if err := s.fetchConfig(ctx, view.Name(), view.Folder(), options); err != nil {
return err
}
view, err := s.session.SetViewOptions(ctx, view, options)
_, err := s.session.SetViewOptions(ctx, view, options)
if err != nil {
return err
}
}

// Now that all views have been updated: reset vulncheck diagnostics, rerun
// diagnostics, and hope for the best...
//
// TODO(golang/go#60465): this not a reliable way to ensure the correctness
// of the resulting diagnostics below. A snapshot could still be in the
// process of diagnosing the workspace, and not observe the configuration
// changes above.
//
// The real fix is golang/go#42814: we should create a new snapshot on any
// change that could affect the derived results in that snapshot. However, we
// are currently (2023-05-26) on the verge of a release, and the proper fix
// is too risky a change. Since in the common case a configuration change is
// only likely to occur during a period of quiescence on the server, it is
// likely that the clearing below will have the desired effect.
s.clearDiagnosticSource(modVulncheckSource)

for _, view := range s.session.Views() {
view := view
go func() {
snapshot, release, err := view.Snapshot()
if err != nil {
Expand Down

0 comments on commit 5974258

Please sign in to comment.