Add options to only sync visible documents to the server

[DanTup]

This is slightly related to microsoft/vscode#15178. Right now there's no way for an editor to know when a document is either "open" or "visible" in a way that matches the users expectation:

Open in this sense means it is managed by the client. It doesn’t necessarily mean that its content is presented in an editor.

There are many situations where a server would like to know what files a user has open (or visible). For example:

• The user wants to ignore lints/errors in certain files - for example files generated by tools
• When the user opens the file in the editor, they want to see these errors/lints (eg. to debug something going wrong in them)
• When the user closes the file, they expect the errors/lints to disappear (immediately, not after 3 minutes)

Even though VS Code doesn't have an API for "open editors", it does have an API for "visible editors". Even if VS Code plans to never support "open editors" it would be very useful for LSP to provide "visible editors". On the other hand, if VS Code does add support for "open editors", then that could be provided instead/as well. In either case, it seems like LSP needs some changes to support one or the other (or both).

(as a workaround, I'm considering implementing custom messages in the VS Code extension + LSP server to pass the "visible editors" over to the server.. it feels really hacky though).

[dbaeumer]

I am very reluctant to sync UI state to the server. We really tried hard to not do this. I do see the problem with the diagnostics and the problem there is that it is a pure push model. So there are discussion to allow a pull model as well where this again would go back under the control of the editor. See microsoft/language-server-protocol#737

And even using VS Code visible editor will not help since there is no API for the tab model (e.g. you don't want to see lint errors disappear for editors for which there is still an open tab).

Any objection to fold this with microsoft/language-server-protocol#737

[DanTup]

I am very reluctant to sync UI state to the server. We really tried hard to not do this.

In Dart the server has a concept of "priority files". That means if there are multiple outstanding requests for multiple files, those that are marked priority are processed first. These are intended to be set to the visible (or at least open) files. Without allowing the server to know what files the user has open (or visible), this isn't possible (currently for non-LSP we use visible files and it works great - for LSP we have to use "open" files which is a bit wonky).

I do see the problem with the diagnostics and the problem there is that it is a pure push model. So there are discussion to allow a pull model as well where this again would go back under the control of the editor. See microsoft/language-server-protocol#737

This seems like it would increase server resources (especially if more things move to this model). Servers will have to start caching more data (or re-computing it frequently) if the client can ask for it at any time. The client is already caching it to show it in its UI - having the server able to tell the client when it changes feels like a better model to me.

That said - I'm not sure how it solves this issue. How does the server know whether to include the "excluded" file when the client asks for all diagnostics? (I'm assuming the client would ask for all, and not individually for each open file - as that would just lead to a lot of complaints - see microsoft/vscode#13953).

And even using VS Code visible editor will not help since there is no API for the tab model (e.g. you don't want to see lint errors disappear for editors for which there is still an open tab).

Errors disappearing (for excluded files) when you tab to another file is a lot more understandable than randomly vanishing up to 3 minutes later (and perhaps even appearing when you don't have the file open, since didOpen makes no guarantees the user actually opened the file) :-)

[dbaeumer]

To avoid caching on the server side we are looking into the concept of batch requests (see microsoft/language-server-protocol#988).

The current concept in LSP is that it is under the clients control what data is visible and what data therefore need to be priorities.

And the concept of an open editor is not enough. A tool could for example show the outline inline in the file explorer and therefore the outline needs to be priorities. So if we let the server decide this then we need to let servers know about what is visible in the UI and what not in terms of LSP requests.

[DanTup]

A tool could for example show the outline inline in the file explorer and therefore the outline needs to be priorities. So if we let the server decide this then we need to let servers know about what is visible in the UI and what not in terms of LSP requests.

That's true, but I was thinking about things that are pushed from the server (like errors) - if a user opens a project of 5000 files, the server needs to parse/lint them all, and doing the open files first is most useful.

I don't think batching helps the case I had in mind, which is the client asking the server for some data multiple times even if it hasn't changed. For example if the client asks the server for errors multiple times, the server has to either keep a cache of them, or re-compute them. Batching requests seems like it could prevent parsing the file multiple times for different requests, but I don't think it prevents requiring the server to keep copies of the data or re-compute them.

I also don't think this addresses the original need though - which is wanting to show errors for some files only when they're open. Even if the errors were pulled from the server, they would still persist when the file was closed unless there was some mechanism to tell the client only to show them while the file is open.

[dbaeumer]

That's true, but I was thinking about things that are pushed from the server (like errors)

I agree with that case but it is the ONLY case in LSP where the server pushes. Everything else is pull. So instead of having a UI model sync for errors I would prefer having a pull model for errors.

And errors are not only presented in the editor. There are problem views and decorations in the file explorer.

[DanTup]

I agree with that case but it is the ONLY case in LSP where the server pushes. Everything else is pull. So instead of having a UI model sync for errors I would prefer having a pull model for errors.

But doesn't pull mean the server has to invalidate everything on every change? If a user is typing inside a comment - with a push model, the server doesn't need to send any new errors, outline, code fixes, etc. - but if the client is pulling, it's going to keep asking for these and forcing the server to compute them (or keep a cache of them). And when the extension host is remote (Codespaces, etc.) it seems like the extra latency/slow round trips would make this worse?

That aside - I still don't think any of that addresses this need. Even in a pull model, it doesn't allow us to show errors for ignored files only while they're open.

[DanTup]

#683 is a related issue. VS Code fires did-open events whenever a file is opened (which includes things like extensions opening them, or the hover preview when holding Ctrl/Cmd), which ends up at the LSP server and causes it to start analyzing files that have never been opened.

[dbaeumer]

@DanTup did you have any chance to look at the new pull model. I would like to close this issue since I really don't want to sync UI state.

[DanTup]

@dbaeumer I've not looked at this yet - last time I looked I couldn't find the related info (I just had a quick search of the specs and still can't see anything obviously related to pulling diagnostics?), and also this same issue affects us in many other ways that are not all related to diagnostics so I was hoping for a more general fix.

For example, our server likes to prioritize and cache parsed results for files that are "open" but not for every file in the workspace.

[dbaeumer]

The proposed implementation and spec is here:

vscode-languageserver-node/protocol/src/common/proposed.diagnostics.md

Line 1 in eba6a73

#### <a href="#pull_diagnostic" name="pull_diagnostic" class="anchor">Pull Diagnostics</a>

and

vscode-languageserver-node/protocol/src/common/proposed.diagnostic.ts

Line 1 in 301224d

/* --------------------------------------------------------------------------------------------

[dbaeumer]

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.

[DanTup]

Thanks - I'll have a dig through that and see whether it would be useful.

this will be an implementation detail in the extension / libs.

I still think there could be value in discussing this in the spec (for benefit of other servers/editors), but I don't know that any other editors using our server are doing what VS Code is, so solving it there probably solves the issue entirely (at least for me) anyway.

Feel free to move this to the LSP lib repo, or I can do it if you want to close this one. Thanks!

[ljw1004]

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.

@dbaeumer could you expand on this? Will the option also control whether didClose is sent for invisible files? And will it also control whether (say) documentSymbols is sent for invisible files?

(I'm asking because I'm implementing a hacky temporary middleware which suppresses didOpen+didClose in the case that it's an invisible tab, and I'd like my middleware to have the same behavior as what you eventually envisage for your LSP option.)

[dbaeumer]

@ljw1004 The idea is to not send and open/change/close events for documents that are not visible in an editor. However since LSP allows sending language requests for not opened documents those will still be allowed on these documents. Sending a language request might need to trigger and open/change notification in case the document is dirty on the client.

Instead of putting this into your own middleware would you consider providing a PR for this?

[mhuisi]

What is the current state of this issue? Is there any workaround that works and reliably filters exactly the invisible documents?

Not spamming the language server with didOpen notifications is important for us in the language server for the Lean 4 programming language and theorem prover since processing documents after didOpen can be potentially very expensive (e.g. Lean needs to execute meta-programs, arbitrary proof automation, dependent type checking, higher order unification and typeclass search so that we can provide diagnostics and respond to requests for the file). We definitely do not want to initiate this process whenever someone hovers over an identifier while holding Ctrl.

A list of things that we have tried in the past to work around this issue:

• Filter using window.visibleTextEditors in the didOpen middleware. This does not work because window.visibleTextEditors is not up-to-date when the didOpen middleware is called.
• Filter all didOpen notifications in the didOpen middleware and add our own window.onDidChangeActiveTextEditor and window.onDidChangeVisibleTextEditors handlers. This does not work because it will occasionally cause a race condition where the language client library attempts to emit requests to the file before our own handlers fired and opened it. See fix: broken document symbols leanprover/vscode-lean4#460 for details.
• Filter using window.tabGroups.all in the didOpen middleware. This often works, but there are some cases where the didOpen middleware fires before window.tabGroups is updated, e.g. in the sequence of steps in Server does not respond to new file leanprover/vscode-lean4#484 where an untitled plaintext file is saved as a Lean document.

I don't see any other VS Code API to filter these requests and I'm wary of messing with the control flow of the language client library since it may introduce undesired race conditions. What is the recommended way to mitigate this issue?

[dbaeumer]

@mhuisi what you can do right now is the following:

• don't do anything on a didOpen in the server
• wait for a concrete request to do the expensive work
• switch from push diagnostics to pull diagnostics to implement diagnostics in that model. The pull diagnostic model will only pull for visible documents.

[mhuisi]

@mhuisi what you can do right now is the following:

* don't do anything on a didOpen in the server

* wait for a concrete request to do the expensive work

* switch from push diagnostics to pull diagnostics to implement diagnostics in that model. The pull diagnostic model will only pull for visible documents.

We can't switch to pull diagnostics because AFAIK VS Code still does not support result streaming / partial results. As mentioned earlier, processing Lean files is expensive and can take a while, so we can't have users wait for diagnostics for (up to) minutes until the diagnostic pull request for the file can be responded to.

[dbaeumer]

So you have a first error after some seconds and the last once after minutes for one file?

[mhuisi]

So you have a first error after some seconds and the last once after minutes for one file?

Yes, because semantic analysis is (necessarily) very expensive in languages like Lean. For a concrete example, proof scripts may use SMT decision procedures to prove lemmas, which in turn can be used by the type checker to ensure that a program satisfies some formal specification. We also implement an additional $/lean/fileProgress push notification that displays an orange text editor decoration in the overview ruler / gutter so that users aren't lost about which parts of the file have already been processed.

There are some other extensions we make to LSP to suit interactive theorem proving needs as well, but those are not really relevant here.

Note that the streaming support issue also has a comment from one of the maintainers of coq-lsp, which is another interactive theorem prover operating under similar constraints as Lean.

[mhuisi]

You can split the diagnostics publishing into two stages even with the new pull model. You can register two pull providers with different identifiers which give you two independent channels to report diagnostics (see4 release/sorry-butterfly-pink/protocol/src/common/protocol.diagnostic.ts#L64)

I would really prefer not to keep piling on additional hacks. It's also not clear to me that this will give us the kind of fine-grained diagnostic reporting that users expect from tools like Lean.

Can you please clarify why e.g. expecting the tabGroups API to accurately reflect the state of the tabs that the user sees when the middleware fires is unreasonable?

[dbaeumer]

I would really prefer not to keep piling on additional hacks.

This is actually not a hack. Other language servers do the same to separate syntax errors from more expensive semantic errors.

The eventing is tricky and I try to stay away form defining in LSP in which order they have to arrive when VS Code events (emitted sync) are combined with LSP middleware call chains (which are async). This is why for example LSP emits its own open sent events instead of users of the libs relying on the didOpen event from VS Code.

Even if it is reflected correctly (there might be a bug / missing event when the input of a editor changes) I still would stay away from specify a event order.

[dbaeumer]

The reason for this is a different event order when open a file or when changing a files language id (save as)

Open

onDidChangeTabs {opened: Array(1), closed: Array(0), changed: Array(0)} 
onDidChangeTabs {opened: Array(0), closed: Array(0), changed: Array(1)} 
onDidChangeTabGroups {opened: Array(0), closed: Array(0), changed: Array(1)}
onDidOpenTextDocument /.../test.txt

Save As

onDidOpenTextDocument /.../test4.txt
onDidChangeTabGroups {opened: Array(0), closed: Array(0), changed: Array(1)}
onDidCloseTextDocument Untitled-1
onDidChangeTabs {opened: Array(1), closed: Array(0), changed: Array(0)}
onDidChangeTabs {opened: Array(0), closed: Array(0), changed: Array(1)}
onDidChangeTabGroups {opened: Array(0), closed: Array(0), changed: Array(1)}
onDidChangeTabs {opened: Array(0), closed: Array(1), changed: Array(0)}

So it is even not something I can fix in LSP without re-interpreting event orders.

[dbaeumer]

The pull diagnostics abstracts from this by listening to open events and tab change events and pulls for the document independent on how it got to live (minus some bugs since as said the whole thing is tricky :-)).

[mhuisi]

I would really prefer not to keep piling on additional hacks.

This is actually not a hack. Other language servers do the same to separate syntax errors from more expensive semantic errors.

The eventing is tricky and I try to stay away form defining in LSP in which order they have to arrive when VS Code events (emitted sync) are combined with LSP middleware call chains (which are async). This is why for example LSP emits its own open sent events instead of users of the libs relying on the didOpen event from VS Code.

Even if it is reflected correctly (there might be a bug / missing event when the input of a editor changes) I still would stay away from specify a event order.

Ah. Unfortunately, all errors in Lean are potentially expensive semantic errors, so there is no simple two-stage separation here that is independent of the progress the server makes on processing the file. Due to Lean's meta-programming features (which are especially helpful for interactive theorem proving!), we cannot proceed with parsing the next command until we have completed the semantic analysis for the previous command, since it may modify the environment of the parser. Hence, given these constraints, I initially understood your suggestion as "use the first pull provider for the first half of the file and the second pull provider for the second half of the file", which I would still refer to as a hack :-)

It's also worth pointing out that most of the interesting diagnostics that we want to display to users are not syntax errors, but exactly those expensive semantic errors. For example, interactive proof editing is one of the main features of Lean, and as users input proofs we need to be able to quickly display semantic errors for the adjusted proof.

So it is even not something I can fix in LSP without re-interpreting event orders.

Should we report this issue to VS Code, or do you think it is hopeless?

The pull diagnostics abstracts from this by listening to open events and tab change events and pulls for the document independent on how it got to live (minus some bugs since as said the whole thing is tricky :-)).

In any case, even if this discussion won't resolve our issues with LSP (at least until VS Code gets streaming support for these requests), thank you for your helpful responses.

[DanTup]

Should we report this issue to VS Code, or do you think it is hopeless?

If you specifically mean it sending open/close when you hover, I think it's hopeless :) I tried that here:

microsoft/vscode#109908

It was closed as a dupe of microsoft/vscode#15178 which has since been implemented.

It affected TypeScript too:

microsoft/vscode#84875

IMO this is a hack. Language servers may want to know about open documents because a) this changes the owner of the document from the disk to the editor and b) they might want to prioritise or cache data related to open files for performance. VS Code sending an open immediately followed by a close when it's just reading the contents from disk because it might show a preview in the tooltip doesn't feel like it needs to trigger any of this, it feels like an implementation detail of VS Code leaking into language servers (either they specifically write code to detect and ignore this, they potentially suffer worse performance because they start work they didn't need to, or they potentially don't send notifications for push-based features until some other request for the file shows up) 😔

[ljw1004]

I work on the Hack LSP server. It also takes time (1s-5mins depending on complexity) to fully recompute diagnostics. My solution:

1. Maintain a list of open files
2. For each didOpen/didChange, mark the file as "dirty"
3. When the LSP is idle (i.e. has no other messages to process), then recompute diagnostics for one of the file's that's dirty.

This solves the transient didOpen/didClose problem nicely. It also offered a HUGE performance benefit for our single-threaded LSP server, because when a user opened a fresh document and did (say) hover/gotodef then the LSP server would be computing latency-sensitive user actions rather than non-critical squiggles.

I don't think of this as a hack! I think it's kind of (in retrospect) the obvious way to implement prioritization of what the LSP should work on next.

When I talk about "performance" I refer to what percentage of hover/gotodef/autocomplete requests get a useful response within 200ms (which is my team's criterion for "adequately fast"). Once I ceased doing work on didOpen, and changed to this lazy model, then we went from ~90% of requests getting responses in under 200ms up to ~95%.

I spent a lot of time experimenting with pull diagnostics and came to the conclusion that they weren't as good a solution. There are two chief reasons: (1) VSCode sends pull diagnostic requests more eagerly than my solution, hence doesn't give as much benefit, (2) more importantly there's no race-free way to combine pull diagnostics with the push diagnostics inherent in a up-to-5-minute error recomputation, although this might be under-specification rather than an inherent blocker. Here are my raw notes:

Normal VSCode didChange behavior. When you change an unmodified file on disk, VSCode updates the editor contents and sends didChange; when you change a modified file on disk, VSCode doesn't update the editor contents and doesn't send didChange. Thus for instance if you rebase at the CLI, then VSCode will send didChange for all files modified by the rebase.

Also, imagine a file on disk has errors in it, but the modified buffer doesn't, and you close the modified buffer. The user will naturally expect the diagnostic to be restored upon file-close. Well, this is helped by some (undocumented) VSCode behavior that (1) immediately prior to didClose, it first synthesizes a didChange event which restores the editor content to what is on disk, apparently via line-diffing. Then it sends a pull diagnostic request for the file. Then (without waiting for a response) it sends didClose.

InterFileDependencies. This is a flag you specify in your server capabilities. It means that a change in one file will give rise to pull diagnostics requests not just for that file, but for all open files. This is true both in the rebase scenario and regular typing. Therefore, rebases will cause exactly the same set of files to get pulled-diagnostic as got didChange.

VSCode has weird behavior (bug?) where it doesn't respect already-open-upon-project-start files for its interFileDependencies. Specifically: if you start with editor tabs open for A and B, and change A, then it will pull diagnostics for A but not B. But if you subsequently switch to tab B and back again to A and change A again, then it will now pull for both A and B.

Sequencing. VSCode sends pull-diagnostics request immediately after sending didChange, but it seems to wait until the previous pull-diagnostic has gotten a response before sending another request. So (1) if there are multiple editor tabs open then it only requests diagnostics one at a time, (2) if you type really fast then it will send didChange for each keystroke but will consolidate and not have a backlog of pull diagnostic requests.

Disk file changes? Races? If you change a file on disk that's not open in the editor, VSCode won't pull any diagnostics (even if you have interFileDependencies turned on). I suppose you must have to use other mechanisms. I don't know what they'd be, i.e. I don't know how my didChangeWatchedFiles handler should trigger the whole pull-diagnostics machinery. Or alternatively, if my LSP responds to this didChangeWatchedFiles event by pushing diagnostics, how will that race with any pull diagnostics that VSCode sends for the file?

Another race I don't understand is if we close a file then VSCode sends didChange followed by a pull-diagnostics request followed immediately by didClose. What if the LSP feels free to push diagnostics for the file after didClose, but VSCode acts upon receipt of pull-diagnostics after that?

Failure? I didn't yet investigate how pull diagnostics works with failure, e.g. if a pull-diagnostic request gets an error, then are any previously pushed diagnostics for this file removed? are any previously pulled diagnostics for this file removed?

All in all, I actually don't see any benefit to pull diagnostics. I think I could achieve the same overall effect by having the LSP server store a list of files which have so far received outstanding didChange/didOpen events, and if it's idle then push diagnostics for them as needed. I could similarly make didChangeWatchedFile add open files into the list to refresh them. This way I wouldn't worry about races.

(The only thing I'd lose with this approach is that pull diagnostics tells me which file should be prioritized).

[mhuisi]

@ljw1004

Unfortunately, I don't think this approach will work for us, as it doesn't really resolve the issue of didOpen requests being emitted for invisible files and subsequently putting pressure on the language server.
We also have no way of separating "processing for diagnostics" and "processing for all other requests" into two separate phases, and the former is by far the most important language server feature for languages like Lean anyways. Additionally, our language server uses multiple processes (one for every open file and one to manage these processes and handle requests that require global information) to reduce overall latency and because of certain technical reasons (e.g.: we mmap data from imports into the process and we need to be resilient against buggy meta-programs and proof automation). There are also lots of threads being spun up inside of those processes for all kinds of reasons :-)

Thanks for your detailed analysis and the word of caution about pull diagnostics.

[mhuisi]

@DanTup I was referring to the tabGroups event ordering issue, since (from my understanding) the tabGroups API was always suggested as the future solution to mitigate these issues in the past.

[DanTup]

I was referring to the tabGroups event ordering issue, since

Oh, you mean so we could use middleware to filter out didOpens for "invisible" documents? That seems valid to me, and is not what I'd already asked for. (I also think if that's solved, it would be nice to support it here rather than in extension-specific middleware). If you open this issue, please post the link here so those of us also interested can add 👍.

[ljw1004]

Unfortunately, I don't think this approach will work for us, as it doesn't really resolve the issue of didOpen requests being emitted for invisible files and subsequently putting pressure on the language server.

@mhuisi The reason it works for us (solves the ephemeral didOpen pressure) is because for those ephemeral cmd+hovers, VSCode sends didOpen followed immediately by didClose. Our LSP server first handles didOpen(file1) by adding file1 to the list of open files and setting its dirty bit. The immediate next thing it does is find didClose(file1) in stdin and so removes file1 from the list of open files. Now our LSP server is idle, so it looks for all dirty files in the list of open files, and doesn't find any, hence experiences no pressure. I'd expected a similar approach to work for you?

(There is a theoretical race condition here, that LSP process might discover empty stdin because the VSCode process I guess got switched out before it had a chance to write didClose, but our telemetry never showed that to happen in practice).

By the way, I earlier tried tabGroup approach suggested in #683 (comment) -- it appeared to work in testing, but once I shipped to production it failed bad enough that we had to do an emergency rollback. Here was my attempted implementation:

/**
 * VSCode behavior is that when you cmd+hover over a symbol, it sends textDocument/definition
 * request, then quickly fires off didOpen+didClose.
 * This has been a long-standing complaint https://github.com/microsoft/vscode-languageserver-node/issues/683
 * because LSPs often do costly work in response to didOpen.
 *
 * Ultimately the plan is for LSP to configure whether it wants to receive didOpen for these ephemeral
 * didOpens https://github.com/microsoft/vscode-languageserver-node/issues/848#issuecomment-960636388.
 * In the meantime, Typescript uses a workaround https://github.com/microsoft/vscode/commit/fa72810c777c07b8589ca0495d343e057d180977
 * which examines the pending queue of messages and if there's a didOpen followed by a didClose
 * then don't bother handling it. Alas we can't use the same workaround because we don't have visibility
 * into the queue, and at the time our didOpen is executed the didClose hasn't yet been sent.
 * So this middleware implements a different workaround suggested in
 * https://github.com/microsoft/vscode-languageserver-node/issues/683#issuecomment-1236347083
 * Namely: when we get a didOpen, check if it truly corresponds to an open tab,
 * and if not then it must have been one of those ephemeral ones. We'll try to replicate
 * future planned LSP behavior by suppressing didOpen (and didClose) for these cases.
 * What should be the behavior if VSCode wants to send other messages too? like documentSymbol
 * or codeAction? I don't know. With this diff I'll just let them through to the LSP.
 * Hopefully the LSP will respond "sorry, action on non-open document, won't do anything."
 * (I don't want this middleware to have to enumerate every single possible message
 * and report the error itself.)
 * Incidentally, the caller kicks off didOpen and didClose in succession, so if our didOpen middleware
 * has any awaits e.g. to find project root, in it then the didClose could reach the client prior
 * to the didOpen! We've implemented it without awaits.
 */
export class EphemeralOpenMiddleware implements Middleware {
  private suppressedUris = new Set<vscode.Uri>();

  constructor() {
    // without the next lines, `this` will be unbound when the middleware functions are called
    this.didOpen = this.didOpen.bind(this);
    this.didClose = this.didClose.bind(this);
  }

  didOpen(
    document: vscode.TextDocument,
    next: (data: vscode.TextDocument) => Promise<void>,
  ): Promise<void> {
    const documentIsInSomeTab = vscode.window.tabGroups.all.some(group =>
      group.tabs.some(tab => document.uri.fsPath.endsWith(tab.label)),
    );
    if (documentIsInSomeTab) {
      return next(document);
    } else {
      this.suppressedUris.add(document.uri);
      return Promise.resolve(undefined);
    }
  }

  didClose(
    document: vscode.TextDocument,
    next: (data: vscode.TextDocument) => Promise<void>,
  ): Promise<void> {
    if (this.suppressedUris.has(document.uri)) {
      this.suppressedUris.delete(document.uri);
      return Promise.resolve(undefined);
    } else {
      return next(document);
    }
  }
}

What we observed is that in production, for reasons I never managed to reproduce mlocally, many of our users encountered a legitimate didOpen (i.e. they truly did open a file) for which documentIsInSomeTab === false. This middleware therefore suppressed it and our language server believed the file was never opened, i.e. we never added it to the LS's open-file-list. But when a subsequent hover/gotodef/completion request came in for that file, well, our LS errors when it receives a request on a non-open file. The number of users who reported "hover sometimes just doesn't work" was high, and it showed up clearly in our telemetry. My conclusion was that the semantics of tabGroups isn't precisely enough defined for me to rest critical code upon it!

[mhuisi]

@ljw1004

The reason it works for us (solves the ephemeral didOpen pressure) is because for those ephemeral cmd+hovers, VSCode sends didOpen followed immediately by didClose. Our LSP server first handles didOpen(file1) by adding file1 to the list of open files and setting its dirty bit. The immediate next thing it does is find didClose(file1) in stdin and so removes file1 from the list of open files. Now our LSP server is idle, so it looks for all dirty files in the list of open files, and doesn't find any, hence experiences no pressure. I'd expected a similar approach to work for you?

Thanks! I had always assumed that there are didOpen notifications for invisible documents that are not closely followed by a didClose. If that is indeed always the case (though it is clearly a bit of a brittle assumption to make), we could also debounce didOpen notifications that are closely followed by a didClose notification in our language server without making any additional adjustments to the language server architecture.

What we observed is that in production, for reasons I never managed to reproduce mlocally, many of our users encountered a legitimate didOpen (i.e. they truly did open a file) for which documentIsInSomeTab === false.

You can find one reproducible example of this in this thread above that was reported by one of our users (saving an untitled file). That's what the tabGroups event order discussion was about.

[dbaeumer]

@ljw1004 @mhuisi @DanTup great discussion. I would like to add a couple of comments:

• it does make sense to not do expensive work immediatelly on didChange. I did the same for ESLint until I moved to pull diagnostics which basically do that for me on the client side for free.
• there can be didOpen events for documents that are not visible in an editor that stay open for a long time. didOpen is in hindsight a bad name since the events purpose is to signal content ownership transfer from the server to the client for a specific document. This usually happens when an editor opens but can also happen in other situations.

Regarding the tab eventing in VS Code. Looking at the event order I would not argue that there is something wrong in VS Code. In the case of saving a untitled file, VS Code generates a new document, however since the editor already exists, it only receives a new input. That results in a changed event on the tab API but on a close/open event on the document. This is different from opening an editor, where the editor first gets created and then the input is created and set.

So if you want to handle this in the middleware you basically need to do the same as the client does for pull diagnostics:

• if you receive a open event check if it is part of the tabs. If yes, forward if no drop
• have a listener on the tab model. If an editor opens or changes check if the document is know on the server. If not, send an open event to the server (you can use getFeature to get the open feature to use the same API: https://github.com/microsoft/vscode-languageserver-node/blob/main/client/src/common/client.ts#L1712)
• if a editor closes or changes check if the document is know on the server and if yes, send a close event.
• if a document closes, check if the document is know on the server. If not, drop the document other send the event.

This is basically what pull diagnostic does.

I haven't implemented that behind a flag since I am not convinced anymore that only syncing visible document to the server is a good approach (there can be long lived documents that are not visible and relevant for a server).

[mhuisi]

there can be didOpen events for documents that are not visible in an editor that stay open for a long time. [...] there can be long lived documents that are not visible and relevant for a server

Do you have an example?

[DanTup]

@dbaeumer

it does make sense to not do expensive work immediatelly on didChange. I did the same for ESLint until I moved to pull diagnostics which basically do that for me on the client side for free.

As far as I can tell, pull diagnostics means VS Code chooses which files to request diagnostics for, but we want users to see all errors across their entire project. When using push diagnostics, we have to reanalyze in didChange.

didOpen is in hindsight a bad name since the events purpose is to signal content ownership transfer from the server to the client for a specific document.

Given this, why is it normal for a client to send didOpen immediately followed by didClose? There is no change of ownership here because the close happens immediately. Given the two cancel each other our, the notifications are pointless and just cause grief for those of us doing work on-change/open.

IMO if VS Code won't fix this, it seems it would be better done here in the LSP client than everyone affected having to write middleware to try and strip this out. (note: I'm not saying the LSP client should necessarily sync only visible files as this issue became, but I do think it could drop these pointless open/closed events that are a VS Code specific quirk and don't make sense over LSP).

[ljw1004]

When using push diagnostics, we have to reanalyze in didChange.

@DanTup I don't think that's true. What we do is that didChange merely sets an "is-dirty" bit in our LS. Then the LS can perform its reanalysis (so clearing the dirty bit) at its leisure, at any time it choses.

[DanTup]

@DanTup I don't think that's true. What we do is that didChange merely sets an "is-dirty" bit in our LS. Then the LS can perform its reanalysis (so clearing the dirty bit) at its leisure, at any time it choses.

Actually we do similar - but in some cases there is some synchronous work we must do (to ensure consistency), and for some configurations we modify our analysis roots based on open files (to support large workspaces where we don't want to analyze everything). We could certainly work around this, but I just don't understand why this is being considered normal. If the request is for transferring ownership to the server, why would it makes sense to send didOpen/didClose immediately just because the user Ctrl+Clicked on a symbol and holding Ctrl happened to trigger a hover and the hover wanted to show a preview of contents?

This feels like a weird quirk/implementation detail of VS Code that doesn't make sense over LSP and servers are having to change their behaviour to try and mitigate the effects of. Why don't we just filter it out here? There is (as far as I can tell) zero benefit in sending those two events over the wire in this case.

[dbaeumer]

I looked into it and here is what I think would make sense to implement to not make things overly complicated:

• if a document is opened and it is NOT visible in a editor the open event will be delayed
• if the editor opens or any other request / notification is trigger (except a close of the delayed document) the open notification will be forwarded to the server.

This behavior will go behind a flag in the client options.

First cut of the code is in dbaeumer/bottom-eagle-chocolate

If you think that this would help for your scenarios I will make next releases so that you can give it a try.

[DanTup]

That sounds good to me, thank you! :)

Will these releases be tied to the next version of LSP? (it's not clear to me what the release schedule is for LSP, I think it's been a while since the last release and I haven't tried to use a client for the new version yet).

[mhuisi]

I looked into it and here is what I think would make sense to implement to not make things overly complicated:

* if a document is opened and it is NOT visible in a editor the open event will be delayed

* if the editor opens or any other request / notification is trigger (except a close of the delayed document) the open notification will be forwarded to the server.

This behavior will go behind a flag in the client options.

First cut of the code is in dbaeumer/bottom-eagle-chocolate

If you think that this would help for your scenarios I will make next releases so that you can give it a try.

Thanks, sounds good! :-)

[dbaeumer]

The new client version 10.0.0-next.9 has the mention support. LanguageClientOptions#textSynchronization.delayOpenNotifications is the flag to control the behavior.

Please note that the way how the client and server exports their API has changed. It is now based on the exports property in the package.json to allow further move to new features. See the Readme for details.

[DanTup]

@dbaeumer I tested this out and it seems to work as described. If I hold ctrl and hover over a bunch of symbols without clicking, it prevents the open/closed events (which also avoids the Dart server doing some analysis and producing some notifications we normally want for open files). Thanks!

Out of interest, do you have some approx target for when this client/next LSP will ship? (I'm not in any rush, it's just useful for my planning).

[dbaeumer]

After summer vacation :-) (August or September)

[DanTup]

Great, thanks! :-)