-
Notifications
You must be signed in to change notification settings - Fork 200
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
LSP: Support signature help #1585
Labels
Comments
github-merge-queue bot
pushed a commit
that referenced
this issue
Aug 15, 2024
# Description ## Problem Resolves #1585 ## Summary While working on autocompletion I was always checking what Rust Analyzer did, or how it worked, and I noticed that in Rust Analyzer when it autocompletes a function a little popup with the parameter names and types would show up, which is really useful! But it turned out that's a separate feature: signature help. This PR implements that. I think it makes autocompletion much better. Before: ![lsp-signature-help-before](https://github.com/user-attachments/assets/2480e9bf-ae17-47a6-be0a-bcc58f2b2b92) After: ![lsp-signature-help-after](https://github.com/user-attachments/assets/e50573d0-17cb-4c3a-b50b-a4fed462acdf) Note: in the latest version the popup text starts with "fn ", but I didn't want to re-record all the gifs 😅 . You basically don't have to remember what were the parameter types. It also works with methods: ![lsp-signature-help-method](https://github.com/user-attachments/assets/72fe65ee-2c68-4463-aa75-fd304fcbe50c) And if you have an `fn`: ![lsp-signature-help-fn](https://github.com/user-attachments/assets/d29001e1-5de4-47f1-aede-da01bc1a3c53) And here it's working in an aztec project (I always check that it works outside of small toy programs 😊): ![lsp-signature-help-aztec](https://github.com/user-attachments/assets/3aa1d395-09bc-4578-b56d-c0e90630c4da) ## Additional Context I promise this will be the last big LSP PR I send 🤞 . Now that we have most of the LSP features registered, next PRs should be much smaller, just adding on top of what there is. These PRs are also relatively big because they traverse the AST and that code is kind of boilerplatey. For that reason here I decided to put the boilerplatey code in a `traversal.rs` file, so that it's clear there's no extra logic there other than traversing the AST. I might send a follow-up PR doing the same for autocompletion. One more thing: you can trigger this functionality as a VSCode command (Trigger Parameter Hints). But then when offering autocompletion you can also tell VSCode to execute a command, and in this PR we tell it to execute exactly that command (Rust Analyzer does the same, though they have a custom command for it, not sure why). UPDATE: I managed to implement the visitor pattern to reduce the boilerplate. [Here's a PR](#5727) with that. I think it will greatly simplify things, and make it easier to keep the code updated as we add more nodes. It takes inspiration in [how it's done in the Crystal compiler](https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/syntax/visitor.cr), which in turn is inspired by how it was done in the Java editor support for Eclipse 😄 ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher <jake@aztecprotocol.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
The LSP should support signature help following https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp
Happy Case
I'm not exactly sure what "Signature Help" is, so we need to do some more research on the outcome of supporting this from the user perspective.
Alternatives Considered
No response
Additional Context
No response
Would you like to submit a PR for this Issue?
No
Support Needs
No response
The text was updated successfully, but these errors were encountered: