Use textDocument/diagnostic method for handling diagnostics
#217
Conversation
| text_document_registration_options: TextDocumentRegistrationOptions { | ||
| // Use Client's document selector | ||
| // (i.e. Server supports diagnostics for all files Client cares about) | ||
| document_selector: None, |
There was a problem hiding this comment.
| }, | ||
| diagnostic_options: DiagnosticOptions { | ||
| identifier: Some("ark-diagnostics".to_string()), | ||
| inter_file_dependencies: true, |
There was a problem hiding this comment.
I'm not 100% sure how this works, but with this change to using diagnostic(), I do occasionally see that diagnostics from test.R will get updated when you switch to test2.R and do some work over there that affects the ones in test.R. I don't think this is based on the language itself in any way, I think it just remembers what file you came from, and will update diagnostics of that file on some reasonable interval.
I don't really plan on relying on this behavior in any way, but it doesn't seem to hurt anything
| // the document now matches the version of the change after applying | ||
| // it in `on_did_change()` | ||
| if params.text_document.version == version { | ||
| diagnostics::enqueue_diagnostics(self.clone(), uri.clone(), version); |
There was a problem hiding this comment.
No longer here in did_change()
| async fn request_diagnostics( | ||
| backend: &Backend, | ||
| uri: &Url, | ||
| ) -> tower_lsp::jsonrpc::Result<Vec<Diagnostic>> { | ||
| // SAFETY: It is absolutely imperative that the `doc` be `Drop`ped outside | ||
| // of any `await` context. That is why the extraction of `doc` is captured | ||
| // inside of `generate_diagnostics()`; `doc` is dropped as this exits, before | ||
| // `publish_diagnostics().await`. If this doesn't happen, then the `await` | ||
| // could switch us to a different LSP task, which will also try and access | ||
| // a document, causing a deadlock since it won't be able to access a | ||
| // document until our mutable `doc` reference is dropped, but we can't drop | ||
| // until we get control back from the `await`. | ||
|
|
||
| // The document is thread safe to access due to the usage of DashMap | ||
| let doc = unwrap!(backend.documents.get(&uri), None => { | ||
| log::error!( | ||
| "[diagnostics({version}, {uri})] No document associated with uri available." | ||
| ); | ||
| return None; | ||
| }); | ||
| // inside of `check_known_document()` and `try_generate_diagnostics()`; `doc` is | ||
| // dropped as these exit, before `sleep().await`. If this doesn't happen, then the | ||
| // `await` could switch us to a different LSP task, which will also try and access | ||
| // a document, causing a deadlock since it won't be able to access a document until | ||
| // our `doc` reference is dropped, but we can't drop until we get control back from | ||
| // the `await`. | ||
|
|
||
| // Before doing anything, check that we know about this file and get its version | ||
| let version = check_known_document(backend, uri)?; | ||
|
|
||
| // Wait some amount of time. Note that the document version is updated on | ||
| // every document change, so if the document changes while this task is waiting, | ||
| // we'll see that the current document version is now out-of-sync with the version | ||
| // associated with this task, and toss it away. | ||
| tokio::time::sleep(Duration::from_millis(1000)).await; | ||
|
|
||
| // Get reference to document. | ||
| // Before sleeping | ||
| try_generate_diagnostics(backend, uri, version) |
There was a problem hiding this comment.
This function has been carefully designed so that check_known_document() and try_generate_diagnostics() are both not async. Each individually get() the underlying document out of the DashMap and drop() it at the end before returning. See the SAFETY note.
I have an additional prototype spec'd out that uses request_diagnostics() for generating diagnostics after each Console execution, so this function was modeled with that in mind too.
| let code = tower_lsp::jsonrpc::ErrorCode::from(-32802); | ||
|
|
||
| let data = DiagnosticServerCancellationData { retrigger_request }; |
There was a problem hiding this comment.
I learned in my reading of the spec that we as the Server can cancel the request on our end
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticServerCancellationData
DavisVaughan
force-pushed
the
feature/pull-diagnostics
branch
from
15256e9 to
e1999cd
Compare
|
Due to some things I'm still figuring out, I no longer think this is going to be the right path forward. I'll explain more elsewhere but mostly I think this isn't going to work because of this note:
Which prevents us from updating pulled diagnostics with our own push event after a console execution |
|
Ok, I'm officially closing this in favor of #224 In theory it would be kind of nice if the Client was in charge of "pulling" diagnostics for us, but we as the Server often do have more information than the Client about what is happening on the R side, which puts the Server in a better place to be able to refresh diagnostics after execution in the Console, for example. The big problem with mixing So instead my plan is to only use |
This is a step towards generating diagnostics after each Console execution
The main difference you will see from this PR is that diagnostics will now immediately be run when a document is opened, rather than waiting for you to change something in the file.
In our LSP, we currently have 1
publish_diagnostics()call that gets fired fromdid_change()events.I think what the protocol really wants you to do is to register a
diagnostic() -> Result<DocumentDiagnosticReportResult>async method and then specify that you as a Server support "pull diagnostics"https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics
The way this generally works is that it puts the Client (really, the vscode-languageserver-node client implementation) mostly in charge of deciding when to "request diagnostics" for a particular file. This has the benefit of generally being a bit smarter than what we can do as a Server. For example, right now we don't get diagnostics "on open", you have to actually type something in a file first. Sure we could add that to
did_open(), but in general it seems like this could actually result in false positives and us doing more work than we need to, see:So this PR switches us from using notification based, server controlled
publish_diagnostics()to request based, client controlleddiagnostic().Looking at the actual source, that lets us generate diagnostics "on change" and "on save" events automatically, depending on the settings used (default is on-change but not on-save):
https://github.com/microsoft/vscode-languageserver-node/blob/c0982bf544bab9600df223146dd75c93c9004dc3/client/src/common/diagnostic.ts#L960-L978
And also on "on open"
https://github.com/microsoft/vscode-languageserver-node/blob/c0982bf544bab9600df223146dd75c93c9004dc3/client/src/common/diagnostic.ts#L910-L933
In general, we would respond to anywhere you see
pull(document)in this file:https://github.com/microsoft/vscode-languageserver-node/blob/c0982bf544bab9600df223146dd75c93c9004dc3/client/src/common/diagnostic.ts