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

Handle textDocument/didChange the same way as textDocument/didSave #6

Closed
mfelsche opened this issue Jun 30, 2024 · 6 comments · Fixed by #7
Closed

Handle textDocument/didChange the same way as textDocument/didSave #6

mfelsche opened this issue Jun 30, 2024 · 6 comments · Fixed by #7

Comments

@mfelsche
Copy link
Contributor

The neovim lsp client only sends textDocument/didChange notifications, no textDocument/didSave notifications.
Either we need to configure the neovim lsp client to do this or handle the textDocument/didChange the same way as we currently do textDocument/didSave and hope for the best that no other lsp client issues both shortly after one another.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jun 30, 2024
@mfelsche
Copy link
Contributor Author

mfelsche commented Jul 1, 2024

Unfortunately vscode is issueing a textDocument/didChange on every change of the buffer (without saving). So a reasonable method is to differentiate the server behaviour based upon the client which is connected here.

@jasoncarr0
Copy link

Perhaps a solution is debouncing? As in, wait for a time interval whenever a change occurs, and see if it stops changing during that interval. Alternatively, if it's cheap to do so, just run the server whenever a change occurs, and then check at completion if it still reflects the input.

@jemc jemc removed the discuss during sync Should be discussed during an upcoming sync label Jul 9, 2024
@mfelsche
Copy link
Contributor Author

Debouncing would be an option, although i think it would increase latency from a possible change to having these changes reflected in answers from the language server (such as diagnostics).

The problem with the didChange notifications from vscode is that the files didn't change on disk and this is what libponyc consumes. We could do a filesystem check and check the mtime of all currently open files for changes since the last compilation. We could also compute hashes for all open files. And only if we have a change visible in the filesystem the language server would start another compilation.

Another approach would be to behave slightly differently based on what client is currently connected. We need to store client information and capabilities anyways, so this should be available in the language server.

I would tend to the first approach, as adding complex logic via dispatching on connected clients makes everything more complex than it might need to be. And we can still go down that road if we detect more changes that cannot be handled with a more generic approach like watching for file system changes.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 18, 2024
@jemc
Copy link
Member

jemc commented Jul 23, 2024

Not necessarily that helpful, but I'll just mention that the last time I worked on a language server I was baking it into the compiler, so I was able to have special case logic for "in-memory overrides" of any particular file path.

Not sure if there's a likely path to adding a feature to Ponyc to add that kind of in-memory override feature.

@jemc
Copy link
Member

jemc commented Jul 23, 2024

Gordon also mentioned in the sync call the inefficient/hacky, but non-invasive approach of creating temporary files on disk that match the active buffers in the editor.

It's not that efficient, but it would get the job done.

@jemc jemc removed the discuss during sync Should be discussed during an upcoming sync label Jul 23, 2024
@mfelsche
Copy link
Contributor Author

Thanks for your input. It turned out, I didn't configure the server capabilities correctly. The server advertises to the client what it wants to receive, especially the didSave notification needs to be configured explicitly. See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didSave for the textDocumentSync.save property and https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_synchronization_sc for the textDocumentSync property.

So we are actually good, and everything is working as expected with the decently recent neovim.

@ponylang-main ponylang-main added discuss during sync Should be discussed during an upcoming sync and removed discuss during sync Should be discussed during an upcoming sync labels Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants