internal/lsp: re-send diagnostics on file events

Fix golang/go#50267 by ensuring that diagnostics are re-sent following
didOpen or didClose events. Additionally, introduce a new hidden
'chattyDiagnostics' option that causes diagnostics to be resent on
*every* file change event. This latter option is for LSP clients that
get confused when diagnostics are not re-sent for later file versions.
For now, be conservative and only force diagnostic publication on
didOpen and didClose.

Update tests whose 'NoDiagnostics' assertions were broken by the new
behavior.

Fixes golang/go#50267

Change-Id: I6332d66a1851e0d8261599d37020a03b4c598f7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420539
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -384,7 +384,7 @@ func main() {}
 			// completed.
 			OnceMet(
 				env.DoneWithChange(),
-				NoDiagnostics("a.go"),
+				EmptyDiagnostics("a.go"),
 			),
 		)
 	})
@@ -757,15 +757,20 @@ func _() {
 		env.OpenFile("a/a1.go")
 		env.CreateBuffer("a/a2.go", ``)
 		env.SaveBufferWithoutActions("a/a2.go")
+		// We can't use OnceMet here (at least, not easily) because the didSave
+		// races with the didChangeWatchedFiles.
+		//
+		// TODO(rfindley): add an AllOf expectation combinator, or an expectation
+		// that all notifications have been processed.
 		env.Await(
-			OnceMet(
-				env.DoneWithSave(),
-				NoDiagnostics("a/a1.go"),
-			),
+			EmptyDiagnostics("a/a1.go"),
 		)
 		env.EditBuffer("a/a2.go", fake.NewEdit(0, 0, 0, 0, `package a`))
 		env.Await(
-			OnceMet(env.DoneWithChange(), NoDiagnostics("a/a1.go")),
+			OnceMet(
+				env.DoneWithChange(),
+				EmptyDiagnostics("a/a1.go"),
+			),
 		)
 	})
 }
@@ -914,7 +919,7 @@ var _ = foo.Bar
 		env.Await(
 			OnceMet(
 				env.DoneWithOpen(),
-				NoDiagnostics("_foo/x.go"),
+				EmptyDiagnostics("_foo/x.go"),
 			))
 	})
 }
@@ -1730,7 +1735,7 @@ package b
 			env.Await(
 				OnceMet(
 					env.DoneWithOpen(),
-					NoDiagnostics("a/a.go"),
+					EmptyDiagnostics("a/a.go"),
 				),
 				NoOutstandingWork(),
 			)

gopls/internal/regtest/diagnostics/invalidation_test.go

