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

Fix panic when using set-language on a scratch buffer #1996

Merged

Conversation

zen3ger
Copy link
Contributor

@zen3ger zen3ger commented Apr 6, 2022

Skip launching a language server if a document doesn't have a valid URL.

Closes #1994

@kirawi
Copy link
Member

kirawi commented Apr 7, 2022

Would this cause any problems with the LSP?

@zen3ger
Copy link
Contributor Author

zen3ger commented Apr 7, 2022

Yeah, you are right. It seems to require that the URI is a valid file path, which is a bit.. not good... Also the scatch buffer URIs have to be unique otherwise it seems to complain as well and get super confused about edits.

I've tried what happens if I generate a file URI as file://<current working dir>/scratch<document id> which seems to work and not cause any issues with the LSPs (tried rust-analyzer, clangd and erlang_ls).

Edit:
No, that's not entirely correct either. If you save a scratch buffer and the detected language ends up being the same as it the one set for the scratch buffer, then we won't close the dummy LSP session and won't start a new client for it, but the URI will no longer match the scratch one...

@zen3ger zen3ger force-pushed the 1994-set-language-in-scratch-buffer branch from 762fc50 to b615c88 Compare April 7, 2022 20:27
@zen3ger
Copy link
Contributor Author

zen3ger commented Apr 7, 2022

Alright, I think this covers everything. I found no protocol errors and the scratch connection is cleaned up.

2022-04-07T22:13:43.321 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"languageId":"rust","text":"\n","uri":"file:///usr/home/zen3ger/project/helix/scratch1","version":0}}}
.
.
.
2022-04-07T22:14:06.535 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":12,"line":1},"start":{"character":12,"line":1}},"text":";"}],"textDocument":{"uri":"file:///usr/home/zen3ger/project/helix/scratch1","version":25}}}
2022-04-07T22:14:20.113 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didClose","params":{"textDocument":{"uri":"file:///usr/home/zen3ger/project/helix/scratch1"}}}
2022-04-07T22:14:20.168 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"languageId":"c","text":"int main(void) {\n    return 0;\n}\n","uri":"file:///usr/home/zen3ger/project/helix/nothing.c","version":25}}}

@archseer
Copy link
Member

archseer commented Apr 8, 2022

Should we set language servers for scratch buffers or just ignore them instead until they get saved with a path?

How does vscode or neovim handle such cases? I don't think we should use fake filenames.

@smason
Copy link

smason commented Apr 8, 2022

Should we set language servers for scratch buffers or just ignore them instead until they get saved with a path?

ignoring until they get saved sounds better to me; the reason I tried this was to get syntax highlighting/tree-sitter and these would presumably still function without a language server

further, the scratch buffer might end up somewhere arbitrary, or only loosely related to the current session

@zen3ger
Copy link
Contributor Author

zen3ger commented Apr 8, 2022

Should we set language servers for scratch buffers or just ignore them instead until they get saved with a path?

Why didn't I think about the obvious solution...

How does vscode or neovim handle such cases?

If I'm not mistaken in neovim LSP is not started when you do syntax=<lang> or filetype to enable the highlighters unless you manually added a hook to do so.

@archseer
Copy link
Member

Let's ignore scratch buffers then. refresh_language_server should return if the doc doesn't have a doc.url()

Skip launching a language server if a document doesn't have a valid URL.
@zen3ger zen3ger force-pushed the 1994-set-language-in-scratch-buffer branch from b615c88 to 888ebeb Compare April 12, 2022 16:41
@archseer archseer merged commit a0c6c45 into helix-editor:master Apr 13, 2022
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 this pull request may close these issues.

Setting language in scratch buffer causes panic
4 participants