cmd/govim: refine quickfix population (#228)

* Provide an option to disable auto-population of the quickfix window
from gopls diagnotics
* Provide CommandQuickfixDiagnostics to manually trigger the population
of the quickfix window with diagnostics
* Make quickfix population from diagnostics best-efforts, i.e. skip on
any errors (like invalid positions)
* Move some user-level settings out of the plugin shim into
minimal.vimrc
* Whilst we are at it, fix a small bug where we were not calling
undojoin prior to any formatting/goimports fixes

cmd/govim/config/config.go

@@ -1,12 +1,23 @@
+// Package config declares the configuration variables, functions and commands
+// used by govim
 package config
 
 const (
 	GlobalPrefix = "g:govim_"
 
 	// GlobalFormatOnSave is a string value variable that configures which tool
-	// to use for formatting on save.  Options are given by constants of type
-	// FormatOnSave
+	// to use for formatting on save. Options are given by constants of type
+	// FormatOnSave. Default: FormatOnSaveGoImports.
 	GlobalFormatOnSave = GlobalPrefix + "format_on_save"
+
+	// GlobalQuickfixAutoDiagnosticsDisable is a boolean (0 or 1 in VimScript)
+	// variable that controls whether auto-population of the quickfix window
+	// with gopls diagnostics is disabled or not. When not disabled, govim waits
+	// for updatetime (help updatetime) before populating the quickfix window
+	// with the current gopls diagnostics. When disabled, the
+	// CommandQuickfixDiagnostics command can be used to manually trigger the
+	// population. Default: false (0)
+	GlobalQuickfixAutoDiagnosticsDisable = GlobalPrefix + "quickfix_auto_diagnotics_disable"
 )
 
 type Command string
@@ -32,6 +43,10 @@ const (
 	// CommandGoImports applies goimports to the selected range, or the entire
 	// file if no range is provided
 	CommandGoImports Command = "GoImports"
+
+	// CommandQuickfixDiagnostics populates the quickfix window with the current
+	// gopls-reported diagnostics
+	CommandQuickfixDiagnostics Command = "QuickfixDiagnostics"
 )
 
 type Function string

cmd/govim/config/minimal.vimrc

@@ -1,6 +1,7 @@
-" This file represents the minimal .vimrc needed to work with govim
-" It is used as part of the automated tests for govim and so will
-" always be current
+" This file represents the minimal .vimrc needed to work with govim.
+"
+" We also include a number of suggested settings that we think the majority of
+" users will like/prefer.
 
 set nocompatible
 set nobackup
@@ -20,3 +21,13 @@ set mouse=a
 " does not work correctly beyond a certain column number (citation needed)
 " hence we use ttymouse=sgr
 set ttymouse=sgr
+
+" Suggestion: By default, govim populates the quickfix window with diagnostics
+" reported by gopls after a period of inactivity, the time period being
+" defined by updatetime (help updatetime). Here we suggest a short updatetime
+" time in order that govim/Vim are more responsive/IDE-like
+set updatetime=500
+
+" Suggestion: To make govim/Vim more responsive/IDE-like, we suggest a short
+" balloondelay
+set balloondelay=250

cmd/govim/format.go

@@ -162,6 +162,7 @@ func (v *vimstate) formatBufferRange(b *types.Buffer, mode config.FormatOnSave,
 				return fmt.Errorf("saw an edit where start line != end line with replacement text %q; We can't currently handle this", e.NewText)
 			}
 			// This is a delete of line
+			v.ChannelEx("undojoin")
 			if res := v.ParseInt(v.ChannelCall("deletebufline", b.Num, start.Line(), end.Line()-1)); res != 0 {
 				return fmt.Errorf("deletebufline(%v, %v, %v) failed", b.Num, start.Line(), end.Line()-1)
 			}
@@ -175,6 +176,7 @@ func (v *vimstate) formatBufferRange(b *types.Buffer, mode config.FormatOnSave,
 				e.NewText = e.NewText[:len(e.NewText)-1]
 			}
 			repl := strings.Split(e.NewText, "\n")
+			v.ChannelEx("undojoin")
 			v.ChannelCall("append", start.Line()-1, repl)
 		}
 	}

cmd/govim/main.go

