microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
10.91k stars 764 forks source link

Support that server can opt-out of requests on non open document #1912

Open angelozerr opened 3 months ago

angelozerr commented 3 months ago

Perhaps it is a stupid question, but I have not seen some docmentation about that.

I'm implementing an LSP support for IntelliJ https://github.com/eclipse-lsp4j/lsp4j it starts working well but sometimes my textDocument/* (ex : textDocument/codeLens, textDocument/documentLink) is done before the didOpen.

This usecase comes from only when the language server starts, and takes some times.

It is the reason why I ask the following question : Is didOpen must be done before textDocument/* ?

Perhaps it is natural in vscode, but in my case, the didOpen is done in a Thread (to avoid blocking the IDE) and the call of textDocument/codeLens is done by the implementation of IJ CodeVision where I don't master the call.

I suppose that I need to implement something to consume textDocument/codeLens before the didOpen, but before doing that I would like to be sure that the it is the proper behavior.

If it is the proper behavior, is LSP specification didOpen could mention that didOpen must be done before all textDocument/*? As didOpen is a notification (there is no response) how the LSP client can be sure that didOpen is finished on language server side?

Many thanks for your answer.

dbaeumer commented 3 months ago

Actually a server should be able to full fill requests without a didOpen notification. Otherwise you could not ask questions about documents that are not open. The LSP spec says:

Note that a server’s ability to fulfill requests is independent of whether a text document is open or closed.

DidOpen / didClose transfers the ownership of the document's content between server and client. It purpose is not to allow requests.

I do agree that a better name could have been chosen :-).

Hope that helps.

angelozerr commented 3 months ago

Thanks @dbaeumer for your answer.

The main problem is that all language server that I have seen (and I have implemented) consider that a feature like textDocument/codeLens requires the fill of internal document filled with didOpen.

In Rust language server for instance, they throw an error that document is not opened when textDocument/codeLens is called. Is it a wrong implementation?

The behavior between vscode and IntelliJ with the same language server are different. vscode seems doing in any case didOpen before codelens, so there is not the problem. In my case with IntelliJ the didOpen can occur after codeLens and I have an error with the same language server and same context file in vscode.

So I have the impression that vscode ensures that didOpen is every time done before codelens.

dbaeumer commented 3 months ago

So I have the impression that vscode ensures that didOpen is every time done before codelens.

The reason for this is that codeLens requests in VS Code are only sent for documents that are present in the editor. Having them visible in the editor results in an didOpen notification since the ownership of the content of the document move to the client.

However VS Code also offers API to trigger codeLens requests programmatically. If this API is used then a request can be sent to the server without a prior didOpen (and this is totally legal from an LSP point of view).

angelozerr commented 3 months ago

However VS Code also offers API to trigger codeLens requests programmatically. If this API is used then a request can be sent to the server without a prior didOpen (and this is totally legal from an LSP point of view).

Ok thanks for the clarification. My initial question was about LSP client implementation (and not LSP language server implementation) and I think the question can be splitted in 2 questions.

Language Server & didOpen

At first I have never seen some language server implementation which takes care of call of features (codelens, hover, completion, etc) without calling didOpen. Takes a sample with CSS language server with completion https://github.com/microsoft/vscode/blob/dc29cb5afd8dcdb7bfbad15f12fbc0443958ed74/extensions/css-language-features/server/src/cssServer.ts#L198

//cc @aeschli

The following code assumes that a didOpen has occured (the https://github.com/microsoft/vscode/blob/dc29cb5afd8dcdb7bfbad15f12fbc0443958ed74/extensions/css-language-features/server/src/cssServer.ts#L43 fill the documents map when a didopen occurs).

Is it a wrong implementation? If yes, if I understand correctly your answer it means that the code should be updated to check if documents has been opened (documents map is filled) and otherwise language server should open the content of the file himself?

LSP client with editor & didOpen

I think LSP client implementation editor should take care of calling didOpen before other features, otherwise the if language server opens himself the file it could be unsynchronized with the editor content.

Hope I'm not wrong and I have understood correctly your explanation.

aeschli commented 3 months ago

Loading resources that are not open is something that could be added to the CSS language server, but I think it's also valid if a server decides not to handle certain URIs. Reading a resource is not trivial. For file URI's the tricky part s loading the file with the current encoding and for other URI schemes all depends on the scheme. If I were to implement it I would probably forward the 'load resources' call back to the client. There was a plan at some point to have a file system API in LSP as well, but there's been no progress on that front AFAIK.

rchl commented 3 months ago

I was also thinking that if the server should be able to fullfill requests for non-open files then how would it know which languageId to use? It would have to maybe have an internal mapping of extension -> languageId but then what's the point of client specifying it in didOpen if the server needs to be able to figure it out itself anyway?

I would prefer if the spec said something to the effect of "the server might be able to fullfill requests for files that are not opened by the client".

angelozerr commented 2 months ago

@dbaeumer why the issue has been closed? I think LSP spec should be improved to explain the correct behavior for:

I ask you because I have seen 2. and 3. behavior in different LS, so I think teh behavior should be more clarified.

Thanks!

dbaeumer commented 2 months ago

Sorry, that someone slipped through the cracks.

I do see @aeschli point that for language servers that don't operate on a file system it is hard to fullfil these requests. Instead of returning nothing it would make sense to allow them to provide an error that the client can handle appropriately

This being said we shouldn't ask client to open file automatically since an open is an ownership transfer and usually triggers a lot of other events.

HighCommander4 commented 6 days ago

Clangd is another example of a language server which does not currently support requests on files which haven't been opened with didOpen.

In clangd's case, I believe the reason is performance related: building the initial AST for a file is slow (often several seconds) but incremental updates are fast, and if a file is not open (client is not sending didChange events to keep the server up to date with changes), we'd have to do the slow AST build for each request.

dbaeumer commented 6 days ago

@HighCommander4 IMO doing a slow AST build is fine. All requests are async and a client should usually not be blocked by this.