internal/lsp: propagate and handle context cancellation errors

We don't distinguish between genuine errors and context cancellation in
diagnostics, which often results in superfluous logging of these errors.
Avoid spamming the logs with them by checking.

Also, remove the logic for sending undelivered diagnostics. It's a relic
of old bugs and isn't useful.

Change-Id: I7df226539b9b1eb49ab3aae8d7b0a67f59fb3058
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210197
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/analysis.go

@@ -38,8 +38,7 @@ func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*analysis
 	for _, ah := range roots {
 		diagnostics, _, err := ah.analyze(ctx)
 		if err != nil {
-			log.Error(ctx, "no results", err)
-			continue
+			return nil, err
 		}
 		results = append(results, diagnostics...)
 	}
@@ -148,7 +147,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id packageID, mode source.P
 func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interface{}, error) {
 	v := act.handle.Get(ctx)
 	if v == nil {
-		return nil, nil, errors.Errorf("no analyses for %s", act.pkg.ID())
+		return nil, nil, ctx.Err()
 	}
 	data, ok := v.(*actionData)
 	if !ok {
@@ -160,21 +159,6 @@ func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interfac
 	return data.diagnostics, data.result, data.err
 }
 
-func (act *actionHandle) cached() ([]*source.Error, interface{}, error) {
-	v := act.handle.Cached()
-	if v == nil {
-		return nil, nil, errors.Errorf("no cached analyses for %s", act.pkg.ID())
-	}
-	data, ok := v.(*actionData)
-	if !ok {
-		return nil, nil, errors.Errorf("unexpected type for cached analysis %s:%s", act.pkg.ID(), act.analyzer.Name)
-	}
-	if data == nil {
-		return nil, nil, errors.Errorf("unexpected nil cached analysis for %s:%s", act.pkg.ID(), act.analyzer.Name)
-	}
-	return data.diagnostics, data.result, data.err
-}
-
 func buildActionKey(a *analysis.Analyzer, ph *packageHandle) string {
 	return hashContents([]byte(fmt.Sprintf("%p %s", a, string(ph.key))))
 }

internal/lsp/diagnostics.go

@@ -50,7 +50,7 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) {
 	}
 }
 
-func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) error {
+func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) {
 	ctx := snapshot.View().BackgroundContext()
 	ctx, done := trace.StartSpan(ctx, "lsp:background-worker")
 	defer done()
@@ -59,57 +59,51 @@ func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) error {
 
 	f, err := snapshot.View().GetFile(ctx, uri)
 	if err != nil {
-		return err
+		log.Error(ctx, "diagnoseFile: no file", err)
+		return
 	}
 	reports, warningMsg, err := source.Diagnostics(ctx, snapshot, f, true, snapshot.View().Options().DisabledAnalyses)
-	if err != nil {
-		return err
-	}
+	// Check the warning message first.
 	if warningMsg != "" {
 		s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
 			Type:    protocol.Info,
 			Message: warningMsg,
 		})
 	}
+	if err != nil {
+		if err != context.Canceled {
+			log.Error(ctx, "diagnoseFile: could not generate diagnostics", err)
+		}
+		return
+	}
 	// Publish empty diagnostics for files.
 	s.publishReports(ctx, reports, true)
-	return nil
 }
 
 func (s *Server) publishReports(ctx context.Context, reports map[source.FileIdentity][]source.Diagnostic, publishEmpty bool) {
-	undelivered := make(map[source.FileIdentity][]source.Diagnostic)
+	// Check for context cancellation before publishing diagnostics.
+	if ctx.Err() != nil {
+		return
+	}
+
 	for fileID, diagnostics := range reports {
+		// Don't deliver diagnostics if the context has already been canceled.
+		if ctx.Err() != nil {
+			break
+		}
 		// Don't publish empty diagnostics unless specified.
 		if len(diagnostics) == 0 && !publishEmpty {
 			continue
 		}
-		if err := s.publishDiagnostics(ctx, fileID, diagnostics); err != nil {
-			undelivered[fileID] = diagnostics
-
-			log.Error(ctx, "failed to deliver diagnostic (will retry)", err, telemetry.File)
+		if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
+			Diagnostics: toProtocolDiagnostics(ctx, diagnostics),
+			URI:         protocol.NewURI(fileID.URI),
+			Version:     fileID.Version,
+		}); err != nil {
+			log.Error(ctx, "failed to deliver diagnostic", err, telemetry.File)
 			continue
 		}
-		// In case we had old, undelivered diagnostics.
-		delete(undelivered, fileID)
 	}
-	// Any time we compute diagnostics, make sure to also send along any
-	// undelivered ones (only for remaining URIs).
-	for uri, diagnostics := range undelivered {
-		if err := s.publishDiagnostics(ctx, uri, diagnostics); err != nil {
-			log.Error(ctx, "failed to deliver diagnostic for (will not retry)", err, telemetry.File)
-		}
-
-		// If we fail to deliver the same diagnostics twice, just give up.
-		delete(undelivered, uri)
-	}
-}
-
-func (s *Server) publishDiagnostics(ctx context.Context, fileID source.FileIdentity, diagnostics []source.Diagnostic) error {
-	return s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
-		Diagnostics: toProtocolDiagnostics(ctx, diagnostics),
-		URI:         protocol.NewURI(fileID.URI),
-		Version:     fileID.Version,
-	})
 }
 
 func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) []protocol.Diagnostic {

internal/lsp/source/diagnostics.go

@@ -56,15 +56,13 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, withAnalysis bo
 	// not correctly configured. Report errors, if possible.
 	var warningMsg string
 	if len(ph.MissingDependencies()) > 0 {
-		warningMsg, err = checkCommonErrors(ctx, snapshot.View(), f.URI())
-		if err != nil {
+		if warningMsg, err = checkCommonErrors(ctx, snapshot.View(), f.URI()); err != nil {
 			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI))
 		}
 	}
 	pkg, err := ph.Check(ctx)
 	if err != nil {
-		log.Error(ctx, "no package for file", err)
-		return singleDiagnostic(fh.Identity(), "%s is not part of a package", f.URI()), "", nil
+		return nil, "", err
 	}
 	// Prepare the reports we will send for the files in this package.
 	reports := make(map[FileIdentity][]Diagnostic)
@@ -82,6 +80,10 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, withAnalysis bo
 	if !diagnostics(ctx, snapshot, pkg, reports) && withAnalysis {
 		// If we don't have any list, parse, or type errors, run analyses.
 		if err := analyses(ctx, snapshot, ph, disabledAnalyses, reports); err != nil {
+			// Exit early if the context has been canceled.
+			if err == context.Canceled {
+				return nil, "", err
+			}
 			log.Error(ctx, "failed to run analyses", err, telemetry.File.Of(f.URI()))
 		}
 	}

internal/lsp/watched_files.go

@@ -72,9 +72,10 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did
 
 				// If this was the only file in the package, clear its diagnostics.
 				if otherFile == nil {
-					if err := s.publishDiagnostics(ctx, source.FileIdentity{
-						URI: uri,
-					}, []source.Diagnostic{}); err != nil {
+					if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
+						URI:     protocol.NewURI(uri),
+						Version: fh.Identity().Version,
+					}); err != nil {
 						log.Error(ctx, "failed to clear diagnostics", err, telemetry.URI.Of(uri))
 					}
 					return nil