@@ -0,0 +1,126 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package diagnostics
+
+import (
+	"fmt"
+	"testing"
+
+	"golang.org/x/tools/internal/lsp/protocol"
+	. "golang.org/x/tools/internal/lsp/regtest"
+)
+
+// Test for golang/go#50267: diagnostics should be re-sent after a file is
+// opened.
+func TestDiagnosticsAreResentAfterCloseOrOpen(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.com
+
+go 1.16
+-- main.go --
+package main
+
+func _() {
+	x := 2
+}
+`
+	Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file.
+		env.OpenFile("main.go")
+		var afterOpen protocol.PublishDiagnosticsParams
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				ReadDiagnostics("main.go", &afterOpen),
+			),
+		)
+		env.CloseBuffer("main.go")
+		var afterClose protocol.PublishDiagnosticsParams
+		env.Await(
+			OnceMet(
+				env.DoneWithClose(),
+				ReadDiagnostics("main.go", &afterClose),
+			),
+		)
+		if afterOpen.Version == afterClose.Version {
+			t.Errorf("publishDiagnostics: got the same version after closing (%d) as after opening", afterOpen.Version)
+		}
+		env.OpenFile("main.go")
+		var afterReopen protocol.PublishDiagnosticsParams
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				ReadDiagnostics("main.go", &afterReopen),
+			),
+		)
+		if afterReopen.Version == afterClose.Version {
+			t.Errorf("pubslishDiagnostics: got the same version after reopening (%d) as after closing", afterClose.Version)
+		}
+	})
+}
+
+// Test for the "chattyDiagnostics" setting: we should get re-published
+// diagnostics after every file change, even if diagnostics did not change.
+func TestChattyDiagnostics(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.com
+
+go 1.16
+-- main.go --
+package main
+
+func _() {
+	x := 2
+}
+
+// Irrelevant comment #0
+`
+
+	WithOptions(
+		Settings{
+			"chattyDiagnostics": true,
+		},
+	).Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file.
+
+		env.OpenFile("main.go")
+		var d protocol.PublishDiagnosticsParams
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				ReadDiagnostics("main.go", &d),
+			),
+		)
+
+		if len(d.Diagnostics) != 1 {
+			t.Fatalf("len(Diagnostics) = %d, want 1", len(d.Diagnostics))
+		}
+		msg := d.Diagnostics[0].Message
+
+		for i := 0; i < 5; i++ {
+			before := d.Version
+			env.RegexpReplace("main.go", "Irrelevant comment #.", fmt.Sprintf("Irrelevant comment #%d", i))
+			env.Await(
+				OnceMet(
+					env.DoneWithChange(),
+					ReadDiagnostics("main.go", &d),
+				),
+			)
+
+			if d.Version == before {
+				t.Errorf("after change, got version %d, want new version", d.Version)
+			}
+
+			// As a sanity check, make sure we have the same diagnostic.
+			if len(d.Diagnostics) != 1 {
+				t.Fatalf("len(Diagnostics) = %d, want 1", len(d.Diagnostics))
+			}
+			newMsg := d.Diagnostics[0].Message
+			if newMsg != msg {
+				t.Errorf("after change, got message %q, want %q", newMsg, msg)
+			}
+		}
+	})
+}

gopls/internal/regtest/misc/configuration_test.go

@@ -37,7 +37,7 @@ var FooErr = errors.New("foo")
 		env.OpenFile("a/a.go")
 		env.Await(
 			env.DoneWithOpen(),
-			NoDiagnostics("a/a.go"),
+			EmptyDiagnostics("a/a.go"),
 		)
 		cfg := env.Editor.Config()
 		cfg.Settings = map[string]interface{}{

gopls/internal/regtest/template/template_test.go

@@ -133,7 +133,7 @@ go 1.12
 		env.Await(
 			OnceMet(
 				env.DoneWithOpen(),
-				NoDiagnostics("hello.tmpl"), // Don't get spurious errors for empty templates.
+				EmptyDiagnostics("hello.tmpl"), // Don't get spurious errors for empty templates.
 			),
 		)
 		env.SetBufferContent("hello.tmpl", "{{range .Planets}}\nHello {{}}\n{{end}}")

gopls/internal/regtest/watch/watch_test.go

@@ -139,7 +139,7 @@ func _() {
 		})
 		env.Await(
 			EmptyDiagnostics("a/a.go"),
-			NoDiagnostics("b/b.go"),
+			EmptyOrNoDiagnostics("b/b.go"),
 		)
 	})
 }
@@ -341,12 +341,12 @@ func _() {
 			env.Await(
 				OnceMet(
 					env.DoneWithChangeWatchedFiles(),
-					NoDiagnostics("a/a.go"),
+					EmptyOrNoDiagnostics("a/a.go"),
 				),
 			)
 			env.WriteWorkspaceFile("b/b.go", newMethod)
 			env.Await(
-				NoDiagnostics("a/a.go"),
+				EmptyOrNoDiagnostics("a/a.go"),
 			)
 		})
 	})
@@ -360,9 +360,9 @@ func _() {
 			env.Await(
 				OnceMet(
 					env.DoneWithChangeWatchedFiles(),
-					NoDiagnostics("a/a.go"),
+					EmptyOrNoDiagnostics("a/a.go"),
 				),
-				NoDiagnostics("b/b.go"),
+				EmptyOrNoDiagnostics("b/b.go"),
 			)
 		})
 	})
@@ -715,11 +715,11 @@ func TestAll(t *testing.T) {
 		env.Await(
 			OnceMet(
 				env.DoneWithChangeWatchedFiles(),
-				NoDiagnostics("a/a.go"),
+				EmptyOrNoDiagnostics("a/a.go"),
 			),
 			OnceMet(
 				env.DoneWithChangeWatchedFiles(),
-				NoDiagnostics("a/a_test.go"),
+				EmptyOrNoDiagnostics("a/a_test.go"),
 			),
 		)
 		// Now, add a new file to the test variant and use its symbol in the
@@ -747,11 +747,11 @@ func TestSomething(t *testing.T) {}
 		env.Await(
 			OnceMet(
 				env.DoneWithChangeWatchedFiles(),
-				NoDiagnostics("a/a_test.go"),
+				EmptyOrNoDiagnostics("a/a_test.go"),
 			),
 			OnceMet(
 				env.DoneWithChangeWatchedFiles(),
-				NoDiagnostics("a/a2_test.go"),
+				EmptyOrNoDiagnostics("a/a2_test.go"),
 			),
 		)
 	})

internal/lsp/cache/session.go

@@ -413,6 +413,8 @@ func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModifica
 	return err
 }
 
+// TODO(rfindley): fileChange seems redundant with source.FileModification.
+// De-dupe into a common representation for changes.
 type fileChange struct {
 	content    []byte
 	exists     bool

internal/lsp/diagnostics.go

@@ -41,17 +41,42 @@ const (
 
 // A diagnosticReport holds results for a single diagnostic source.
 type diagnosticReport struct {
-	snapshotID    uint64
-	publishedHash string
+	snapshotID    uint64 // snapshot ID on which the report was computed
+	publishedHash string // last published hash for this (URI, source)
 	diags         map[string]*source.Diagnostic
 }
 
 // fileReports holds a collection of diagnostic reports for a single file, as
 // well as the hash of the last published set of diagnostics.
 type fileReports struct {
-	snapshotID    uint64
+	// publishedSnapshotID is the last snapshot ID for which we have "published"
+	// diagnostics (though the publishDiagnostics notification may not have
+	// actually been sent, if nothing changed).
+	//
+	// Specifically, publishedSnapshotID is updated to a later snapshot ID when
+	// we either:
+	//  (1) publish diagnostics for the file for a snapshot, or
+	//  (2) determine that published diagnostics are valid for a new snapshot.
+	//
+	// Notably publishedSnapshotID may not match the snapshot id on individual reports in
+	// the reports map:
+	// - we may have published partial diagnostics from only a subset of
+	//   diagnostic sources for which new results have been computed, or
+	// - we may have started computing reports for an even new snapshot, but not
+	//   yet published.
+	//
+	// This prevents gopls from publishing stale diagnostics.
+	publishedSnapshotID uint64
+
+	// publishedHash is a hash of the latest diagnostics published for the file.
 	publishedHash string
-	reports       map[diagnosticSource]diagnosticReport
+
+	// If set, mustPublish marks diagnostics as needing publication, independent
+	// of whether their publishedHash has changed.
+	mustPublish bool
+
+	// The last stored diagnostics for each diagnostic source.
+	reports map[diagnosticSource]diagnosticReport
 }
 
 func (d diagnosticSource) String() string {
@@ -358,6 +383,24 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg
 	}
 }
 
+// mustPublishDiagnostics marks the uri as needing publication, independent of
+// whether the published contents have changed.
+//
+// This can be used for ensuring gopls publishes diagnostics after certain file
+// events.
+func (s *Server) mustPublishDiagnostics(uri span.URI) {
+	s.diagnosticsMu.Lock()
+	defer s.diagnosticsMu.Unlock()
+
+	if s.diagnostics[uri] == nil {
+		s.diagnostics[uri] = &fileReports{
+			publishedHash: hashDiagnostics(), // Hash for 0 diagnostics.
+			reports:       map[diagnosticSource]diagnosticReport{},
+		}
+	}
+	s.diagnostics[uri].mustPublish = true
+}
+
 // storeDiagnostics stores results from a single diagnostic source. If merge is
 // true, it merges results into any existing results for this snapshot.
 func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsource diagnosticSource, diags []*source.Diagnostic) {
@@ -367,6 +410,7 @@ func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsourc
 	if fh == nil {
 		return
 	}
+
 	s.diagnosticsMu.Lock()
 	defer s.diagnosticsMu.Unlock()
 	if s.diagnostics[uri] == nil {
@@ -512,7 +556,7 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 
 		// If we've already delivered diagnostics for a future snapshot for this
 		// file, do not deliver them.
-		if r.snapshotID > snapshot.ID() {
+		if r.publishedSnapshotID > snapshot.ID() {
 			continue
 		}
 		anyReportsChanged := false
@@ -541,10 +585,10 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 		}
 		source.SortDiagnostics(diags)
 		hash := hashDiagnostics(diags...)
-		if hash == r.publishedHash {
+		if hash == r.publishedHash && !r.mustPublish {
 			// Update snapshotID to be the latest snapshot for which this diagnostic
 			// hash is valid.
-			r.snapshotID = snapshot.ID()
+			r.publishedSnapshotID = snapshot.ID()
 			continue
 		}
 		var version int32
@@ -558,7 +602,8 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 		}); err == nil {
 			published++
 			r.publishedHash = hash
-			r.snapshotID = snapshot.ID()
+			r.mustPublish = false // diagnostics have been successfully published
+			r.publishedSnapshotID = snapshot.ID()
 			for dsource, hash := range reportHashes {
 				report := r.reports[dsource]
 				report.publishedHash = hash

internal/lsp/source/options.go

@@ -573,6 +573,16 @@ type InternalOptions struct {
 	// that the new algorithm has worked, and write some summary
 	// statistics to a file in os.TmpDir()
 	NewDiff string
+
+	// ChattyDiagnostics controls whether to report file diagnostics for each
+	// file change. If unset, gopls only reports diagnostics when they change, or
+	// when a file is opened or closed.
+	//
+	// TODO(rfindley): is seems that for many clients this should be true by
+	// default. For example, coc.nvim seems to get confused if diagnostics are
+	// not re-published. Switch the default to true after some period of internal
+	// testing.
+	ChattyDiagnostics bool
 }
 
 type ImportShortcut string
@@ -805,6 +815,7 @@ func (o *Options) EnableAllExperiments() {
 	o.ExperimentalUseInvalidMetadata = true
 	o.ExperimentalWatchedFileDelay = 50 * time.Millisecond
 	o.NewDiff = "checked"
+	o.ChattyDiagnostics = true
 }
 
 func (o *Options) enableAllExperimentMaps() {
@@ -1075,6 +1086,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 	case "newDiff":
 		result.setString(&o.NewDiff)
 
+	case "chattyDiagnostics":
+		result.setBool(&o.ChattyDiagnostics)
+
 	// Replaced settings.
 	case "experimentalDisabledAnalyses":
 		result.deprecated("analyses")

internal/lsp/text_synchronization.go

@@ -286,16 +286,35 @@ func (s *Server) processModifications(ctx context.Context, modifications []sourc
 		return errors.New("server is shut down")
 	}
 	s.stateMu.Unlock()
+
 	// If the set of changes included directories, expand those directories
 	// to their files.
 	modifications = s.session.ExpandModificationsToDirectories(ctx, modifications)
 
+	// Build a lookup map for file modifications, so that we can later join
+	// with the snapshot file associations.
+	modMap := make(map[span.URI]source.FileModification)
+	for _, mod := range modifications {
+		modMap[mod.URI] = mod
+	}
+
 	snapshots, release, err := s.session.DidModifyFiles(ctx, modifications)
 	if err != nil {
 		close(diagnoseDone)
 		return err
 	}
 
+	// golang/go#50267: diagnostics should be re-sent after an open or close. For
+	// some clients, it may be helpful to re-send after each change.
+	for snapshot, uris := range snapshots {
+		for _, uri := range uris {
+			mod := modMap[uri]
+			if snapshot.View().Options().ChattyDiagnostics || mod.Action == source.Open || mod.Action == source.Close {
+				s.mustPublishDiagnostics(uri)
+			}
+		}
+	}
+
 	go func() {
 		s.diagnoseSnapshots(snapshots, onDisk)
 		release()