hrsh7th / vim-lamp

💡Language Server Protocol client for Vim.
MIT License
32 stars 0 forks source link

The ability to reload diagnostic #23

Closed Shatur closed 4 years ago

Shatur commented 4 years ago

Sometimes diagnostics are not properly updated. It would be nice to have the ability to trigger diagnostics update manually. Is it possible to implement?

hrsh7th commented 4 years ago

It's possible but I want to fix the problem...

It is most difficult critical bug of the vim-lamp now.

hrsh7th commented 4 years ago

@Shatur95 Currently, vim-lamp has an incremental diff feature that improves performance but I doubt the reason for the problem is it... maybe.

So I introduce the flag to toggle the feature. Could you test to reproduce the problem with temporarily disable it?

hrsh7th commented 4 years ago

Could you test with let g:lamp_experimental_passthrough_diagnostics = v:true ?

When the flags turns on, you get a bit ugly behavior but it needs to investigate sorry...

Shatur commented 4 years ago

Good idea! I will try to provide minimal reproducible example and test new diagnostics feature.

hrsh7th commented 4 years ago

If you like a new feature than an old feature, please tell me it.

I don't know the best visualization for the diagnostics.

Shatur commented 4 years ago

What difference between v:true and v:false for g:lamp_experimental_passthrough_diagnostics?

Shatur commented 4 years ago

I tested with and without incremental feature. The problem still exits.

I found minimal reproducible example:

  1. Download my akd
  2. Then you will need to generate compile_commands.json:
mkdir build
cd build
cmake -D CMAKE_EXPORT_COMPILE_COMMANDS ..
cp compile_commands.json ..

(you will have compile_commands.json in project code directory. I use a plugin for automation but I decided do describe manual steps)

  1. Install and setup clangd in vim-lamp.
  2. Open keyboarddaemon.h and create the following function void test()
  3. Open keyboarddaemon.cpp and create the following definition void KeyboardDaemon() {}

Expected behavior

No errors detected.

Actual behavior

Diagnostics say that the function does not exists even after :wa.

Video In this video I created function, but diagnostics updated only after a long time. In all IDE's diagnostics updated immediately even without save. asciicast

hrsh7th commented 4 years ago

Sorry, the experimental flags are checked existence only.

Thank you for create repro steps. I try to run cmake but it failed on my mac. I will try to check on any Linux environment.

hrsh7th commented 4 years ago

To prepare the Linux environment take time So I try to detect the cause by removing updating condition for diagnostics one by one...

I was remove one condition for now on the master branch.

hrsh7th commented 4 years ago

My suspicion points are

Shatur commented 4 years ago

I try to run cmake but it failed on my mac. I will try to check on any Linux environment.

Sorry, I did now know that you use Mac. Provided program is Linux-only. Try the following minimal example (try to create void test2() function in keyboarddaemon.h and it implementation in keyboarddaemon.h): Sample.zip

I try to detect the cause by removing updating condition for diagnostics one by one...

Just tested latest master, have the same issue. But why remove? I thought that new condition should be added. The diagnostics just not updated properly after creating function definition. I think in is because of different files. If I will create definition in the same file (C++ have such possibility) - everything works.

My suspicion points are

I agree. I think that there is two problems. Currently I did not encountered the diff bug. But the repro example shows that diagnostics do not updated with different files.

Could you explain what is passthrough mode?

hrsh7th commented 4 years ago

Thank you for Sample.zip! I try to reproduce it!

The diagnostics feature is too difficult.

  1. LSP client should send text changes to the server per press key.
  2. LSP server typically computes diagnostics every text changes and send it to client.
  3. LSP client shouldn't show every received diagnostic to improve UX.

For example, vim-lamp skip or debounce diagnostics when these cases.

  1. [SKIP] The target buffer does not visible on any window.
  2. [DEBOUNCE] In insert-mode.
  3. and some conditions...

So I suspect the cause is in these conditions.

hrsh7th commented 4 years ago

Could you explain what is passthrough mode?

Passthrough mode is removing the some above conditions on diagnostics feature.

Shatur commented 4 years ago

Thanks for explanations! Yes, I tested with passthrough mode and the issue still exists. vim-lsp also have the same bug.

hrsh7th commented 4 years ago

Thank you for your investigate.

Oh... If vim-lsp has the same problem, this problem caused by the server maybe.

Anyway, I'll try to detect the cause with your example project!

Shatur commented 4 years ago

Thanks for looking into it!

hrsh7th commented 4 years ago

Unfortunately, It seems the clangd's problem. I searching to solve this on the clangd's issue tracker.

Shatur commented 4 years ago

Bad to hear :( Other IDE's that use clangd do not have this issue.

hrsh7th commented 4 years ago

Hm... I try to check with coc.nvim temporary but it has same problem...

hrsh7th commented 4 years ago

I try again with VSCode.

hrsh7th commented 4 years ago

VSCode has same issue...

Shatur commented 4 years ago

Let me check it myself :)

hrsh7th commented 4 years ago

Yes, thanks!

FYI: I used vscode-clangd.

Currently, My thoughts are

Shatur commented 4 years ago

Wow, you are right. Only QtCreator works with this correctly (without save). But VSCode updates this file immediately after "Save all" and typing any symbol. Maybe is it possible to at least achieve something similar?

Shatur commented 4 years ago

Yes, I think that my and your problems is different. If I reproduce your problem with minimal example - I will let you know.

hrsh7th commented 4 years ago

Anyway, I re-enable diagnostics and incremental diff. when you met the problem, please tell me it.

And I try to check the ccls.

Shatur commented 4 years ago

Did I understand correctly that incremental diff is now enabled by default and "Passthrough mode" is disabled by default?

hrsh7th commented 4 years ago

Yes. You are right.

Shatur commented 4 years ago

But I still see checks in the latest master. Is it expected?

hrsh7th commented 4 years ago

Sorry. I've updated it now. 🙇

Shatur commented 4 years ago

Thanks! Is it possible to to fix diagnostics at least after saving all files as in VSCode?

hrsh7th commented 4 years ago

I try it.

I guess VSCode sent didChange on save (if the server does not support textDocumentSync.save capability.

Shatur commented 4 years ago

It would be awesome. In C++ is very common to use a lot of different files :)

hrsh7th commented 4 years ago

Could you test https://github.com/hrsh7th/vim-lamp/commit/81a9312c00b96b02f58d78fea40e6544115648c1 ?

Shatur commented 4 years ago

Any differences with and without this fix :( Diagnostics becomes updated only after several time.

Shatur commented 4 years ago

asciicast

Shatur commented 4 years ago

In this video I used small helper command that just creates implementation in .cpp. But the issue occurs even with manual editing.

hrsh7th commented 4 years ago

Sorry. Probably I'm misunderstood your mean.

I just implemented updating diagnostics at saving the buffer.

Kapture 2020-05-21 at 0 13 27

Shatur commented 4 years ago

Tested, but it looks like I still have the same issue as with creating implementation: asciicast

hrsh7th commented 4 years ago

Is the following feature you want?

  1. Open *.cpp file and *.h file.
  2. Edit *.h file (And the edit occurs error on *.cpp.
  3. Focus *.cpp.
  4. Update *.cpp diagnostics.

If so, I can implement it (But it is not general implementation, I think).

Shatur commented 4 years ago

Yes, it would be just awesome. But if this is a bit hacky I would suggest to just add the ability to update manually reload diagnostics. This would also help to debug your issue.

hrsh7th commented 4 years ago

I've added experimental.did_change_on_focus option. Please test it.

Shatur commented 4 years ago

Sorry, but how can I enable it?

Shatur commented 4 years ago

Hm... I experienced another issue: After swathing between header / cpp completion sometimes stop to working. I tested previous commits (via checkout) and the issue still exits: asciicast

hrsh7th commented 4 years ago

Sorry. You can enable it by lamp#config('experimental.did_change_on_focus', v:true) on lamp#initialized autocmd.

I try to use this feature more.

Shatur commented 4 years ago

Sorry. You can enable it by lamp#config('experimental.did_change_on_focus', v:true) on lamp#initialized autocmd.

Thanks, tested, but still not works :( asciicast

Shatur commented 4 years ago

Hm... I experienced another issue: After swathing between header / cpp completion sometimes stop to working. I tested previous commits (via checkout) and the issue still exits.

This is another issue that related with recent changes. I found a way how to reproduce it:

  1. Open .cpp file. You will have completion.
  2. Go to the end of file and type void.
  3. Delete it.
  4. Completion from LSP stop working.

Video: asciicast

hrsh7th commented 4 years ago

Thanks, tested, but still not works :(

Very strange... My environment works well...

This is another issue that related with recent changes. I found a way how to reproduce it:

Open .cpp file. You will have completion. Go to the end of file and type void. Delete it. Completion from LSP stop working. Video:

It is reproduced on my machine. It seems bug of lamp's diff algorithm. It is difficult to fix but I try it.

Shatur commented 4 years ago

Very strange... My environment works well...

Okay, I will try without any plugins.

It is reproduced on my machine. It seems bug of lamp's diff algorithm. It is difficult to fix but I try it.

At least it reproduced :D

hrsh7th commented 4 years ago
16:43:16    clangd          -> [NOTIFY] {'method': 'textDocument/didChange', 'params': {'contentChanges': [{'range': {'end': {'character': 0, 'line': 12}, 'start': {'character': 0, 'line': 12}}, 'text': '', 'rangeLength': 0}], 'textDocument': {'uri': 'file:///path/to/Sample/keyboarddaemon.cpp', 'version': 5}}}
16:43:16    clangd          [STDERR]    I[16:43:16.873] <-- textDocument/didChangeE[16:43:16.874] Failed to update /path/to/Sample/keyboarddaemon.cpp: Line value is out of range (12)

Probably end of line diff was broken.