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

Issue:
Signs are not updating properly
I just noticed that signs are not always properly updated. The logic we're using to calculate the diff is incorrect in that it compares the previous signs with the new ones, but the previous signs were applied to an older version of the buffer, so there are cases were that diff is invalid.

It's hard to put in words, but essentially the line numbers of the signs don't match when comparing different versions of the documents, so diffing them could be inaccurate, which results in us removing signs incorrectly sometimes, or other times placing a sign more than once in the same line, which eventually leads to stale signs.

This gif illustrates shows the issue and you'll also see at the end of the gif that the placed signs list shows more than one sign placed on the same line:

http://g.recordit.co/DNSNfCBx5A.gif

I think to solve this we shouldn't really be applying diffs of signs but rather replacing all the signs. One thing we could do to make this a little cheaper is to only place the signs that overlap the current viewport and update on each cursor move. This would result in unnecessary updates too, but if we also store the previous viewport and compare the current one with the previous one I think we should be able to decide whether to update or not more accurately (if the viewport is the same after a cursor move, don't update signs).

I'm hoping to open a PR to solve this this weekend or early next week, but if someone else wants to give it a go I'm more than happy to help.


Discussion / hints:

Fix patch:
diff --git a/autoload/LanguageClient.vim b/autoload/LanguageClient.vim
index 656f1639d..2225ab49b 100644
--- a/autoload/LanguageClient.vim
+++ b/autoload/LanguageClient.vim
@@ -177,15 +177,33 @@ function! s:set_virtual_texts(buf_id, ns_id, line_start, line_end, virtual_texts
     endfor
 endfunction
 
-function! s:set_signs(file, signs_to_add, signs_to_delete) abort
-    " TODO: Optimize to update sign instead of add + remove sign.
-    for l:sign in a:signs_to_add
-        let l:line = l:sign['line'] + 1
-        execute ':sign place ' . l:sign['id'] . ' line=' . l:line . ' name=' . l:sign['name'] . ' file=' . a:file
-    endfor
-    for l:sign in a:signs_to_delete
-        execute ':sign unplace ' . l:sign['id']
-    endfor
+function! s:place_sign(id, name, file, line) abort
+  if !exists('*sign_place')
+    execute 'sign place id=' . a:id . ' name=' . a:name . ' file=' . a:file . ' line=' . a:line
+  endif
+
+  call sign_place(0, 'LanguageClientNeovim', a:name, a:file, { 'lnum': a:line })
+endfunction
+
+" clears all signs on the buffer with the given name
+function! s:clear_buffer_signs(file) abort
+  if !exists('*sign_unplace')
+    execute 'sign unplace * group=LanguageClientNeovim buffer=' . a:file
+  else
+    call sign_unplace('LanguageClientNeovim', { 'buffer': a:file })
+  endif
+endfunction
+
+" replaces the signs on a file with the ones passed as an argument
+function! s:set_signs(file, signs) abort
+  call s:clear_buffer_signs(a:file)
+
+  for l:sign in a:signs
+    let l:line = l:sign['line'] + 1
+    let l:name = l:sign['name']
+    let l:id = l:sign['id']
+    call s:place_sign(l:id, l:name, a:file, l:line)
+  endfor
 endfunction
 
 " Execute serious of ex commands.
diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs
index 08df852f4..2a7de2539 100644
--- a/src/language_server_protocol.rs
+++ b/src/language_server_protocol.rs
@@ -1,6 +1,7 @@
-use crate::extensions::java;
 use crate::language_client::LanguageClient;
+use crate::sign::Sign;
 use crate::vim::{try_get, Mode};
+use crate::{extensions::java, viewport::Viewport};
 use crate::{
     rpcclient::RpcClient,
     types::*,
@@ -1082,7 +1083,7 @@ impl LanguageClient {
             }
             self.process_diagnostics(&f, &[])?;
         }
-        self.handle_cursor_moved(&Value::Null)?;
+        self.handle_cursor_moved(&Value::Null, true)?;
 
         self.update(|state| {
             state.clients.remove(&Some(language_id.into()));
@@ -2407,7 +2408,7 @@ impl LanguageClient {
         });
 
         self.process_diagnostics(&current_filename, &diagnostics)?;
-        self.handle_cursor_moved(&Value::Null)?;
+        self.handle_cursor_moved(&Value::Null, true)?;
         self.vim()?
             .rpcclient
             .notify("s:ExecuteAutocmd", "LanguageClientDiagnosticsChanged")?;
@@ -2973,7 +2974,7 @@ impl LanguageClient {
                 self.get(|state| state.diagnostics.get(&filename).cloned())?
             {
                 self.process_diagnostics(&filename, &diagnostics)?;
-                self.handle_cursor_moved(params)?;
+                self.handle_cursor_moved(params, true)?;
             }
         } else {
             let auto_start: u8 = self
@@ -3036,7 +3037,6 @@ impl LanguageClient {
             state.text_documents.retain(|f, _| f != &filename);
             state.diagnostics.retain(|f, _| f != &filename);
             state.line_diagnostics.retain(|fl, _| fl.0 != *filename);
-            state.signs.retain(|f, _| f != &filename);
             Ok(())
         })?;
         self.text_document_did_close(params)?;
@@ -3044,7 +3044,32 @@ impl LanguageClient {
     }
 
     #[tracing::instrument(level = "info", skip(self))]
-    pub fn handle_cursor_moved(&self, params: &Value) -> Result<()> {
+    fn get_signs_to_display(&self, filename: &str, viewport: &Viewport) -> Result<Vec<Sign>> {
+        let signs: Vec<_> = self.get(|state| {
+            let diagnostics = state.diagnostics.get(filename).cloned().unwrap_or_default();
+            let mut diagnostics = diagnostics
+                .iter()
+                .filter(|diag| viewport.overlaps(diag.range))
+                .sorted_by_key(|diag| {
+                    (
+                        diag.range.start.line,
+                        diag.severity.unwrap_or(DiagnosticSeverity::Hint),
+                    )
+                })
+                .collect_vec();
+            diagnostics.dedup_by_key(|diag| diag.range.start.line);
+            diagnostics
+                .into_iter()
+                .take(state.diagnostics_signs_max.unwrap_or(std::usize::MAX))
+                .map(Into::into)
+                .collect()
+        })?;
+
+        Ok(signs)
+    }
+
+    #[tracing::instrument(level = "info", skip(self))]
+    pub fn handle_cursor_moved(&self, params: &Value, force_redraw: bool) -> Result<()> {
         let filename = self.vim()?.get_filename(params)?;
         let language_id = self.vim()?.get_language_id(&filename, params)?;
         let line = self.vim()?.get_position(params)?.line;
@@ -3080,67 +3105,23 @@ impl LanguageClient {
             })?;
         }
 
-        let viewport = self.vim()?.get_viewport(params)?;
-
-        // use the most severe diagnostic of each line as the sign
-        let signs_next: Vec<_> = self.update(|state| {
-            let mut diagnostics = state
-                .diagnostics
-                .entry(filename.clone())
-                .or_default()
-                .iter()
-                .filter(|diag| viewport.overlaps(diag.range))
-                .sorted_by_key(|diag| {
-                    (
-                        diag.range.start.line,
-                        diag.severity.unwrap_or(DiagnosticSeverity::Hint),
-                    )
-                })
-                .collect_vec();
-            diagnostics.dedup_by_key(|diag| diag.range.start.line);
-            Ok(diagnostics
-                .into_iter()
-                .take(state.diagnostics_signs_max.unwrap_or(std::usize::MAX))
-                .map(Into::into)
-                .collect())
-        })?;
-        self.update(|state| {
-            let signs_prev: Vec<_> = state
-                .signs
-                .entry(filename.clone())
-                .or_default()
-                .iter()
-                .map(|(_, sign)| sign.clone())
-                .collect();
-            let mut signs_to_add = vec![];
-            let mut signs_to_delete = vec![];
-            let diffs = diff::slice(&signs_next, &signs_prev);
-            for diff in diffs {
-                match diff {
-                    diff::Result::Left(s) => {
-                        signs_to_add.push(s.clone());
-                    }
-                    diff::Result::Right(s) => {
-                        signs_to_delete.push(s.clone());
-                    }
-                    _ => {}
-                }
+        let current_viewport = self.vim()?.get_viewport(params)?;
+        let previous_viewport = self.get(|state| state.viewports.get(&filename).cloned())?;
+        match previous_viewport {
+            // if the viewport hasn't changed and force_redraw is not set, we can safely exit this
+            // function early and save us some sign and virtual text redrawing.
+            Some(pv) if pv == current_viewport && !force_redraw => {
+                return Ok(());
             }
+            _ => {}
+        }
 
-            let signs = state.signs.entry(filename.clone()).or_default();
-            // signs might be deleted AND added in the same line to change severity,
-            // so deletions must be before additions
-            for sign in &signs_to_delete {
-                signs.remove(&sign.line);
-            }
-            for sign in &signs_to_add {
-                signs.insert(sign.line, sign.clone());
-            }
-            state
-                .vim
-                .set_signs(&filename, &signs_to_add, &signs_to_delete)?;
+        let signs = self.get_signs_to_display(&filename, &current_viewport)?;
+        self.update(|state| {
+            state.viewports.insert(filename.clone(), current_viewport);
             Ok(())
         })?;
+        self.vim()?.set_signs(&filename, &signs)?;
 
         let highlights: Vec<_> = self.update(|state| {
             Ok(state
@@ -3149,7 +3130,7 @@ impl LanguageClient {
                 .or_insert_with(Vec::new)
                 .iter()
                 .filter_map(|h| {
-                    if h.line < viewport.start || h.line > viewport.end {
+                    if h.line < current_viewport.start || h.line > current_viewport.end {
                         return None;
                     }
 
@@ -3185,7 +3166,7 @@ impl LanguageClient {
 
             self.vim()?.rpcclient.notify(
                 "nvim_buf_clear_highlight",
-                json!([0, source, viewport.start, viewport.end]),
+                json!([0, source, current_viewport.start, current_viewport.end]),
             )?;
 
             self.vim()?
diff --git a/src/rpchandler.rs b/src/rpchandler.rs
index c71bc081e..9f61d0692 100644
--- a/src/rpchandler.rs
+++ b/src/rpchandler.rs
@@ -191,7 +191,7 @@ impl LanguageClient {
             NOTIFICATION_HANDLE_TEXT_CHANGED => self.handle_text_changed(&params)?,
             NOTIFICATION_HANDLE_BUF_WRITE_POST => self.handle_buf_write_post(&params)?,
             NOTIFICATION_HANDLE_BUF_DELETE => self.handle_buf_delete(&params)?,
-            NOTIFICATION_HANDLE_CURSOR_MOVED => self.handle_cursor_moved(&params)?,
+            NOTIFICATION_HANDLE_CURSOR_MOVED => self.handle_cursor_moved(&params, false)?,
             NOTIFICATION_HANDLE_COMPLETE_DONE => self.handle_complete_done(&params)?,
             NOTIFICATION_FZF_SINK_LOCATION => self.fzf_sink_location(&params)?,
             NOTIFICATION_FZF_SINK_COMMAND => self.fzf_sink_command(&params)?,
diff --git a/src/types.rs b/src/types.rs
index 7cff2160e..b31a949e5 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -1,12 +1,11 @@
-use crate::logger::Logger;
 use crate::rpcclient::RpcClient;
-use crate::sign::Sign;
 use crate::{
     language_client::LanguageClient,
     utils::{code_action_kind_as_str, ToUrl},
     vim::Vim,
     watcher::FSWatch,
 };
+use crate::{logger::Logger, viewport::Viewport};
 use anyhow::{anyhow, Result};
 use jsonrpc_core::Params;
 use log::*;
@@ -22,7 +21,7 @@ use pathdiff::diff_paths;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use std::{
-    collections::{BTreeMap, HashMap},
+    collections::HashMap,
     io::{BufRead, BufReader, BufWriter, Write},
     net::TcpStream,
     path::{Path, PathBuf},
@@ -151,6 +150,7 @@ pub struct State {
     pub registrations: Vec<Registration>,
     pub roots: HashMap<String, String>,
     pub text_documents: HashMap<String, TextDocumentItem>,
+    pub viewports: HashMap<String, Viewport>,
     pub text_documents_metadata: HashMap<String, TextDocumentItemMetadata>,
     pub semantic_scopes: HashMap<String, Vec<Vec<String>>>,
     pub semantic_scope_to_hl_group_table: HashMap<String, Vec<Option<String>>>,
@@ -163,8 +163,6 @@ pub struct State {
     pub code_lens_hl_group: String,
     #[serde(skip_serializing)]
     pub line_diagnostics: HashMap<(String, u64), String>,
-    /// Active signs.
-    pub signs: HashMap<String, BTreeMap<u64, Sign>>,
     pub namespace_ids: HashMap<String, i64>,
     pub highlight_source: Option<u64>,
     pub highlights: HashMap<String, Vec<Highlight>>,
@@ -248,6 +246,7 @@ impl State {
             registrations: vec![],
             roots: HashMap::new(),
             text_documents: HashMap::new(),
+            viewports: HashMap::new(),
             text_documents_metadata: HashMap::new(),
             semantic_scopes: HashMap::new(),
             semantic_scope_to_hl_group_table: HashMap::new(),
@@ -255,7 +254,6 @@ impl State {
             code_lens: HashMap::new(),
             diagnostics: HashMap::new(),
             line_diagnostics: HashMap::new(),
-            signs: HashMap::new(),
             namespace_ids: HashMap::new(),
             highlight_source: None,
             highlights: HashMap::new(),
diff --git a/src/vim.rs b/src/vim.rs
index 1c054a126..c26170eca 100644
--- a/src/vim.rs
+++ b/src/vim.rs
@@ -241,15 +241,7 @@ impl Vim {
         )
     }
 
-    pub fn set_signs(
-        &self,
-        filename: &str,
-        signs_to_add: &[Sign],
-        signs_to_delete: &[Sign],
-    ) -> Result<i8> {
-        self.rpcclient.call(
-            "s:set_signs",
-            json!([filename, signs_to_add, signs_to_delete]),
-        )
+    pub fn set_signs(&self, filename: &str, signs: &[Sign]) -> Result<i8> {
+        self.rpcclient.call("s:set_signs", json!([filename, signs]))
     }
 }


Test patch: