Repository: autozimu/LanguageClient-neovim (language: rust)

Issue:
Error in vim: "Unknown function: nvim_buf_add_highlight"
- Did you upgrade to latest plugin version? Yes
- Did you upgrade to/compile latest binary? Run shell command
  `bin/languageclient --version` to get its version number.
- (Neovim users only) Did you check output of `:checkhealth LanguageClient`? N/A
- Did you check [troubleshooting]? Yes

[troubleshooting]: https://github.com/autozimu/LanguageClient-neovim/blob/next/INSTALL.md#troubleshooting

## Describe the bug

When running `:call LanguageClient#textDocument_documentHighlight()` (either manually or via an autocommand) in Vim 8.2, I get the following error:

```
[LC] Error: Failure { jsonrpc: Some(V2), error: Error { code: InternalError, message: "\'Vim(let):E117: Unknown function: nvim_buf_add_highlight\'", data: None }, id: Num(42) }
```
From the name, it looks like a nvim-specific function which should not be called in Vim. 

## Environment

- neovim/vim version (`nvim --version` or `vim --version`):
```
VIM - Vi IMproved 8.2 (2019 Dec 12, compiled Apr 26 2020 11:23:21)
Included patches: 1-510                                                                        
Compiled by Arch Linux                                                                         
Huge version with GTK3 GUI.  Features included (+) or not (-):    
```
- This plugin version (`git rev-parse --short HEAD`): `18434f2`
- This plugin's binary version (`bin/languageclient --version`): `languageclient 0.1.157`
- Minimal vimrc content (A minimal vimrc is the smallest vimrc that could
  reproduce the issue. Refer to an example [here][min-vimrc.vim]): Not needed
- Language server link and version: rust-analyzer, but I don't think it's server-specific.

[min-vimrc.vim]: https://github.com/autozimu/LanguageClient-neovim/blob/next/min-vimrc.vim

## To Reproduce

Steps to reproduce the behavior:

I'm sure you will manage :) Let me know if not.


Discussion / hints:
I see where the issue is. The correct thing to do would be to delegate the vim/neovim function call to a function of our own where we clear the previous highlights (if any) and set the new ones. The setting part is `matchaddpos` in vim and `nvim_buf_add_highlight` in neovim. The clearing is the one I don't know in vim, I'll investigate on that, but if anyone knows the answer to it please ping me.
(Sorry clicked wrong button)
Any update on the issue ?? Looking at the vim help - the clearing of 'matchaddpos' is done with 'clearmatches' or with 'matchdelete(id)' for a single, specific match (id is obtained by 'getmatches').
I have something in the works, but the highlight code is spread all over the place so it might take a while for me to finish this and validate it's working correctly. If someone wants to give it a go I'm happy to try and help.
I'm pretty new to the whole LSP thing (I still can't configure the servers to work as intended with the LanguageClient on vim 8.2) so I'd probably be of even less help :( 

Fix patch:
diff --git a/autoload/LanguageClient.vim b/autoload/LanguageClient.vim
index 2225ab49b..82a427611 100644
--- a/autoload/LanguageClient.vim
+++ b/autoload/LanguageClient.vim
@@ -290,6 +290,11 @@ function! s:MatchDelete(ids) abort
 endfunction
 
 function! s:ApplySemanticHighlights(bufnr, ns_id, clears, highlights) abort
+    " TODO: implement this for vim8
+    if !has('nvim')
+      return
+    endif
+
     for clear in a:clears
         call nvim_buf_clear_namespace(a:bufnr, a:ns_id, clear.line_start, clear.line_end)
     endfor
@@ -300,10 +305,45 @@ function! s:ApplySemanticHighlights(bufnr, ns_id, clears, highlights) abort
 endfunction
 
 " Batch version of nvim_buf_add_highlight
-function! s:AddHighlights(source, highlights) abort
+function! s:AddHighlights(namespace_id, highlights) abort
+  if has('nvim')
+    for hl in a:highlights
+        call nvim_buf_add_highlight(0, a:namespace_id, hl.group, hl.line, hl.character_start, hl.character_end)
+    endfor
+  else
+    let match_ids = []
     for hl in a:highlights
