Merge pull request #2779 from bhcleek/lsp/highlight/after-save

lsp: keep diagnostics highlighting on format

Author: bhcleek

autoload/go/config_test.vim

@@ -97,6 +97,7 @@ func! Test_GoplsEnabled_Clear() abort
 
   finally
     unlet g:go_gopls_enabled
+    call delete(l:tmp, 'rf')
   endtry
 endfunc
 " restore Vi compatibility settings

autoload/go/fmt.vim

@@ -64,7 +64,8 @@ function! go#fmt#Format(withGoimport) abort
 
   let current_col = col('.')
   let [l:out, l:err] = go#fmt#run(l:bin_name, l:tmpname, expand('%'))
-  let diff_offset = len(readfile(l:tmpname)) - line('$')
+  let line_offset = len(readfile(l:tmpname)) - line('$')
+  let l:orig_line = getline('.')
 
   if l:err == 0
     call go#fmt#update_file(l:tmpname, expand('%'))
@@ -92,8 +93,10 @@ function! go#fmt#Format(withGoimport) abort
     call winrestview(l:curw)
   endif
 
-  " be smart and jump to the line the new statement was added/removed
-  call cursor(line('.') + diff_offset, current_col)
+  " be smart and jump to the line the new statement was added/removed and
+  " adjust the column within the line
+  let l:lineno = line('.') + line_offset
+  call cursor(l:lineno, current_col + (len(getline(l:lineno)) - len(l:orig_line)))
 
   " Syntax highlighting breaks less often.
   syntax sync fromstart
@@ -117,11 +120,6 @@ function! go#fmt#update_file(source, target)
     call setfperm(a:target , original_fperm)
   endif
 
-  " reset diagnostic matches before reloading so that any locations that have
-  " been invalidated by formatting won't cause errors when the buffer is
-  " reloaded from disk.
-  let b:go_diagnostic_matches = {'errors': [], 'warnings': []}
-
   " reload buffer to reflect latest changes
   silent edit!
 

autoload/go/guru.vim

@@ -484,7 +484,11 @@ function! s:same_ids_highlight(exit_val, output, mode) abort
     " is redisplayed: e.g. :edit, :GoRename, etc.
     augroup vim-go-sameids
       autocmd! * <buffer>
-      autocmd BufWinEnter <buffer> nested call go#guru#SameIds(0)
+      if has('textprop')
+        autocmd BufReadPost <buffer> nested call go#guru#SameIds(0)
+      else
+        autocmd BufWinEnter <buffer> nested call go#guru#SameIds(0)
+      endif
     augroup end
   endif
 endfunction

autoload/go/highlight_test.vim

@@ -270,6 +270,153 @@ function! s:numericHighlightGroupInSliceSlicing(testname, from, to)
   endtry
 endfunction
 
+function! Test_diagnostic_after_fmt() abort
+  let g:go_fmt_command = 'gofmt'
+  try
+    call s:diagnostic_after_write( [
+          \ 'package main',
+          \ 'import "fmt"',
+          \ '',
+          \ 'func main() {',
+          \ '',
+          \ "\tfmt.Println(\x1fhello)",
+          \ '}',
+          \ ], [])
+  finally
+    unlet g:go_fmt_command
+  endtry
+endfunction
+
+function! Test_diagnostic_after_fmt_change() abort
+  " craft a file that will be changed when its written (gofmt will change it).
+  let g:go_fmt_command = 'gofmt'
+  try
+    call s:diagnostic_after_write( [
+          \ 'package main',
+          \ 'import "fmt"',
+          \ '',
+          \ 'func main() {',
+          \ '',
+          \ "fmt.Println(\x1fhello)",
+          \ '}',
+          \ ], [])
+  finally
+    unlet g:go_fmt_command
+  endtry
+endfunction
+
+function! Test_diagnostic_after_fmt_cleared() abort
+  " craft a file that will be fixed when it is written.
+  let g:go_fmt_command = 'gofmt'
+  try
+    call s:diagnostic_after_write( [
+          \ 'package main',
+          \ 'import "fmt"',
+          \ '',
+          \ 'func main() {',
+          \ '',
+          \ "fmt.Println(\x1fhello)",
+          \ '}',
+          \ ], ['hello := "hello, vim-go"'])
+  finally
+    unlet g:go_fmt_command
+  endtry
+endfunction
+
+function! Test_diagnostic_after_reload() abort
+  let l:dir = gotest#write_file('diagnostic/after-reload.go', [
+              \ 'package main',
+              \ 'import "fmt"',
+              \ '',
+              \ 'func main() {',
+              \ '',
+              \ "\tfmt.Println(\x1fhello)",
+              \ '}',
+              \ ])
+  try
+    call s:check_diagnostics('', 'goDiagnosticError', 'initial')
+    let l:pos = getcurpos()
+    edit
+    call setpos('.', l:pos)
+    call s:check_diagnostics('', 'goDiagnosticError', 'after-reload')
+  finally
+    call delete(l:dir, 'rf')
+  endtry
+endfunction
+
+function! s:diagnostic_after_write(contents, changes) abort
+  syntax on
+
+  let l:dir = gotest#write_file('diagnostic/after-write.go', a:contents)
+
+  try
+    let l:pos = getcurpos()
+    call s:check_diagnostics('', 'goDiagnosticError', 'initial')
+
+    " write a:changes to the previous line and make sure l:actual and
+    " l:expected are set so that they won't accidentally match on the next
+    " check.
+    if len(a:changes) > 0
+      call append(l:pos[1]-1, a:changes)
+      let l:actual = 'goDiagnosticError'
+      let l:expected = ''
+    else
+      let l:actual = ''
+      let l:expected = 'goDiagnosticError'
+    endif
+
+    write
+
+    call s:check_diagnostics(l:actual, l:expected, 'after-write')
+  finally
+    call delete(l:dir, 'rf')
+  endtry
+endfunction
+
+function! s:check_diagnostics(actual, expected, when)
+  let l:actual = a:actual
+  let l:start = reltime()
+
+  while l:actual != a:expected && reltimefloat(reltime(l:start)) < 10
+    " Get the cursor position on each iteration, because the cursor postion
+    " may change between iterations when go#fmt#GoFmt formats, reloads the
+    " file, and moves the cursor to try to keep it where the user expects it
+    " to be when gofmt modifies the files.
+    let l:pos = getcurpos()
+    if !has('textprop')
+      let l:matches = getmatches()
+      if len(l:matches) == 0
+        let l:actual = ''
+      endif
+
+      for l:m in l:matches
+        let l:matchline = l:m.pos1[0]
+        if len(l:m.pos1) < 2
+          continue
+        endif
+        let l:matchcol = get(l:m.pos1, 1, 1)
+        if l:pos[1] == l:matchline && l:pos[2] >= l:matchcol && l:pos[2] <= l:matchcol + l:m.pos1[2]
+        " Ideally, we'd check that the cursor is within the match, but when a
+        " tab is added on the current line, the cursor position within the
+        " line will stay constant while the line itself is shifted over by a
+        " column, so just check the line itself instead of checking a precise
+        " cursor location.
+        " if l:pos[1] == l:matchline
+          let l:actual = l:m.group
+          break
+        endif
+      endfor
+
+      sleep 100m
+      continue
+    endif
+
+    let l:actual = get(prop_list(l:pos[1]), 0, {'type': ''}).type
+    sleep 100m
+  endwhile
+
+  call assert_equal(a:expected, l:actual, a:when)
+endfunction
 " restore Vi compatibility settings
 let &cpo = s:cpo_save
 unlet s:cpo_save

autoload/go/lsp.vim

@@ -240,13 +240,11 @@ function! s:newlsp() abort
           if !bufexists(l:bufname)
             "let l:starttime = reltime()
             call bufadd(l:bufname)
-            "echom printf('added %s (%s)', l:bufname, reltimestr(reltime(l:startime)))
           endif
 
           if !bufloaded(l:bufname)
             "let l:starttime = reltime()
             call bufload(l:bufname)
-            "echom printf('loaded %s (%s)', l:bufname, reltimestr(reltime(l:starttime)))
           endif
 
           for l:diag in l:data.diagnostics
@@ -273,9 +271,11 @@ function! s:newlsp() abort
           " version.
           let l:lsp = s:lspfactory.get()
           let l:version = get(l:lsp.fileVersions, l:fname, 0)
-          if l:version != 0 && (!has_key(l:data, 'version') || l:data.version == l:version)
-            call s:highlightMatches(l:errorMatches, l:warningMatches)
-          endif
+          " it's tempting to only highlight matches when they are for the
+          " current version of the buffer, but that causes problems when the
+          " version number has been updated and the content has not. In such a
+          " case, the diagnostics may not be sent for later versions.
+          call s:highlightMatches(l:errorMatches, l:warningMatches)
         endif
 
         let self.diagnostics[l:fname] = l:diagnostics
@@ -1269,11 +1269,15 @@ function! s:highlightMatches(errorMatches, warningMatches) abort
   augroup vim-go-diagnostics
     autocmd! * <buffer>
     autocmd BufDelete <buffer> autocmd! vim-go-diagnostics * <buffer=abuf>
-    autocmd BufWinEnter <buffer> nested call s:highlightMatches(b:go_diagnostic_matches.errors, b:go_diagnostic_matches.warnings)
+    if has('textprop')
+      autocmd BufReadPost <buffer> nested call s:highlightMatches(b:go_diagnostic_matches.errors, b:go_diagnostic_matches.warnings)
+    else
+      autocmd BufWinEnter <buffer> nested call s:highlightMatches(b:go_diagnostic_matches.errors, b:go_diagnostic_matches.warnings)
+    endif
   augroup end
 endfunction
 
-" ClearDiagnosticsHighlights removes all goDiagnosticError and
+" ClearDiagnosticHighlights removes all goDiagnosticError and
 " goDiagnosticWarning matches.
 function! go#lsp#ClearDiagnosticHighlights() abort
   call go#util#ClearHighlights('goDiagnosticError')

autoload/go/util.vim

@@ -559,7 +559,7 @@ function! go#util#SetEnv(name, value) abort
 endfunction
 
 function! go#util#ClearHighlights(group) abort
-  if exists('*prop_remove')
+  if has('textprop')
     " the property type may not exist when syntax highlighting is not enabled.
     if empty(prop_type_get(a:group))
       return
@@ -617,7 +617,7 @@ endfunction
 " pos should be a list of 3 element lists. The lists should be [line, col,
 " length] as used by matchaddpos().
 function! go#util#HighlightPositions(group, pos) abort
-  if exists('*prop_add')
+  if has('textprop')
     for l:pos in a:pos
       " use a single line prop by default
       let l:prop = {'type': a:group, 'length': l:pos[2]}

autoload/gotest.vim

@@ -12,9 +12,6 @@ set cpo&vim
 " The full path to the created directory is returned, it is the caller's
 " responsibility to clean that up!
 fun! gotest#write_file(path, contents) abort
-  if go#util#has_job()
-    call go#lsp#CleanWorkspaces()
-  endif
   let l:dir = go#util#tempdir("vim-go-test/testrun/")
   let $GOPATH .= ':' . l:dir
   let l:full_path = l:dir . '/src/' . a:path
@@ -23,10 +20,6 @@ fun! gotest#write_file(path, contents) abort
   call writefile(a:contents, l:full_path)
   exe 'cd ' . l:dir . '/src'
 
-  if go#util#has_job()
-    call go#lsp#AddWorkspaceDirectory(fnamemodify(l:full_path, ':p:h'))
-  endif
-
   silent exe 'e! ' . a:path
 
   " Set cursor.

ftplugin/go.vim

@@ -99,25 +99,29 @@ augroup vim-go-buffer
   autocmd BufWritePre <buffer> call go#auto#fmt_autosave()
   autocmd BufWritePost <buffer> call go#auto#metalinter_autosave()
 
-  "TODO(bc): how to clear sameids and diagnostics when a non-go buffer is
-  " loaded into a window and the previously loaded buffer is still loaded in
-  " another window?
-
-  " clear SameIds when the buffer is unloaded from its last window so that
-  " loading another buffer (especially of a different filetype) in the same
-  " window doesn't highlight the most recently matched identifier's positions.
-  autocmd BufWinLeave <buffer> call go#guru#ClearSameIds()
-  " clear SameIds when a new buffer is loaded in the window so that the
-  " previous buffer's highlighting isn't used.
-  autocmd BufWinEnter <buffer> call go#guru#ClearSameIds()
-
-  " clear diagnostics when the buffer is unloaded from its last window so that
-  " loading another buffer (especially of a different filetype) in the same
-  " window doesn't highlight th previously loaded buffer's diagnostics.
-  autocmd BufWinLeave <buffer> call go#lsp#ClearDiagnosticHighlights()
-  " clear diagnostics when a new buffer is loaded in the window so that the
-  " previous buffer's diagnostics aren't used.
-  autocmd BufWinEnter <buffer> call go#lsp#ClearDiagnosticHighlights()
+  if !has('textprop')
+    "TODO(bc): how to clear sameids and diagnostics when a non-go buffer is
+    " loaded into a window and the previously loaded buffer is still loaded in
+    " another window?
+
+    " TODO(bc): only clear when the new buffer isn't the old buffer
+
+    " clear SameIds when the buffer is unloaded from its last window so that
+    " loading another buffer (especially of a different filetype) in the same
+    " window doesn't highlight the most recently matched identifier's positions.
+    autocmd BufWinLeave <buffer> call go#guru#ClearSameIds()
+    " clear SameIds when a new buffer is loaded in the window so that the
+    " previous buffer's highlighting isn't used.
+    autocmd BufWinEnter <buffer> call go#guru#ClearSameIds()
+
+    " clear diagnostics when the buffer is unloaded from its last window so that
+    " loading another buffer (especially of a different filetype) in the same
+    " window doesn't highlight the previously loaded buffer's diagnostics.
+    autocmd BufWinLeave <buffer> call go#lsp#ClearDiagnosticHighlights()
+    " clear diagnostics when a new buffer is loaded in the window so that the
+    " previous buffer's diagnostics aren't used.
+    "autocmd BufWinEnter <buffer> call go#lsp#ClearDiagnosticHighlights()
+  endif
 
   autocmd BufEnter <buffer>
         \  if go#config#AutodetectGopath() && !exists('b:old_gopath')

plugin/go.vim

@@ -38,8 +38,7 @@ call s:checkVersion()
 " these packages are used by vim-go and can be automatically installed if
 " needed by the user with GoInstallBinaries.
 
-" NOTE(bc): varying the binary name and the tail of the import path (e.g.
-" gocode-gomod) does not yet work in module aware mode.
+" NOTE(bc): varying the binary name and the tail of the import path does not yet work in module aware mode.
 let s:packages = {
       \ 'asmfmt':        ['github.com/klauspost/asmfmt/cmd/asmfmt@master'],
       \ 'dlv':           ['github.com/go-delve/delve/cmd/dlv@master'],

syntax/go.vim

@@ -399,7 +399,7 @@ function! s:hi()
   " filetype plugin on, the highlight groups won't be defined when
   " ftplugin/go.vim is executed when the first go file is opened.
   " See https://github.com/fatih/vim-go/issues/2658.
-  if exists('*prop_type_add')
+  if has('textprop')
     if empty(prop_type_get('goSameId'))
       call prop_type_add('goSameId', {'highlight': 'goSameId'})
     endif