gopls/internal/lsp: clear vuln diagnostics on config changes

findleyr · findleyr · commit 5974258e689a · 2023-05-26T18:36:54.000Z
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 gopls@v0.12.1. Fixes golang/go#60465 Change-Id: If41f5420c24dfa15a7d83e89988488619a2dd857 Reviewed-on: https://go-review.googlesource.com/c/tools/+/498560 gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
@@ -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)
 		}
 
diff --git a/gopls/internal/lsp/workspace.go b/gopls/internal/lsp/workspace.go
@@ -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 {
