gopls/internal/lsp: eliminate diagnoseDetached

There is no reason to diagnose on a detached context. If the snapshot
background context is cancelled, it is either because the view is shut
down (and so we don't care about the results) or a subsequent snapshot
has been created. In the latter case, any results that may be published
using the detached context could be inaccurate, and we must overwrite
them with the later snapshot anyway.

Instead, use the diagnoseSnapshot helper, which runs on the snapshot
background context. In order to avoid unnecessary delay, lift the delay
value used in this method to the callsites. This should also speed up
go.mod related custom commands that were unnecessarily incurring the
delay.

Change-Id: I648e9ebcffb027efc76b74ca38354b9ad99da6e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487137
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/lsp/command.go

@@ -205,7 +205,7 @@ func (c *commandHandler) CheckUpgrades(ctx context.Context, args command.CheckUp
 		}
 		deps.snapshot.View().RegisterModuleUpgrades(args.URI.SpanURI(), upgrades)
 		// Re-diagnose the snapshot to publish the new module diagnostics.
-		c.s.diagnoseSnapshot(deps.snapshot, nil, false)
+		c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0)
 		return nil
 	})
 }
@@ -236,7 +236,7 @@ func (c *commandHandler) ResetGoModDiagnostics(ctx context.Context, args command
 		}
 
 		// Re-diagnose the snapshot to remove the diagnostics.
-		c.s.diagnoseSnapshot(deps.snapshot, nil, false)
+		c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0)
 		return nil
 	})
 }
@@ -713,7 +713,7 @@ func (c *commandHandler) ToggleGCDetails(ctx context.Context, args command.URIAr
 			c.s.gcOptimizationDetails[meta.ID] = struct{}{}
 		}
 		c.s.gcOptimizationDetailsMu.Unlock()
-		c.s.diagnoseSnapshot(deps.snapshot, nil, false)
+		c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0)
 		return nil
 	})
 }
@@ -914,7 +914,7 @@ func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.Vulnch
 		result.AsOf = time.Now()
 		deps.snapshot.View().SetVulnerabilities(args.URI.SpanURI(), &result)
 
-		c.s.diagnoseSnapshot(deps.snapshot, nil, false)
+		c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0)
 		vulns := result.Vulns
 		affecting := make([]string, 0, len(vulns))
 		for _, v := range vulns {

gopls/internal/lsp/diagnostics.go

@@ -24,7 +24,6 @@ import (
 	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
-	"golang.org/x/tools/internal/xcontext"
 )
 
 // TODO(rfindley): simplify this very complicated logic for publishing
@@ -150,34 +149,31 @@ func computeDiagnosticHash(diags ...*source.Diagnostic) string {
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
 
-// TODO(rfindley): is diagnoseDetached even necessary? We should always
-// eventually diagnose after a change. I don't see the value in ensuring that
-// the first diagnostics pass completes.
-func (s *Server) diagnoseDetached(snapshot source.Snapshot) {
-	ctx := snapshot.BackgroundContext()
-	ctx = xcontext.Detach(ctx)
-	s.diagnose(ctx, snapshot, analyzeOpenPackages)
-	s.publishDiagnostics(ctx, true, snapshot)
-}
-
 func (s *Server) diagnoseSnapshots(snapshots map[source.Snapshot][]span.URI, onDisk bool) {
 	var diagnosticWG sync.WaitGroup
 	for snapshot, uris := range snapshots {
 		diagnosticWG.Add(1)
 		go func(snapshot source.Snapshot, uris []span.URI) {
 			defer diagnosticWG.Done()
-			s.diagnoseSnapshot(snapshot, uris, onDisk)
+			s.diagnoseSnapshot(snapshot, uris, onDisk, snapshot.View().Options().DiagnosticsDelay)
 		}(snapshot, uris)
 	}
 	diagnosticWG.Wait()
 }
 
-func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.URI, onDisk bool) {
+// diagnoseSnapshot computes and publishes diagnostics for the given snapshot.
+//
+// If delay is non-zero, computing diagnostics does not start until after this
+// delay has expired, to allow work to be cancelled by subsequent changes.
+//
+// If changedURIs is non-empty, it is a set of recently changed files that
+// should be diagnosed immediately, and onDisk reports whether these file
+// changes came from a change to on-disk files.
+func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.URI, onDisk bool, delay time.Duration) {
 	ctx := snapshot.BackgroundContext()
 	ctx, done := event.Start(ctx, "Server.diagnoseSnapshot", source.SnapshotLabels(snapshot)...)
 	defer done()
 
-	delay := snapshot.View().Options().DiagnosticsDelay
 	if delay > 0 {
 		// 2-phase diagnostics.
 		//

gopls/internal/lsp/general.go

@@ -362,7 +362,7 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 		// Diagnose the newly created view asynchronously.
 		ndiagnose.Add(1)
 		go func() {
-			s.diagnoseDetached(snapshot)
+			s.diagnoseSnapshot(snapshot, nil, false, 0)
 			<-initialized
 			release()
 			ndiagnose.Done()
@@ -625,7 +625,7 @@ func (s *Server) exit(ctx context.Context) error {
 		// TODO: We should be able to do better than this.
 		os.Exit(1)
 	}
-	// we don't terminate the process on a normal exit, we just allow it to
+	// We don't terminate the process on a normal exit, we just allow it to
 	// close naturally if needed after the connection is closed.
 	return nil
 }

gopls/internal/lsp/workspace.go

@@ -71,7 +71,7 @@ func (s *Server) didChangeConfiguration(ctx context.Context, _ *protocol.DidChan
 				return // view is shut down; no need to diagnose
 			}
 			defer release()
-			s.diagnoseDetached(snapshot)
+			s.diagnoseSnapshot(snapshot, nil, false, 0)
 		}()
 	}