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

feat(lsp): commands #1304

Merged
merged 4 commits into from
Dec 21, 2021
Merged

feat(lsp): commands #1304

merged 4 commits into from
Dec 21, 2021

Conversation

matoous
Copy link
Contributor

@matoous matoous commented Dec 19, 2021

This is work in progress PR that implements LSP commands. Usable for example for code actions and code lens.

Fixes: #1299
Related: #183

Comment on lines 3293 to 3295
tokio::spawn(async move {
language_server.command(command).await
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem you're seeing is because you're trying to move language_server into the async closure.

This should work:

Suggested change
tokio::spawn(async move {
language_server.command(command).await
});
let future = language_server.command(command);
tokio::spawn(async move {
future.await
// TODO: log the result here
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried that before, this moves the lifetime issue to

error[E0621]: explicit lifetime required in the type of `cx`
    --> helix-term/src/commands.rs:3268:30
     |
3268 |                 let picker = Picker::new(
     |                              ^^^^^^^^^^^ lifetime `'static` required

Copy link
Member

@archseer archseer Dec 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, you're also reusing the language_server from outside the cx.callback, and the compiler can't know how long the callback will live for. So it expects language_server -> doc -> cx to live for static.

To avoid this you need to re-fetch the doc/language_server inside the callback, easiest way to do that is let (doc, _view) = current!(editor), the most correct would be to let doc_id = doc.id() before cx.callback, then re-fetch that exact ID in the closure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the help! Fixed now. I also extracted the command execution into separate function so it can be re-used so it can be re-used between commands, code actions, and possibly code lenses in the future.

@@ -188,11 +188,11 @@ impl Client {
}

/// Reply to a language server RPC call.
pub fn reply(
pub fn reply<R>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a reason the method did not use generics: due to monomorphization it can lead to code bloat since the method is big. The way to fix this would be to rename the old function as reply_impl, then define a new reply function that uses generics and calls reply_impl.

See the example here under "inlining in practice" (under "fifth,") https://matklad.github.io/2021/07/09/inline-in-rust.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, didn't realize the implications, thanks a lot. I just reverted the changes for now. If it isn't good enough I can introduce reply and reply_impl.

@matoous matoous changed the title WIP: feat(lsp): commands feat(lsp): commands Dec 20, 2021
@matoous matoous marked this pull request as ready for review December 20, 2021 09:13
@matoous
Copy link
Contributor Author

matoous commented Dec 20, 2021

@archseer sorry, forgot to run clippy, the lint issue should be fixed now.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@archseer archseer merged commit 75a8b78 into helix-editor:master Dec 21, 2021
@matoous matoous deleted the md/codeAction-command branch December 21, 2021 09:36
@matoous matoous mentioned this pull request Dec 28, 2021
4 tasks
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.

LSP Fill struct code action doesn't work (golang)
2 participants