Ignore onDidOpenTextDocument+onDidCloseTextDocument that fire together when showing definition hover previews

[DanTup]

VS Code sends both onDidOpenTextDocument and onDidCloseTextDocument immediately together whenever it renders a preview of a document in a tooltip:

microsoft/vscode#109908
microsoft/vscode#84875

Apparently this is by design, but it results in the LSP client telling the server to open and then immediately close a document. This may trigger expensive work of analyzing a file that has not been opened by the user.

This was fixed by TypeScript in the client extension (microsoft/vscode@fa72810). If that's how this should be fixed, it seems like the LSP client will need the same workaround since there is no client code for us to implement ourselves this as LSP users.

(Honestly I think this would be better handled in VS Code, but since it's been rejected I think it should be done here rather than in the LSP server, as it seems silly for LSP servers to be adding delays to work around VS Codes quirks).

[dbaeumer]

As commented in the VS Code issue fixing this requires microsoft/vscode#15178. Everyhing else is not guaranteed to work.

[dbaeumer]

If you want to experiment with this you can always try to hold these back in the middleware and see if a close event fires. But I agree it will be cumbersome.

[DanTup]

@dbaeumer that issue has been around for 4 years, I'm not hopeful it's going to be addressed anytime soon (even though it solves lots of problems). Isn't it worth having some fix here in the meantime? (TypeScript has implemented one specifically to avoid this).

[dbaeumer]

@DanTup it is a lot of work since the client does currently no batching. It forwards everything to the JSON RPC layer which does have a queue but no knowledge about the method types. Having two queues will be a stupid idea and breaks the short circuit cancel implementation in the JSON RPC layer. So to address this in the current implementation we would need to allow to plugin a strategy into the JSON RPC layer to allow some rewriting of the queue before a message is put onto the wire.

[DanTup]

Understood. I had other ideas (like dropping the event if the document is not in the active editor list), but honestly everything feels like a bit of a hack because of limited/incompatible VS Code APIs. It's kinda weird that "Ctrl+clicking a symbol has a delay" is blocked by "VS Code doesn't have an API to know which files are open" 😔

[dbaeumer]

Dropping the event if it is not the active editor list doesn't work because of tab management. You can have two tabs a.ts and b.ts but there must not be an active editor for b.ts. This is why we have the feature request about the tab management. So the only way to address this in a way that doesn't introduce other problems is to not send the open event if there is already a close event known or if we get access to the tab management in the VS Code API.

[DanTup]

Understood.

So to address this in the current implementation we would need to allow to plugin a strategy into the JSON RPC layer to allow some rewriting of the queue before a message is put onto the wire.

Is this a realistic possibility? If so, how much longer would VS Code have to not have the tab API for it to be worth doing this? There are still other open issues caused by those APIs (like https://github.com/microsoft/language-server-protocol/issues/1045 and https://github.com/microsoft/language-server-protocol/issues/1031).

[dbaeumer]

Even if we add filtering in the JSON-RPC layer it will not help addressing #684 nor will it help with https://github.com/microsoft/language-server-protocol/issues/1031.

So I am not a fan of going down that route.

[DanTup]

Yep, I meant they were also issues blocked by VS Code not having the open-editors API. It seems like there's a lot of things needing it, but still strong resistance to supporting it.

I may consider adding a queue in the server (based on the client info saying it's VS Code). Really I only need to catch when these two events are adjacent. It's a little messy, but not sure there are any other options.

[dbaeumer]

Doing this server side is not a bad idea. I do it in ESLint as well and even fold change events.

[DanTup]

Doing this server side is not a bad idea.

It might be the only way, but I do think it's a bad idea. It seems like a slippery slope having (potentially many) editor-agnostic LSP servers work around the (very strange) quirks of one specific editor 😔

[dbaeumer]

I agree with the open/close for the hover. This needs to be addressed in the client. However having a queue in the server is very helpful as well. In ESLint I for example fold incremental change events and only validate a file on it latest state even if I have n change notifications in the queue. Such a logic is very server specific and can't be implemented in general on the client.

[DanTup]

However having a queue in the server is very helpful as well

Even with a queue, coding logic to hold Open events around to see whether a Close event arrives within some period of time feels like a client-specific hack. When a user opens a file, we want to provide information as fast as possible, so delaying things for all clients seems like a step in the wrong direction :-(

In ESLint I for example fold incremental change events and only validate a file on it latest state even if I have n change notifications in the queue

How do you decide when to process them? What happens if a request comes in while there are edits in the queue that would depend on those changes (eg. a hover's position may be incorrect if you have unapplied edits)?

[dbaeumer]

The document changes are always applied. When I schedule a validate I remember the version of the text document to check and if it has moved forward I might cancel the validate request since I definitely know that I need to redo the work. This is not working for hover.

[DanTup]

There's a proposed VS Code API for "open tabs" rather than documents:

microsoft/vscode#133532

Is it reasonable for this LSP client to switch over to that when it ships? (If so, feedback is being solicited on the issue above).

[dbaeumer]

The new diagnostic pull is already using the API. The idea for LSP is that a client will be able to configure this. Some servers might be interested in receiving open documents even if they are not visible in the client.

[DanTup]

The idea for LSP is that a client will be able to configure this.

Great, that'll work for me :-)

Some servers might be interested in receiving open documents even if they are not visible in the client.

Out of curiosity, do you have a use case for when a server might want this? My understanding was that these "open documents" according to VS Code/LSP Client are just things the editor has internal models for, and do not reflect what the user would consider an open document. What use are these to the server?

[dbaeumer]

Virtual text documents for embedded languages is one example.

[pgrivachev]

Hey folks, what is the reliable solution on client side for this issue?

I tried to filter requests (in client's middleware) by visible tabs, but I'm not sure about this solution.

middleware: {
  didOpen: async (data: TextDocument, next: (data: TextDocument) => Promise<void>): Promise<void> => {
    window.tabGroups.all.some(g => g.tabs.some(t => data.uri.fsPath.endsWith(t.label))) ? next(data) : undefined;
  },
},

[DanTup]

This is somewhat related to #848. I believe there is a plan to have an option for the client to use VS Code's new tab model so that only "really open" files are sent:

#848 (comment)

For the more general fix. When VS Code has a proper tab model I will add support to the LSP libs to configure if open should be send for invisible files or not. But this will be an implementation detail in the extension / libs.

[pgrivachev]

Thank you @DanTup!

[dbaeumer]

Dups #848