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

"Find References" triggers textDocument/didOpen but no textDocument/didClose, even after opening and closing the file #205790

Closed
KevinEady opened this issue Feb 21, 2024 · 13 comments
Assignees
Labels
*as-designed Described behavior is as designed

Comments

@KevinEady
Copy link

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.86.2 (Universal)
  • OS Version: macOS 14.0 (23A344)

Steps to Reproduce:

  1. Use "Find references" that returns references that will publish diagnostics from textDocument/didOpen.
  2. Open a file from the References pane that has published diagnostics.
  3. Close the file, and the diagnostics for that file are still in the Problems pane.
  4. Remove the result from the References pane, and the diagnostics for that file are still in the Problems pane.

Sample repository with a minimal reproduction and additional steps: https://github.com/KevinEady/vscode-bug-no-didClose

vscode-bug-no-didClose

The extension clears published diagnostics in the textDocument/didClose handler (as seen when file1.txt closes), see here. Since the diagnostics for the files in the References pane never clears, I can only assume there is no textDocument/didClose event triggered for these files. Enabling the LSP debug logs via "escript.trace.server": "verbose" confirms this assumption.

It seems impossible to remove these diagnostics, as the textDocument/didClose never triggers, even after opening the file.

@jrieken jrieken added the *as-designed Described behavior is as designed label Feb 21, 2024
@VSCodeTriageBot
Copy link
Collaborator

The described behavior is how it is expected to work. If you disagree, please explain what is expected and what is not in more detail. See also our issue reporting guidelines.

Happy Coding!

@VSCodeTriageBot VSCodeTriageBot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2024
@jrieken
Copy link
Member

jrieken commented Feb 21, 2024

Please use vscode.window.tabGrous to know when editor tabs change. Tabs are a view-concept and they don't correlate with document lifecycles. The editor decides when documents are closed

@KevinEady
Copy link
Author

Hi @jrieken , can you provide a little more context to how this would be done? This vscode.window.tabGroups seems like something that needs to be done within the "client extension" part itself, and not the Language Server. How can I clear diagnostics from within the client extension itself? Do I need to listen to the "tab closed event" inside the client extension, then send a notification to the LSP server, then have the LSP server respond with clearing the diagnostics?

@KevinEady
Copy link
Author

Additionally, which editor event can I use to listen to the "Reference Cleared" event, ie. when the user clicks any of these buttons:

Screenshot 2024-02-21 at 13 47 32 Screenshot 2024-02-21 at 13 48 21

I will need to clear the diagnostics for these files when the reference is removed, since the user may not open a tab for the corresponding file and the diagnostics would stay in the Problems pane indefinitely.

@jrieken
Copy link
Member

jrieken commented Feb 21, 2024

Do I need to listen to the "tab closed event" inside the client extension, then send a notification to the LSP server, then have the LSP server respond with clearing the diagnostics?

I am no LSP expert, but likely "yes"

@jrieken
Copy link
Member

jrieken commented Feb 21, 2024

I will need to clear the diagnostics for these files when the reference is removed, since the user may not open a tab for the corresponding file and the diagnostics would stay in the Problems pane indefinitely.

There is no such event. I believe you don't want create diagnostics to begin with - only for tabs

@KevinEady
Copy link
Author

Thank you for your help... I will try that then.

FWIW, the Microsoft LSP sample sends diagnostics on textDocument/didOpen and textDocument/didChange: https://github.com/microsoft/vscode-extension-samples/blob/be3ef98e19fca4714a1d73fa5bd7181eb9d70354/lsp-sample/server/src/server.ts#L131-L135

It sounds like this implementation is incorrect then, at the very least misleading.

@jrieken
Copy link
Member

jrieken commented Feb 21, 2024

yes, looks like it didn't get updated when we added the tabsGroups-api.

fyi @dbaeumer

@KevinEady
Copy link
Author

Hi @jrieken ,

I finished my investigation and this tab group events do not seem to be the correct way:

    window.tabGroups.onDidChangeTabs(e => {
        const toLabels = (tabs: readonly Tab[]) => tabs.map(tab => tab.label);
        console.log(`onDidChangeTabs. Changed: ${toLabels(e.changed)} Opened: ${toLabels(e.opened)} Closed: ${toLabels(e.closed)}`);
    });
  1. The event is not fired on initial opening of the window. Naturally we want diagnostics for the documents that exist when the window opens.
  2. I'm not sure how to receive the full filesystem path (or URI) for the given tab? Tab.label only contains the file name, excluding the path (eg. file1.txt) This doesn't help as I would need the full path. I understand that not all tabs correspond to filesystem paths (eg. files that have not been saved yet), but my Language Server can only process existent files, so no need to worry about unsaved tabs.

And another thing. You mentioned:

Tabs are a view-concept and they don't correlate with document lifecycles. The editor decides when documents are closed

Can you tell me when the editor would decide to close a document that's been opened from the references pane (without the tab being opened, as shown in the original post's video above)? Maybe I'm missing something, but I can't really understand why the editor would consider the document open if there is no tab and the reference has been cleared from the Reference Pane. It seems the editor considers the document opened indefinitely, as opening a corresponding tab and closing the tab + clearing the references does not trigger textDocument/didClose, so ... the editor must have some reason to consider the document open, and I'm not sure what it is. If I can understand this, then I may have a different solution, which could just be as simple as some user interaction (eg. "click ").

Again, thank you for your assistance and quick response times. I greatly appreciate it!

@jrieken
Copy link
Member

jrieken commented Feb 21, 2024

I'm not sure how to receive the full filesystem path (or URI) for the given tab?

Use Tab.input

Can you tell me when the editor would decide to close a document that's been opened

No, just accept that editor decides but note that document != editor != tab

@KevinEady
Copy link
Author

KevinEady commented Feb 21, 2024

Thanks for the insight with Tab.input! After seven hours of back-and-forth various implementations, I think I have a good workaround, in case anyone else stumbles upon this issue:

  1. On server start (via client.onDidChangeState), as well as on tab change, send a list of open tabs to LSP server.
  2. In LSP, only send diagnostics for documents that have an open tab.
  3. On tab change inside LSP, clear diagnostics for closed tabs, and send diagnostics for new tabs.

I think this covers all my use-cases.

Thanks for all your assistance today 🙇

@dbaeumer
Copy link
Member

@KevinEady just see the issue now. Was out on vacation. Actually LSP has full support for diagnostics only for files that are open in tabs. Instead of letting the server push the diagnostics you can let the LSP client pull for diagnostics. The client will then do all the tab handling, editor focus switching, ... for you. For the documentation see: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics

One of the reasons to introduce the pull model was to avoid syncing the tab model to the server since it is nothing the server should be concern about.

@KevinEady
Copy link
Author

Thanks for the insight @dbaeumer ! I was able to successfully change my implementation, removing this "send open tabs" workaround and relying solely on the textDocument/diagnostic message 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*as-designed Described behavior is as designed
Projects
None yet
Development

No branches or pull requests

4 participants