-        call nvim_buf_add_highlight(0, a:source, hl.group, hl.line, hl.character_start, hl.character_end)
+      let match_id = matchaddpos(hl.group, [[hl.line + 1, hl.character_start + 1, hl.character_end - hl.character_start]])
+      let match_ids = add(match_ids, match_id)
+    endfor
+
+    call setbufvar(bufname(), 'document_highlight_match_ids', match_ids)
+  endif
+endfunction
+
+function! s:SetHighlights(highlights) abort
+  call s:ClearHighlights()
+  if has('nvim')
+    let s:namespace_id = nvim_create_namespace('__LCN_DOCUMENT_HIGHLIGHTS__')
+    call s:AddHighlights(s:namespace_id, a:highlights)
+  else
+    call s:AddHighlights(0, a:highlights)
+  endif
+endfunction
+
+function! s:ClearHighlights() abort
+  if has('nvim')
+    let s:namespace_id = nvim_create_namespace('__LCN_DOCUMENT_HIGHLIGHTS__')
+    call nvim_buf_clear_namespace(0, s:namespace_id, 0, -1)
+  else
+    let match_ids = get(b:, 'document_highlight_match_ids', [])
+    for mid in match_ids
+      " call inside a try/catch to avoid error for manually cleared matches
+      try | call matchdelete(mid) | catch
+      endtry
     endfor
+    call setbufvar(bufname(), 'document_highlight_match_ids', [])
+  endif
 endfunction
 
 " Get an variable value.
diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs
index 69ac7edf7..64aa041ed 100644
--- a/src/language_server_protocol.rs
+++ b/src/language_server_protocol.rs
@@ -1,7 +1,7 @@
 use crate::language_client::LanguageClient;
 use crate::sign::Sign;
 use crate::vim::{try_get, Mode};
-use crate::{extensions::java, viewport::Viewport};
+use crate::{extensions::java, viewport::Viewport, vim::Highlight};
 use crate::{
     rpcclient::RpcClient,
     types::*,
@@ -508,53 +508,7 @@ impl LanguageClient {
                 })
                 .collect::<Result<Vec<_>>>()?;
 
-            let buffer = self.vim()?.get_bufnr(&filename, params)?;
-
-            // The following code needs to be inside the critical section as a whole to update
-            // everything correctly and not leave hanging highlights.
-            self.update(|state| {
-                let source = if let Some(hs) = state.document_highlight_source {
-                    if hs.buffer == buffer {
-                        // If we want to highlight in the same buffer as last time, we can reuse
-                        // the previous source.
-                        Some(hs.source)
-                    } else {
-                        // Clear the highlight in the previous buffer.
-                        state.vim.rpcclient.notify(
-                            "nvim_buf_clear_highlight",
-                            json!([hs.buffer, hs.source, 0, -1]),
-                        )?;
-
-                        None
-                    }
-                } else {
-                    None
-                };
-
-                let source = match source {
-                    Some(source) => source,
-                    None => {
-                        // Create a new source.
-                        let source = state.vim.rpcclient.call(
-                            "nvim_buf_add_highlight",
-                            json!([buffer, 0, "Error", 1, 1, 1]),
-                        )?;
-                        state.document_highlight_source = Some(HighlightSource { buffer, source });
-                        source
-                    }
-                };
-
-                state
-                    .vim
-                    .rpcclient
-                    .notify("nvim_buf_clear_highlight", json!([buffer, source, 0, -1]))?;
-                state
-                    .vim
-                    .rpcclient
-                    .notify("s:AddHighlights", json!([source, highlights]))?;
-
-                Ok(())
-            })?;
+            self.vim()?.set_highlights(&highlights)?;
         }
 
         Ok(result)
@@ -562,20 +516,7 @@ impl LanguageClient {
 
     #[tracing::instrument(level = "info", skip(self))]
     pub fn clear_document_highlight(&self, _params: &Value) -> Result<()> {
-        // The following code needs to be inside the critical section as a whole to update
-        // everything correctly and not leave hanging highlights.
-        self.update(|state| {
-            if let Some(HighlightSource { buffer, source }) = state.document_highlight_source.take()
-            {
-                state
-                    .vim
-                    .rpcclient
-                    .notify("nvim_buf_clear_highlight", json!([buffer, source, 0, -1]))?;
-            }
-            Ok(())
-        })?;
-
-        Ok(())
+        self.vim()?.clear_highlights()
     }
 
     #[tracing::instrument(level = "info", skip(self))]
@@ -3139,41 +3080,7 @@ impl LanguageClient {
                 .collect())
         })?;
 
