search

autozimu / LanguageClient-neovim

Language Server Protocol (LSP) support for vim and neovim.
MIT License
3.55k stars 272 forks source link

syntax highlighting (errors and warnings) doesn't disappear after fixing the errors #1210

Open sim590 opened 3 years ago

sim590 commented 3 years ago

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

Environment

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

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.

sim590 commented 3 years ago

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.

martskins commented 3 years ago

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

sim590 commented 3 years ago

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.

sim590 commented 3 years ago

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.

martskins commented 3 years ago

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?

martskins commented 3 years ago

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.

martskins commented 3 years ago

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.

sim590 commented 3 years ago

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...

martskins commented 3 years ago

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.

martskins commented 3 years ago

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.

martskins commented 3 years ago

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.

martskins commented 3 years ago

Actually, I guess locking specifically that bit out should be fine, so I opened #1211. If you have Rust installed and want to give that a try and see whether you can still reproduce that would be great. I'm no longer able to do reproduce the issue with that branch now, but extra eyes are always welcome.

Also, if everything works out fine I'll be merging this to the dev branch, so hopefully you'll have a working version you can use sooinsh, a proper release cut will take a few more days but I've been meaning to cut a release for quite a while now so I'll probably do once we have tested this thoroughly.

sim590 commented 3 years ago

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.

That is exactly what I had in mind when I was talking about a sequence number. Thanks for clarifying!

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.

Well, that's what you do when you consider the sequence number version, no? The number actually help you sort out which diagnostics to consider as the last true diagnostic.

martskins commented 3 years ago

Well, that's what you do when you consider the sequence number version, no? The number actually help you sort out which diagnostics to consider as the last true diagnostic.

Well, not exactly, as the versions are not necessarily sequential and you could get more than one notification for the same version (as is the case here). You could say that you can filter out notifications/messages that you are not interested based on it though, that I believe would be a fair statement, but it's not necessarily the same as sorting, especially because there is no caching of the messages whatsoever, the client process what it receives in the moment it receives it. But I might be getting to a pedantic argument here :grin:

sim590 commented 3 years ago

Hi @martskins,

I just tried commit a734a05 and I still reproduce the same behaviour with the minimal example I gave, unfortunately.

martskins commented 3 years ago

@sim590 hm, that is weird. Sorry for the maybe dumb question but you did compile the code from source and changed your Plug line (or whatever you are using) to use your local copy of this instead right?

sim590 commented 3 years ago

I did replace next by dev in my Plug line:

Plug 'autozimu/LanguageClient-neovim', {
    \ 'branch': 'dev',
    \ 'do': './install.sh'
    \ }

and did run :source $MYVIMRC | PlugInstall! LanguageClient-neovim in order to update the repository. It should have rerun install.hs in principle. I'm guessing that is not sufficient? I guess I have to run make?

sim590 commented 3 years ago

OK. After running make, it did work!

martskins commented 3 years ago

Ah great! Was in the process of explaining you did need to run make release, good to know it worked :+1:

sim590 commented 3 years ago

So I played with it a bit and I can say that the minimal example is fixed, but there are still issues. I can't say how to reproduce them just yet. But here's a screen capture:

still

martskins commented 3 years ago

Hm, might need to get more of the logic inside the lock then. I'll ping you when I have a branch with that so you can test it out.

zodman commented 3 years ago

Damn, it happened to me, I had to close the file and open it again for the highlight to still stay ...

when I see it was the vim-polyglot causing it... I had de-installed and the problem goes away ...

moinessim commented 3 years ago

Hi there, same bug here but with dotnet + ionide-vim, fsharp project.

No need to restart vim, though. Just splitting the buffer refreshes the highlight...

davidhalter commented 2 years ago

I'm having the same issue with multiple different language servers. I'm still very unsure what it's caused by (could also be a VIM bug).

You can remove these artifacts by running :call clearmatches() to continue working in the same buffer.

davidhalter commented 1 year ago

I now clear the matches when writing the file, which helps quite a bit.

autocmd BufWritePre *rs call clearmatches()