Skip to content

Enable setting and updating diagnostics on a single quickfix list(diagnostics) #974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Enable setting and updating diagnostics on a single quickfix list(diagnostics) #974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0853e83
Enable setting and updating diagnostics on a single quickfix list
ajayvigneshk Feb 12, 2020
c9f11de
Add abort attribute for vim function
ajayvigneshk Feb 12, 2020
c1bd434
cargo fmt checks fix
ajayvigneshk Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions autoload/LSP.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
" TODO: make buffer aware.

function! LSP#GetQfListIdForTitle(title) abort
echom a:title
let headnr=getqflist({ 'id':0 ,'nr':'$', 'title': 0}).nr
while headnr >= 0
let qflistat= getqflist({'id':0,'nr':headnr,'title':0})
if qflistat.title==a:title
return qflistat.id
endif
let headnr = headnr-1
endwhile
return-1 "not found
endfunction

function! LSP#filename() abort
" When executing autocommand, `%` might have already changed.
let l:filename = expand('<afile>:p')
Expand Down
10 changes: 9 additions & 1 deletion src/language_server_protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,15 @@ impl LanguageClient {
let diagnostics_list = self.get(|state| state.diagnostics_list)?;
match diagnostics_list {
DiagnosticsList::Quickfix => {
self.vim()?.setqflist(&qflist, "r", title)?;
//Has to be atomic
self.update(|state| {
let id = state.vim.getqfid(title)?;
if id != -1 {
state.vim.updateqflist(&qflist, title, id)
} else {
state.vim.addnewqflist(&qflist, title)
}
})?;
}
DiagnosticsList::Location => {
self.vim()?.setloclist(&qflist, "r", title)?;
Expand Down
19 changes: 19 additions & 0 deletions src/vim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,25 @@ impl Vim {
Ok(())
}

pub fn updateqflist(&self, list: &[QuickfixEntry], title: &str, id: i32) -> Result<()> {
info!("Begin updateqflist");
let parms = json!([[], "r" ,{"title":title,"id":id, "items":list}]);
self.rpcclient.notify("setqflist", parms)?;
Ok(())
}

pub fn addnewqflist(&self, list: &[QuickfixEntry], title: &str) -> Result<()> {
info!("Begin addnewqflist");
let parms = json!([[], " ",{"title":title, "items":list}]);
self.rpcclient.notify("setqflist", parms)?;
Ok(())
}

pub fn getqfid(&self, title: &str) -> Result<i32> {
info!("Begin getqfid");
self.rpcclient.call("LSP#GetQfListIdForTitle", title)
}
Comment on lines +209 to +226
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we moved all of this to a single vim function, say: UpsertQflist or something like that. I know this is used in a single place right now, but if it were to be used somewhere else it would require repeating that logic of check if there's an existing qflist, if there is update, if there isn't create. So I think it would be better to have this logic on the vimscript side. That would also mean that you can get rid of that self.update call you made for atomicity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.update call seems to be required for locking. For instance, without it, I can see multiple concurrent calls on the log to qflist modification methods causing inconsistent behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't really need to lock to get access to the vim rpcclient though. Or at least not through .update. I understand you wanted to make those 3 calls as atomic as possible with that though, but what I mean is, if we move that logic to vimscript you won't need to call self.update, cause it's a single call you'll be making, and you can get away with just self.vim()?.

So my suggestion is to basically create a function in vimscript that takes two arguments, lines and title. And have that function search for a qflist with that title and create one if it doesn't find it, and set the items on that list, all within the same function. Again that allows you to get away with a single call to vim from rust and also means that other bits of the code that need that logic won't have to repeat the search/create -> set lines logic again.

Copy link
Author

@ajayvigneshk ajayvigneshk Oct 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the motivation / intended change too. This section of code has to be locked across multiple textDocument/publishDiagnostics notification handlers which appear to be concurrent. Can you help me with that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took so long to get back, I seemed to have not noticed this message. My point was that you don't really need to sync between concurrent publishDiagnostics. What makes you think you need to take care of that though? Have you experienced any issues with it?

Copy link
Author

@ajayvigneshk ajayvigneshk Dec 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, Yes I did face issues with it. For eg. something like this say there're two publishDiagnostics events. publishDiagnostics event -> check if named quickfix list exists -> if not, create qf list.
I faced issue with two qf lists getting created, because before creating qf list as a part of handling the first event, the second one checks if a qf list already exists, gets responded in the negative, effectively making both handlers create qf lists. This defeats the intended purpose of this change.


pub fn setqflist(&self, list: &[QuickfixEntry], action: &str, title: &str) -> Result<()> {
info!("Begin setqflist");
let parms = json!([list, action]);
Expand Down