-        if Some(highlights.clone())
-            != self.get(|state| state.highlights_placed.get(&filename).cloned())?
-            && self.get(|state| state.is_nvim)?
-        {
-            let source = if let Some(source) = self.get(|state| state.highlight_source)? {
-                source
-            } else {
-                let source = self
-                    .vim()?
-                    .rpcclient
-                    .call("nvim_buf_add_highlight", json!([0, 0, "Error", 1, 1, 1]))?;
-                self.update(|state| {
-                    state.highlight_source = Some(source);
-                    Ok(())
-                })?;
-                source
-            };
-
-            self.update(|state| {
-                state
-                    .highlights_placed
-                    .insert(filename.clone(), highlights.clone());
-                Ok(())
-            })?;
-
-            self.vim()?.rpcclient.notify(
-                "nvim_buf_clear_highlight",
-                json!([0, source, current_viewport.start, current_viewport.end]),
-            )?;
-
-            self.vim()?
-                .rpcclient
-                .notify("s:AddHighlights", json!([source, highlights]))?;
-        }
-
+        self.vim()?.set_highlights(&highlights)?;
         self.draw_virtual_texts(&params)?;
 
         Ok(())
diff --git a/src/types.rs b/src/types.rs
index b31a949e5..b07b80842 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -5,7 +5,7 @@ use crate::{
     vim::Vim,
     watcher::FSWatch,
 };
-use crate::{logger::Logger, viewport::Viewport};
+use crate::{logger::Logger, viewport::Viewport, vim::Highlight};
 use anyhow::{anyhow, Result};
 use jsonrpc_core::Params;
 use log::*;
@@ -119,12 +119,6 @@ pub enum Call {
     Notification(LanguageId, jsonrpc_core::Notification),
 }
 
-#[derive(Clone, Copy, Serialize)]
-pub struct HighlightSource {
-    pub buffer: Bufnr,
-    pub source: u64,
-}
-
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub enum UseVirtualText {
     Diagnostics,
@@ -169,7 +163,6 @@ pub struct State {
     pub highlights_placed: HashMap<String, Vec<Highlight>>,
     // TODO: make file specific.
     pub highlight_match_ids: Vec<u32>,
-    pub document_highlight_source: Option<HighlightSource>,
     pub user_handlers: HashMap<String, String>,
     #[serde(skip_serializing)]
     pub watchers: HashMap<String, FSWatch>,
@@ -259,7 +252,6 @@ impl State {
             highlights: HashMap::new(),
             highlights_placed: HashMap::new(),
             highlight_match_ids: Vec::new(),
-            document_highlight_source: None,
             user_handlers: HashMap::new(),
             watchers: HashMap::new(),
             watcher_rxs: HashMap::new(),
@@ -506,22 +498,6 @@ pub struct TextDocumentSemanticHighlightState {
     pub highlights: Option<Vec<Highlight>>,
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize)]
-pub struct Highlight {
-    pub line: u64,
-    pub character_start: u64,
-    pub character_end: u64,
-    pub group: String,
-    pub text: String,
-}
-
-impl PartialEq for Highlight {
-    fn eq(&self, other: &Self) -> bool {
-        // Quick check whether highlight should be updated.
-        self.text == other.text && self.group == other.group
-    }
-}
-
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ClearNamespace {
     pub line_start: u64,
diff --git a/src/vim.rs b/src/vim.rs
index 9bf0cd3b1..b47b3af13 100644
--- a/src/vim.rs
+++ b/src/vim.rs
@@ -1,5 +1,5 @@
-use crate::rpcclient::RpcClient;
 use crate::{
+    rpcclient::RpcClient,
     sign::Sign,
     types::{Bufnr, QuickfixEntry, VimExp, VirtualText},
     utils::Canonicalize,
@@ -23,6 +23,21 @@ pub fn try_get<'a, R: Deserialize<'a>>(key: &str, params: &'a Value) -> Result<O
     }
 }
 
+#[derive(Clone, Copy, Serialize)]
+pub struct HighlightSource {
+    pub buffer: Bufnr,
+    pub source: u64,
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct Highlight {
+    pub line: u64,
+    pub character_start: u64,
+    pub character_end: u64,
+    pub group: String,
+    pub text: String,
+}
+
 #[derive(PartialEq)]
 pub enum Mode {
     Normal,
@@ -223,6 +238,21 @@ impl Vim {
         Ok(())
     }
 
+    /// clears all highlights in the current buffer.
+    pub fn clear_highlights(&self) -> Result<()> {
+        self.rpcclient.notify("s:ClearHighlights", json!([]))
+    }
+
+    /// replaces the highlights of the current document with the passed highlights.
+    pub fn set_highlights(&self, highlights: &[Highlight]) -> Result<()> {
+        if highlights.is_empty() {
+            return Ok(());
+        }
+
+        self.rpcclient
+            .notify("s:SetHighlights", json!([highlights]))
+    }
+
     pub fn create_namespace(&self, name: &str) -> Result<i64> {
         self.rpcclient.call("nvim_create_namespace", [name])
     }


Test patch: