From 0d04f65da922c811c0d443e902ea62bbc27e5fd9 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 1 Aug 2022 17:30:10 -0400 Subject: [PATCH] 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 Reviewed-by: Hyang-Ah Hana Kim TryBot-Result: Gopher Robot gopls-CI: kokoro --- .../regtest/diagnostics/diagnostics_test.go | 21 +-- .../regtest/diagnostics/invalidation_test.go | 126 ++++++++++++++++++ .../regtest/misc/configuration_test.go | 2 +- .../regtest/template/template_test.go | 2 +- gopls/internal/regtest/watch/watch_test.go | 18 +-- internal/lsp/cache/session.go | 2 + internal/lsp/diagnostics.go | 61 +++++++-- internal/lsp/source/options.go | 14 ++ internal/lsp/text_synchronization.go | 19 +++ 9 files changed, 238 insertions(+), 27 deletions(-) create mode 100644 gopls/internal/regtest/diagnostics/invalidation_test.go diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index e1b55b5b13..f915fccbd1 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/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(), ) diff --git a/gopls/internal/regtest/diagnostics/invalidation_test.go b/gopls/internal/regtest/diagnostics/invalidation_test.go new file mode 100644 index 0000000000..ea65037644 --- /dev/null +++ b/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) + } + } + }) +} diff --git a/gopls/internal/regtest/misc/configuration_test.go b/gopls/internal/regtest/misc/configuration_test.go index 9629a2382e..433f96e855 100644 --- a/gopls/internal/regtest/misc/configuration_test.go +++ b/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{}{ diff --git a/gopls/internal/regtest/template/template_test.go b/gopls/internal/regtest/template/template_test.go index 0fdc3bda6f..292088ce59 100644 --- a/gopls/internal/regtest/template/template_test.go +++ b/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}}") diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index 2890f401a9..31655955e2 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/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"), ), ) }) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 372e8e9795..a77deb6211 100644 --- a/internal/lsp/cache/session.go +++ b/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 diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 460659f30d..642957cbad 100644 --- a/internal/lsp/diagnostics.go +++ b/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 diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index a695feaf26..8abcc7452c 100644 --- a/internal/lsp/source/options.go +++ b/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") diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 514834b077..dd6714553c 100644 --- a/internal/lsp/text_synchronization.go +++ b/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()