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

Lsp/inlay hints on pipe #3290

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

ascandone
Copy link
Contributor

@ascandone ascandone commented Jun 18, 2024

Implements #3291

screen

This PR shows inlay hints with the type of a pipeline step (only for pipeline expressions that span across multiple lines). Hints won't be shown for literal values (ints, floats, strings, bitstrings
The functionality is toggled via the gleam.inlayHints.pipelines client config option

The PR is meant to be read as a whole (not commit by commit - however the config functionality is implemented in a single commit: 9e12629)

Here's the vscode client PR: gleam-lang/vscode-gleam#82

How to activate hints

Vscode

Once the vscode client pr is merged, it will be possible to toggle the option via the vscode settings

Sublime text

Screenshot 2024-07-04 at 09 25 56

The user will need to enable the functionality in the settings of the LSP sublime extension:

{
  "clients": {
    "gleam": {
      "settings": {
        "gleam.inlayHints.pipelines": true
      }
    }
  },
}

Nvim

image

The user will need to enable the functionality in the init.lua file:

require'lspconfig'.gleam.setup{
  settings = {
    gleam = {
      inlayHints = {
        pipelines = true,
      },
    },
  },
}

Future development

  • handle hover on inlay hints (e.g. show type doc)
  • handle click on inlay hints (e.g. goto definition)

Credits

The part of the pr about config is partially based upon this previous pr #2393

@ascandone
Copy link
Contributor Author

ascandone commented Jun 21, 2024

Hi @lpil the PR is almost ready for review.
Is it ok for it to implement the feature without including a switch in the config (for now)?

If not, could we consider merging the PR (once it's reviewed) as dead code? (e.g. setting the inlay_hint_provider option as false in the capabilities config)
This would decrease the risk of having conflicts while I try to implement the config part (that would also involve a PR on the client)

@lpil
Copy link
Member

lpil commented Jun 23, 2024

Is it ok for it to implement the feature without including a switch in the config (for now)?

How would the programmer turn it on in this case? I think they need to be able to turn it on and add as needed in a straightforward fashion.

@ascandone
Copy link
Contributor Author

ascandone commented Jun 23, 2024

Is it ok for it to implement the feature without including a switch in the config (for now)?

How would the programmer turn it on in this case? I think they need to be able to turn it on and add as needed in a straightforward fashion.

It wouldn't be possible. What I was proposing was either:

  1. To just implement the feature without allowing to turn it off (for now)
  2. To merge the feature as dead code, and then right after try to implement another PR to create the option in the config. This would just reduce the odds of having git conflicts in case I take too much time implementing the config part

Otherwise I can just try to implement the config part in this PR

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 2 times, most recently from 697960d to 2d52e9e Compare June 25, 2024 01:52
@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 4 times, most recently from 9ed02ef to 7894a79 Compare June 25, 2024 12:28
@ascandone ascandone marked this pull request as ready for review June 25, 2024 12:33
@ascandone
Copy link
Contributor Author

@lpil I think that should be it 🙂
(sorry for the ping, I didn't know if marking the PR as ready already sends you a notification)

@ascandone
Copy link
Contributor Author

ascandone commented Jul 5, 2024

@lpil I've updated the PR with the description of how to turn the feature on. In general, it requires setting to true the gleam.inlayHints.pipelines client config option, but every editor has it's way to do that

Depending on the editor, there could be ways to turn it on momentarily (e.g. with a keyboard shortcut)
E.g. this (untested) config (took from here)

if client and client.server_capabilities.inlayHintProvider and vim.lsp.inlay_hint then
    map('<leader>th', function()
        vim.lsp.inlay_hint.enable(not vim.lsp.inlay_hint.is_enabled())
    end, '[T]oggle Inlay [H]ints')
end

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Great stuff! Thank you very much for waiting too, I've nearly got through all the compiler PRs 😅

compiler-core/src/ast/inlay_hints.rs Outdated Show resolved Hide resolved
compiler-core/src/ast/inlay_hints.rs Outdated Show resolved Hide resolved
compiler-core/src/ast/tests/inlay_hints.rs Outdated Show resolved Hide resolved
compiler-core/src/language_server/configuration.rs Outdated Show resolved Hide resolved
compiler-core/src/language_server/messages.rs Outdated Show resolved Hide resolved

fn configuration_update_received(&mut self, result: serde_json::Value) -> Next {
let Some(first_el) = result.as_array().and_then(|a| a.first()) else {
return Next::MorePlease;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be an error?

}

fn response(&mut self, response: lsp_server::Response) -> Next {
if let Some(handler) = self.response_handlers.remove(&response.id) {
Copy link
Member

Choose a reason for hiding this comment

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

When would it be valid for there to be a response that we don't have a handler for? Or there being a handler but no result in the response?

compiler-core/src/language_server/tests/inlay_hints.rs Outdated Show resolved Hide resolved
@lpil lpil marked this pull request as draft July 13, 2024 17:32
@ascandone
Copy link
Contributor Author

Sorry for keeping it open for a long time; I had less time recently but I might able to make it to the finish line in the following days

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 3 times, most recently from e6b80fd to 9824c74 Compare July 30, 2024 20:57
@lpil
Copy link
Member

lpil commented Aug 20, 2024

Hello! Sorry, just catch up after my holiday. Is this ready for review?

@ascandone
Copy link
Contributor Author

Hello! Sorry, just catch up after my holiday. Is this ready for review?

Hi Louis, sorry about the wait - I have been a little busy too. There are still a couple of things missing but I should be able to take care of them this week

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 2 times, most recently from db11626 to f14e019 Compare September 8, 2024 12:29
@ascandone
Copy link
Contributor Author

ascandone commented Sep 8, 2024

Hey @lpil everything should be good to go, except for this couple of bits where I am not sure how to handle error within MessageBuffer handlers

fn configuration_update_received(&mut self, result: serde_json::Value) -> Next {
let parsed_update_items: Result<(Configuration,), _> = serde_json::from_value(result);
let Ok((parsed_config,)) = parsed_update_items else {
return Next::MorePlease;
};

fn response(&mut self, response: lsp_server::Response) -> Next {
if let Some(handler) = self.response_handlers.remove(&response.id) {
if let Some(result) = response.result {
return self.handle_response(handler, result);
}
}
// We do not use or expect responses from the client currently.
Next::MorePlease
}

Should I add a new variant to the Next enum? Or return Next::Stop ?

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.

4 participants