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

Issue:
syntax highlighting (errors and warnings) doesn't disappear after fixing the errors
- 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. **Yes. I do run `install.sh` everytime I upgrade.**
- (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 resolving a syntax error in a file, the error red highlighting is still applied on the line where the bug was and it doesn't go away. I have to restart Vim in order for the highlighting to be reset.

Here is a minimal example:

https://github.com/sim590/language-client-highlight-bug

[lchb]: https://github.com/sim590/language-client-highlight-bug

## Environment

- neovim/vim version (`nvim --version` or `vim --version`):

      VIM - Vi IMproved 8.2 (2019 Dec 12, compilé Feb 09 2021 23:51:55)

- This plugin version (`git rev-parse --short HEAD`): a42594c
- This plugin's binary version (`bin/languageclient --version`): languageclient 0.1.161
- Minimal vimrc content (A minimal vimrc is the smallest vimrc that could
  reproduce the issue. Refer to an example [here][min-vimrc.vim]): It is part of the [minimal example repository][lchb].
- Language server link and version: https://github.com/haskell/haskell-language-server

[min-vimrc.vim]: https://github.com/autozimu/LanguageClient-neovim/blob/next/min-vimrc.vim

## To Reproduce

Steps to reproduce the behavior:

1. Fetch the minimal example and enter the repository:

       $ git clone https://github.com/sim590/language-client-highlight-bug
       $ cd language-client-highlight-bug

2. Open the `Main.hs` file.
3. Execute the vim normal script: `7Gf=wC undefined^[` where `^[` is the escape key.

## Current behavior

The word `undefined` is highlighted in red even though it is correct syntax.

LanguageClient highlights this region when `undefined` is being written, so when it sees for instance `undef`, it correctly highlights the word in red, but as soon as the syntax is correct, i.e. that you write the final `d`, you can see that the error symbol in the left column is gone, but a subword like `undefine` is highlighted in red (but not the letter `d`).

## Expected behavior

The highlighting should disappear when an error/warning is fixed.

## Screenshots

![vim](https://user-images.githubusercontent.com/4147254/112744833-65fba880-8f71-11eb-88aa-4e65bc8c2d64.png)

## Additional context

It seems like fixing the warning just above (the line with `my_func`) does go around the issue. However, you understand that it is not always possible to do so. Sometimes, it doesn't help either and you have to restart Vim in order to retrieve a usable state.



Discussion / hints:
This issue is related to #1147. However, now, there is a contradictory behaviour that lets us deduce that the problem is not on the server side: the column error sign does go away, but not the highlighting. Therefore, it's clear that LanguageClient has received the information that the error is not on the line anymore, but it somehow has trouble to remove highlighting properly.
I had some issues setting up haskell so I ended up testing this with Go instead. I'm not sure if I understood correctly, but if I did what you are saying is that after correcting an error that ends up manifesting as highlighted text in vim you end up with text still highlighted after fixing it? If that's the case, do you see this happening in other languages? I've tried reproducing this in the bases that my understanding of this issue is correct, but I don't see this happening in vim with Go and you minimal vimrc (+ a few minor tweaks to set up gopls instead haskell ls).

Again, if my understand is correct here I should still be seeing errors highlighted in this gif after fixing the error, am I correct?

![Peek 2021-03-28 22-01](https://user-images.githubusercontent.com/4250565/112767978-8bd88a00-9011-11eb-816f-c6dd5bbae395.gif)

Hi @martskins.

Thanks for your response.

I think that the issue reveals itself when there's a warning above in the file. Notice in my example, there's a warning above about `my_func`.

Could you trigger a warning in your Go file just above and try again to see if the problem occurs? It seems tricky to trigger though. May be warnings are not sufficient, but in my case it is.
> I had some issues setting up haskell

In order to test with Haskell, you could do the following:

1. Install `ghcup`.
2. Run `ghcup install ghc 8.6.5` (that is the version I use, but I doubt the version is important).
3. Then, set the version: `ghcup set ghc 8.6.5`. This will create relevant symlinks.
4. Run `ghcup install hls`. This is the haskell language server.
5. Add `/home/USER/.ghcup/bin` in your `$PATH`.

All of those packages are installed locally under `~/.ghcup/`, so no `sudo` required.

Then, you should be good to go (I mean good to _haskell_ :wink: ). I think.



Gopls doesn't seem to report warnings if there are compiler errors, but I could set up the scenario with the warning you mentioned in rust, and I wasn't able to reproduce. Any chance you could set your log level to `DEBUG` and share the contents of your log file after triggering this issue? 
Alright, I was able to reproduce following your steps now, so no need for the log output. It seems like it only happens in vim, not happening in neovim, so definitely something weird our side. I'll see if I can find the culprit.
Ok so I got some interesting insights on this, not sure how to fix it yet or if it's actually something that needs to be fixed upstream, but it seems like the server is sending the diagnostics notification twice, and it overlaps on diagnostics as well.

So for example, in your command `7Gf=wC undefined`, each key you press while typing `undefined` results in two publish diagnostics notifications being sent from the server. I suspect there is some race condition happening somewhere here, but I'm failing to understand why we get two notifications instead of a single one. Here's an example of what those notifications look like:


```
{
  "diagnostics":[
    {"code":"-Wdeferred-out-of-scope-variables","message":"Variable not in scope: un :: Int","range":{"end":{"character":10,"line":6},"start":{"character":8,"line":6}},"severity":1,"source":"typecheck"},
  ],
  "uri":"file:///home/martin/dev/language-client-highlight-bug/app/Main.hs",
  "version":3,
}

{
  "diagnostics":[
    {"code":"Use camelCase","message":"Use camelCase\\nFound:\\n  my_func :: Int -> Int\\nWhy not:\\n  myFunc :: Int -> Int\\n","range":{"end":{"character":21,"line":2},"start":{"character":0,"line":2}},"severity":3,"source":"hlint"},
    {"code":"Use camelCase","message":"Use camelCase\\nFound:\\n  my_func = ...\\nWhy not:\\n  myFunc = ...\\n","range":{"end":{"character":19,"line":3},"start":{"character":0,"line":3}},"severity":3,"source":"hlint"},
    {"code":"-Wdeferred-out-of-scope-variables","message":"Variable not in scope: un :: Int","range":{"end":{"character":10,"line":6},"start":{"character":8,"line":6}},"severity":1,"source":"typecheck"},
  ],
  "uri":"file:///home/martin/dev/language-client-highlight-bug/app/Main.hs",
  "version":3,
}
```

It's always in the same pattern, a notification with a single diagnostics for the error and then another notification with all 3 diagnostics, which also includes the error on the first notification.
If I build my understanding on what you just said, the server sends diagnostics in the form of notifications and possibly multiple times (twice or more). Those notifications serve to find out what errors there are in the file. If I understand correctly a notification should contain all of the errors that the server knows about, right?

It seems to me that the server may be writing on some error queue while reading the file for errors. Another thread would be reading that queue and sending it to the client at the same time. In the scenario you just listed, the queue could have contained only the first message at first and then it contained the others once the queue was updated by syntax analyser thread? Could it be the sorting of those messages that is not totally well implemented on the client side?

Is the field `version` (like `"version":3` in your example) a sequence number helping for sorting messages? If so, then my interpretation above would be false since in your example, both have `"version":3`? Is it rather the version of the protocol? But, in any case, wouldn't it be as simple as taking the last notification as the last true state of the errors in the file?

Also, why would the signcolumn not be affected by this race condition while the highlighting would? It sounds to me that there's an inconsistency in the client, no? I would look at how the sign column is removed and try to understand why the same would not happen for the highlighting.

I have not read the code though. I'm thinking out loud to give ideas...
> if I understand correctly a notification should contain all of the errors that the server knows about

correct, with the minor observation that errors here actually should be diagnostics, as they are not necessarily errors, could be warnings or hints

> Could it be the sorting of those messages that is not totally well implemented on the client side?

The client doesn't do (and shouldn't do) any kind of sorting of the messages, notifications are processed as soon as they are received. 

> Is the field version (like "version":3 in your example) a sequence number helping for sorting messages? 

Nope. The version is the document (buffer, file) version. That is, every time you change something on a buffer a new version is "created". Typically you want to ignore messages that are targeted at version prior to the one you currently have, so if you receive messages for both version N and N+1 of a file and you are certain that you have version N+1 in vim then you probably want to ignore the messages for the version N.

> Also, why would the signcolumn not be affected by this race condition while the highlighting would?

Not sure, they are differente pieces of code though. But yeah, I'm leaning towards the idea that there is something we're doing wrong. Having said that though, I'm still not sure why haskell language server sends those two notifications like that, with such a short time difference and with that consistency in behaviour. Maybe we are causing that somehow too, not sure just yet.
Also, fwiw, adding a mutex to lock out the diagnostics processing bit solves this, but I want to avoid going that route because it seems to me like there should be a better solution, but not quite sure what that is yet.
I'm gonna borrow your repo there and open an issue on the haskell-language-server repo to see if this is something that needs their attention or if it's intentional.

Fix patch:
diff --git a/src/language_client.rs b/src/language_client.rs
index 16f22943..0204c7cf 100644
--- a/src/language_client.rs
+++ b/src/language_client.rs
@@ -527,75 +527,76 @@ impl LanguageClient {
         })?;
 
         if !self.get_config(|c| c.is_nvim)? {
-            // Clear old highlights.
-            let ids = self.get_state(|state| state.highlight_match_ids.clone())?;
-            self.vim()?
-                .rpcclient
-                .notify("s:MatchDelete", json!([ids]))?;
-
-            // Group diagnostics by severity so we can highlight them
-            // in a single call.
-            let mut match_groups: HashMap<_, Vec<_>> = HashMap::new();
-
-            for dn in diagnostics {
-                let severity = dn.severity.unwrap_or(DiagnosticSeverity::Hint).to_int()?;
-                match_groups
-                    .entry(severity)
-                    .or_insert_with(Vec::new)
-                    .push(dn);
-            }
+            // this needs to be in this locked block so that notifications that arrive too close to
+            // each other do not have the chance to cause any race conditions.
+            self.update_state(|state| {
+                // Clear old highlights.
+                let ids = state.highlight_match_ids.clone();
+                state.vim.rpcclient.notify("s:MatchDelete", json!([ids]))?;
+
+                // Group diagnostics by severity so we can highlight them
+                // in a single call.
+                let mut match_groups: HashMap<_, Vec<_>> = HashMap::new();
+
+                for dn in diagnostics {
+                    let severity = dn.severity.unwrap_or(DiagnosticSeverity::Hint).to_int()?;
+                    match_groups
+                        .entry(severity)
+                        .or_insert_with(Vec::new)
+                        .push(dn);
+                }
 
-            let mut new_match_ids = Vec::new();
+                let mut new_match_ids = Vec::new();
 
-            for (severity, dns) in match_groups {
-                let hl_group = diagnostics_display
-                    .get(&severity)
-                    .ok_or_else(|| anyhow!("Failed to get display"))?
-                    .texthl
-                    .clone();
-                let ranges: Vec<Vec<_>> = dns
-                    .iter()
-                    .flat_map(|dn| {
-                        if dn.range.start.line == dn.range.end.line {
-                            let length = dn.range.end.character - dn.range.start.character;
-                            // Vim line numbers are 1 off
-                            // `matchaddpos` expects an array of [line, col, length]
-                            // for each match.
-                            vec![vec![
-                                dn.range.start.line + 1,
-                                dn.range.start.character + 1,
-                                length,
-                            ]]
-                        } else {
-                            let mut middle_lines: Vec<_> = (dn.range.start.line + 1
-                                ..dn.range.end.line)
-                                .map(|l| vec![l + 1])
-                                .collect();
-                            let start_line = vec![
-                                dn.range.start.line + 1,
-                                dn.range.start.character + 1,
-                                999_999, //Clear to the end of the line
-                            ];
-                            let end_line =
-                                vec![dn.range.end.line + 1, 1, dn.range.end.character + 1];
-                            middle_lines.push(start_line);
-                            // For a multi-ringe range ending at the exact start of the last line,
-                            // don't highlight the first character of the last line.
-                            if dn.range.end.character > 0 {
-                                middle_lines.push(end_line);
+                for (severity, dns) in match_groups {
+                    let hl_group = diagnostics_display
+                        .get(&severity)
+                        .ok_or_else(|| anyhow!("Failed to get display"))?
+                        .texthl
+                        .clone();
+                    let ranges: Vec<Vec<_>> = dns
+                        .iter()
+                        .flat_map(|dn| {
+                            if dn.range.start.line == dn.range.end.line {
+                                let length = dn.range.end.character - dn.range.start.character;
+                                // Vim line numbers are 1 off
+                                // `matchaddpos` expects an array of [line, col, length]
+                                // for each match.
+                                vec![vec![
+                                    dn.range.start.line + 1,
+                                    dn.range.start.character + 1,
+                                    length,
+                                ]]
+                            } else {
+                                let mut middle_lines: Vec<_> = (dn.range.start.line + 1
+                                    ..dn.range.end.line)
+                                    .map(|l| vec![l + 1])
+                                    .collect();
+                                let start_line = vec![
+                                    dn.range.start.line + 1,
+                                    dn.range.start.character + 1,
+                                    999_999, //Clear to the end of the line
+                                ];
+                                let end_line =
+                                    vec![dn.range.end.line + 1, 1, dn.range.end.character + 1];
+                                middle_lines.push(start_line);
+                                // For a multi-ringe range ending at the exact start of the last line,
+                                // don't highlight the first character of the last line.
+                                if dn.range.end.character > 0 {
+                                    middle_lines.push(end_line);
+                                }
+                                middle_lines
                             }
-                            middle_lines
-                        }
-                    })
-                    .collect();
+                        })
+                        .collect();
+
+                    let match_id = state
+                        .vim
+                        .rpcclient
+                        .call("matchaddpos", json!([hl_group, ranges]))?;
+                    new_match_ids.push(match_id);
+                }
 
-                let match_id = self
-                    .vim()?
-                    .rpcclient
-                    .call("matchaddpos", json!([hl_group, ranges]))?;
-                new_match_ids.push(match_id);
-            }
-            self.update_state(|state| {
                 state.highlight_match_ids = new_match_ids;
                 Ok(())
             })?;


Test patch: