Skip to content
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.

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

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

Open
angelozerr opened this issue Mar 25, 2024 · 8 comments
Open

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

angelozerr opened this issue Mar 25, 2024 · 8 comments
Labels
feature-request Request for new features or functionality
Milestone

Comments

@angelozerr
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

aeschli commented Mar 28, 2024

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
Copy link
Contributor

rchl commented Apr 2, 2024

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
Copy link
Contributor Author

angelozerr commented Apr 24, 2024

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

  • LSP client : I have the impression that LSP client should take care that didOpen should be called BEFORE all LSP features like codelens, inlay hints,etc
  • LSP server : what is the correct behavior when document is not opened :
    1. load the resource himself but as @rchl said, we will loose the languageId information
    2. or throw an error which mentions that didOpen must be done before the LSP feature codelens, etc
    3. or returns an empty are of codelens?

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
Copy link
Member

dbaeumer commented May 6, 2024

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.

@dbaeumer dbaeumer changed the title Is didOpen must be done before textDocument/* ? Support that server can opt-out of requests on non open document May 6, 2024
@dbaeumer dbaeumer reopened this May 6, 2024
@dbaeumer dbaeumer added the feature-request Request for new features or functionality label May 6, 2024
@dbaeumer dbaeumer added this to the On Deck milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

4 participants