cmd/govim: de-dupe PublishDiagnostics notifications from gopls (#282)

This helps to throttle the number of changes we try to push through to
the quickfix window.

However, golang/go#32443 remains an issue in
as much as incomplete diagnostics are sent to the client. i.e. we can't
be sure to receive the most recent diagnostics for the file which we are
editing (and in which we know, for example, we are making errors).

cmd/govim/functions.go

@@ -6,7 +6,6 @@ import (
 	"github.com/myitcv/govim"
 	"github.com/myitcv/govim/cmd/govim/config"
 	"github.com/myitcv/govim/cmd/govim/internal/lsp/protocol"
-	"github.com/myitcv/govim/cmd/govim/internal/span"
 	"github.com/myitcv/govim/cmd/govim/types"
 	"github.com/myitcv/govim/internal/plugin"
 )
@@ -24,10 +23,6 @@ type vimstate struct {
 	// events, rather than via open Buffers in Vim
 	watchedFiles map[string]*types.WatchedFile
 
-	// diagnostics gives us the current diagnostics by URI
-	diagnostics        map[span.URI][]protocol.Diagnostic
-	diagnosticsChanged bool
-
 	// jumpStack is akin to the Vim concept of a tagstack
 	jumpStack    []protocol.Location
 	jumpStackPos int

cmd/govim/gopls_client.go

@@ -59,21 +59,37 @@ func (g *govimplugin) Event(context.Context, *interface{}) error {
 
 func (g *govimplugin) PublishDiagnostics(ctxt context.Context, params *protocol.PublishDiagnosticsParams) error {
 	g.logGoplsClientf("PublishDiagnostics callback: %v", pretty.Sprint(params))
-	g.Schedule(func(govim.Govim) error {
-		v := g.vimstate
-		// TODO improve the efficiency of this. When we are watching files not yet loaded
-		// in Vim, we will likely have a reference to the file in vimstate. At this point
-		// we will need a lookup from URI -> Buffer which might give us nothing, at which
-		// point we fall back to the file. For now we simply check buffers first, then fall
-		// back to loading the file from disk
+	g.diagnosticsLock.Lock()
+	defer g.diagnosticsLock.Unlock()
 
-		v.diagnostics[span.URI(params.URI)] = params.Diagnostics
-		v.diagnosticsChanged = true
+	uri := span.URI(params.URI)
+	curr, ok := g.diagnostics[uri]
+	updt := params.Diagnostics
+	if !ok {
+		g.diagnostics[uri] = updt
+		if len(params.Diagnostics) > 0 {
+			goto Schedule
+		} else {
+			return nil
+		}
+	}
+	if len(curr) != len(updt) {
+		g.diagnostics[uri] = updt
+		goto Schedule
+	}
+	if len(curr) == 0 {
+		return nil
+	}
+	// Let's not try and be too clever for now diff diagnostics.
+	// Instead be pessimistic.
+	g.diagnostics[uri] = updt
 
-		if v.config.QuickfixAutoDiagnosticsDisable {
+Schedule:
+	g.Schedule(func(govim.Govim) error {
+		if g.vimstate.config.QuickfixAutoDiagnosticsDisable {
 			return nil
 		}
-		return v.updateQuickfix()
+		return g.vimstate.updateQuickfix()
 	})
 	return nil
 }

cmd/govim/main.go

@@ -13,6 +13,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/myitcv/govim"
@@ -146,18 +147,22 @@ type govimplugin struct {
 	tomb tomb.Tomb
 
 	modWatcher *modWatcher
+
+	// diagnostics gives us the current diagnostics by URI
+	diagnostics     map[span.URI][]protocol.Diagnostic
+	diagnosticsLock sync.Mutex
 }
 
 func newplugin(goplspath string) *govimplugin {
 	d := plugin.NewDriver(PluginPrefix)
 	res := &govimplugin{
-		goplspath: goplspath,
-		Driver:    d,
+		diagnostics: make(map[span.URI][]protocol.Diagnostic),
+		goplspath:   goplspath,
+		Driver:      d,
 		vimstate: &vimstate{
 			Driver:       d,
 			buffers:      make(map[int]*types.Buffer),
 			watchedFiles: make(map[string]*types.WatchedFile),
-			diagnostics:  make(map[span.URI][]protocol.Diagnostic),
 		},
 	}
 	res.vimstate.govimplugin = res

cmd/govim/quickfix.go

@@ -7,6 +7,7 @@ import (
 	"sort"
 
 	"github.com/myitcv/govim"
+	"github.com/myitcv/govim/cmd/govim/internal/lsp/protocol"
 	"github.com/myitcv/govim/cmd/govim/internal/span"
 	"github.com/myitcv/govim/cmd/govim/types"
 )
@@ -23,14 +24,15 @@ func (v *vimstate) quickfixDiagnostics(flags govim.CommandFlags, args ...string)
 }
 
 func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
-	defer func() {
-		v.diagnosticsChanged = false
-	}()
-	if !v.diagnosticsChanged {
-		return nil
+	v.diagnosticsLock.Lock()
+	diags := make(map[span.URI][]protocol.Diagnostic)
+	for k, v := range v.diagnostics {
+		diags[k] = v
 	}
+	v.diagnosticsLock.Unlock()
+
 	var fns []span.URI
-	for u := range v.diagnostics {
+	for u := range diags {
 		fns = append(fns, u)
 	}
 	sort.Slice(fns, func(i, j int) bool {
@@ -45,7 +47,7 @@ func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
 
 	// now update the quickfix window based on the current diagnostics
 	for _, uri := range fns {
-		diags := v.diagnostics[uri]
+		diags := diags[uri]
 		fn, err := uri.Filename()
 		if err != nil {
 			v.Logf("updateQuickfix: failed to resolve filename from URI %q: %v", uri, err)