cmd/govim: safely handle busy state changes for non-.go buffers (#823)

In 8926dec we backed out the changes for tracking all buffer changes.
But in so doing we didn't properly handle the situation where the busy
state changes and the cursor is in a non-go file.

Fix that and add a test to verify we don't regress.

Whilst we are at it:

* consistently use GOVIM_test_SetUserBusy in our testscript tests
* slightly refactor the types/consts used for getting the current cursor
  position

Fixes #821

cmd/govim/highlight.go

@@ -121,38 +121,52 @@ func (v *vimstate) redefineHighlights(force bool) error {
 }
 
 func (v *vimstate) referenceHighlight(flags govim.CommandFlags, args ...string) error {
-	return v.updateReferenceHighlight(true)
+	return v.updateReferenceHighlight(true, nil)
 }
 
 // removeReferenceHighlight is used to only remove existing reference highlights if the
 // cursor has moved outside of the range(s) of existing highlights, to avoid flickering
-func (v *vimstate) removeReferenceHighlight() error {
+func (v *vimstate) removeReferenceHighlight(cursorPos cursorPosition) error {
 	if len(v.currentReferences) == 0 {
 		return nil
 	}
 
-	_, pos, err := v.cursorPos()
-	if err != nil {
-		return fmt.Errorf("failed to get current position: %v", err)
+	var pos *types.Point
+	if b, ok := v.buffers[cursorPos.BufNr]; ok {
+		point, err := types.PointFromVim(b, cursorPos.Line, cursorPos.Col)
+		if err == nil {
+			pos = &point
+		}
 	}
 
 	for i := range v.currentReferences {
-		if !pos.IsWithin(*v.currentReferences[i]) {
+		if pos == nil || !pos.IsWithin(*v.currentReferences[i]) {
 			v.removeTextProps(types.ReferencesTextPropID)
 			return nil
 		}
 	}
 	return nil
 }
 
-func (v *vimstate) updateReferenceHighlight(force bool) error {
+// updateReferenceHighlight updates the reference highlighting if the cursor is
+// currently in a valid position, i.e. a go file. The cursor position can passed
+// in by supplying a non-nil cursorPos.
+func (v *vimstate) updateReferenceHighlight(force bool, cursorPos *cursorPosition) error {
 	if !force && (v.config.HighlightReferences == nil || !*v.config.HighlightReferences) {
 		return nil
 	}
 
-	b, pos, err := v.cursorPos()
+	if cursorPos == nil {
+		v.Parse(v.ChannelExpr(cursorPositionExpr), &cursorPos)
+	}
+	b, ok := v.buffers[(*cursorPos).BufNr]
+	if !ok {
+		// we are not tracking this buffer... i.e. is not a .go file
+		return nil
+	}
+	pos, err := types.PointFromVim(b, cursorPos.Line, cursorPos.Col)
 	if err != nil {
-		return fmt.Errorf("failed to get current position: %v", err)
+		return fmt.Errorf("failed to calculate cursor position in buffer %v from line %v and col %v: %v", b.Num, cursorPos.Line, cursorPos.Col, err)
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())

cmd/govim/main.go

@@ -295,7 +295,7 @@ func (g *govimplugin) Init(gg govim.Govim, errCh chan error) error {
 	g.DefineFunction(string(config.FunctionBufChanged), []string{"bufnr", "start", "end", "added", "changes"}, g.vimstate.bufChanged)
 	g.DefineFunction(string(config.FunctionSetConfig), []string{"config"}, g.vimstate.setConfig)
 	g.ChannelExf(`call govim#config#Set("%vFunc", function("%v%v"))`, config.InternalFunctionPrefix, PluginPrefix, config.FunctionSetConfig)
-	g.DefineFunction(string(config.FunctionSetUserBusy), []string{"isBusy"}, g.vimstate.setUserBusy)
+	g.DefineFunction(string(config.FunctionSetUserBusy), []string{"isBusy", "cursorPos"}, g.vimstate.setUserBusy)
 	g.DefineFunction(string(config.FunctionPopupSelection), []string{"id", "selected"}, g.vimstate.popupSelection)
 	g.DefineCommand(string(config.CommandReferences), g.vimstate.references)
 	g.DefineCommand(string(config.CommandRename), g.vimstate.rename, govim.NArgsZeroOrOne)

cmd/govim/testdata/scenario_default/busy_state_change_non_go_file.txt

@@ -0,0 +1,21 @@
+# Test that a busy state change (from busy to not) in a non go file
+# succeeds. This ensures we are correctly non trying to handle events
+# in non .go files
+
+vim ex 'call GOVIM_test_SetUserBusy(1)'
+vim ex 'e main.go'
+vim ex 'e other.txt'
+vim ex 'call GOVIM_test_SetUserBusy(0)'
+
+# Assert that we have received no error (Type: 1) or warning (Type: 2) log messages
+# Disabled pending resolution to https://github.com/golang/go/issues/34103
+# errlogmatch -start -count=0 'LogMessage callback: &protocol\.LogMessageParams\{Type:(1|2), Message:".*'
+
+-- go.mod --
+module mod.com
+
+go 1.12
+-- main.go --
+package main
+-- other.txt --
+hello

cmd/govim/testdata/scenario_default/reference_highlight.txt

@@ -1,20 +1,20 @@
 # Tests that references to the identifier at the cursor is being highlighted when the user
-# goes idle. Since user idle detection is disabled in tests, GOVIM_internal_SetUserBusy()
+# goes idle. Since user idle detection is disabled in tests, GOVIM_test_SetUserBusy()
 # is invoked directly.
 
 vim ex 'e main.go'
 
 # Placing the cursor on "foo" should highlight all other occations of foo
 vim ex 'call cursor(5,5)'
-vim ex 'call GOVIM_internal_SetUserBusy(1)'
-vim ex 'call GOVIM_internal_SetUserBusy(0)'
+vim ex 'call GOVIM_test_SetUserBusy(1)'
+vim ex 'call GOVIM_test_SetUserBusy(0)'
 vimexprwait foo_references.golden 'map(range(1,line(\"$\")), \"prop_list(v:val)\")'
 
 
 # Placing the cursor on "fmt" should highlight all other occations of fmt
 vim ex 'call cursor(9,2)'
-vim ex 'call GOVIM_internal_SetUserBusy(1)'
-vim ex 'call GOVIM_internal_SetUserBusy(0)'
+vim ex 'call GOVIM_test_SetUserBusy(1)'
+vim ex 'call GOVIM_test_SetUserBusy(0)'
 vim ex 'w'
 vimexprwait fmt_references.golden 'map(range(1,line(\"$\")), \"prop_list(v:val)\")'
 

cmd/govim/util.go

@@ -24,17 +24,21 @@ func (v *vimstate) currentBufferInfo(expr json.RawMessage) *types.Buffer {
 	return types.NewBuffer(buf.Num, buf.Name, []byte(buf.Contents), buf.Loaded == 1)
 }
 
+type cursorPosition struct {
+	BufNr int `json:"bufnr"`
+	Line  int `json:"line"`
+	Col   int `json:"col"`
+}
+
+const cursorPositionExpr = `{"bufnr": bufnr(""), "line": line("."), "col": col(".")}`
+
 func (v *vimstate) cursorPos() (b *types.Buffer, p types.Point, err error) {
-	var pos struct {
-		BufNum int `json:"bufnum"`
-		Line   int `json:"line"`
-		Col    int `json:"col"`
-	}
-	expr := v.ChannelExpr(`{"bufnum": bufnr(""), "line": line("."), "col": col(".")}`)
+	var pos cursorPosition
+	expr := v.ChannelExpr(cursorPositionExpr)
 	v.Parse(expr, &pos)
-	b, ok := v.buffers[pos.BufNum]
+	b, ok := v.buffers[pos.BufNr]
 	if !ok {
-		err = fmt.Errorf("failed to resolve buffer %v", pos.BufNum)
+		err = fmt.Errorf("failed to resolve buffer %v", pos.BufNr)
 		return
 	}
 	p, err = types.PointFromVim(b, pos.Line, pos.Col)

cmd/govim/vimstate.go

@@ -122,7 +122,7 @@ func (v *vimstate) setConfig(args ...json.RawMessage) (interface{}, error) {
 			// HighlightReferences is now not on - remove existing text properties
 			v.removeTextProps(types.ReferencesTextPropID)
 		} else {
-			if err := v.updateReferenceHighlight(true); err != nil {
+			if err := v.updateReferenceHighlight(true, nil); err != nil {
 				return nil, fmt.Errorf("failed to update reference highlight: %v", err)
 			}
 		}
@@ -168,15 +168,15 @@ func (v *vimstate) popupSelection(args ...json.RawMessage) (interface{}, error)
 }
 
 func (v *vimstate) setUserBusy(args ...json.RawMessage) (interface{}, error) {
-	var isBusy int
-	v.Parse(args[0], &isBusy)
-	v.userBusy = isBusy != 0
+	v.userBusy = v.ParseInt(args[0]) != 0
+	var pos cursorPosition
+	v.Parse(args[1], &pos)
 
 	if v.userBusy {
-		return nil, v.removeReferenceHighlight()
+		return nil, v.removeReferenceHighlight(pos)
 	}
 
-	if err := v.updateReferenceHighlight(false); err != nil {
+	if err := v.updateReferenceHighlight(false, &pos); err != nil {
 		return nil, err
 	}
 

plugin/govim.vim

@@ -195,7 +195,7 @@ endfunction
 function s:userBusy(busy)
   if s:userBusy != a:busy
     let s:userBusy = a:busy
-    call GOVIM_internal_SetUserBusy(s:userBusy)
+    call GOVIM_internal_SetUserBusy(s:userBusy, {"bufnr": bufnr(""), "line": line("."), "col": col(".")})
   endif
 endfunction