@@ -153,10 +153,11 @@ func (g *govimplugin) Init(gg govim.Govim, errCh chan error) error {
 	g.DefineCommand(string(config.CommandGoToDef), g.gotoDef, govim.NArgsZeroOrOne)
 	g.DefineCommand(string(config.CommandGoToPrevDef), g.gotoPrevDef, govim.NArgsZeroOrOne, govim.CountN(1))
 	g.DefineFunction(string(config.FunctionHover), []string{}, g.hover)
-	g.DefineAutoCommand("", govim.Events{govim.EventCursorHold, govim.EventCursorHoldI}, govim.Patterns{"*.go"}, false, g.updateQuickfix)
+	g.DefineAutoCommand("", govim.Events{govim.EventCursorHold, govim.EventCursorHoldI}, govim.Patterns{"*.go"}, false, g.autoUpdateQuickfix)
 	g.DefineAutoCommand("", govim.Events{govim.EventBufDelete}, govim.Patterns{"*.go"}, false, g.deleteCurrentBuffer, "eval(expand('<abuf>'))")
 	g.DefineCommand(string(config.CommandGoFmt), g.gofmtCurrentBufferRange, govim.RangeFile)
 	g.DefineCommand(string(config.CommandGoImports), g.goimportsCurrentBufferRange, govim.RangeFile)
+	g.DefineCommand(string(config.CommandQuickfixDiagnostics), g.quickfixDiagnostics)
 
 	g.isGui = g.ParseInt(g.ChannelExpr(`has("gui_running")`)) == 1
 

cmd/govim/quickfix.go

@@ -2,11 +2,12 @@ package main
 
 import (
 	"encoding/json"
-	"fmt"
 	"io/ioutil"
 	"path/filepath"
 	"sort"
 
+	"github.com/myitcv/govim"
+	"github.com/myitcv/govim/cmd/govim/config"
 	"github.com/myitcv/govim/cmd/govim/internal/span"
 	"github.com/myitcv/govim/cmd/govim/types"
 )
@@ -18,6 +19,19 @@ type quickfixEntry struct {
 	Text     string `json:"text"`
 }
 
+func (v *vimstate) quickfixDiagnostics(flags govim.CommandFlags, args ...string) error {
+	return v.updateQuickfix()
+}
+
+func (v *vimstate) autoUpdateQuickfix(args ...json.RawMessage) error {
+	// TODO this double round-trip is not very efficient
+	if v.ParseInt(v.ChannelExprf("exists(%q)", config.GlobalQuickfixAutoDiagnosticsDisable)) != 0 &&
+		v.ParseInt(v.ChannelExpr(config.GlobalQuickfixAutoDiagnosticsDisable)) != 0 {
+		return nil
+	}
+	return v.updateQuickfix()
+}
+
 func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
 	defer func() {
 		v.diagnosticsChanged = false
@@ -44,7 +58,8 @@ func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
 		diags := v.diagnostics[uri]
 		fn, err := uri.Filename()
 		if err != nil {
-			return fmt.Errorf("failed to resolve filename from URI %q: %v", uri, err)
+			v.Logf("updateQuickfix: failed to resolve filename from URI %q: %v", uri, err)
+			continue
 		}
 		var buf *types.Buffer
 		for _, b := range v.buffers {
@@ -55,7 +70,8 @@ func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
 		if buf == nil {
 			byts, err := ioutil.ReadFile(fn)
 			if err != nil {
-				return fmt.Errorf("failed to read contents of %v: %v", fn, err)
+				v.Logf("updateQuickfix: failed to read contents of %v: %v", fn, err)
+				continue
 			}
 			// create a temp buffer
 			buf = &types.Buffer{
@@ -67,12 +83,14 @@ func (v *vimstate) updateQuickfix(args ...json.RawMessage) error {
 		// make fn relative for reporting purposes
 		fn, err = filepath.Rel(cwd, fn)
 		if err != nil {
-			return fmt.Errorf("failed to call filepath.Rel(%q, %q): %v", cwd, fn, err)
+			v.Logf("updateQuickfix: failed to call filepath.Rel(%q, %q): %v", cwd, fn, err)
+			continue
 		}
 		for _, d := range diags {
 			p, err := types.PointFromPosition(buf, d.Range.Start)
 			if err != nil {
-				return fmt.Errorf("failed to resolve position: %v", err)
+				v.Logf("updateQuickfix: failed to resolve position: %v", err)
+				continue
 			}
 			fixes = append(fixes, quickfixEntry{
 				Filename: fn,

plugin/govim.vim

@@ -25,10 +25,8 @@ let s:plugindir = expand(expand("<sfile>:p:h:h"))
 let s:govim_status = "loading"
 let s:loadStatusCallbacks = []
 
-set balloondelay=250
 set ballooneval
 set balloonevalterm
-set updatetime=500
 
 " TODO these probably doesn't belong here?
 let g:govim_format_on_save = "goimports"