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 12 commits into
base: main
Choose a base branch
from

Conversation

ascandone
Copy link
Contributor

@ascandone ascandone commented Jun 18, 2024

edit: in addition to the inlay hints on pipelines, this PR also implements inlay hints for functions' parameter types (via the gleam.inlayHints.parameterTypes option) and for functions' return type (via the gleam.inlayHints.returnTypes option)


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 or add annotations)
  • other possible inlay hints:
    • function calls args, e.g. my_fn(/*param_name*/0, /* other_param*/ 100)
    • local let bindings, e.g. let x/*: Int*/ = my_fn(). Same for use bindings
    • bindings of case patterns

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)

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch from 64afe96 to 9f46fff Compare June 22, 2024 09:06
@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 ascandone force-pushed the lsp/inlay-hints-on-pipe branch from fe20ce7 to 7894a79 Compare July 3, 2024 15:53
@ascandone ascandone requested a review from lpil July 5, 2024 13:15
@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 😅

}

#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
Copy link
Member

Choose a reason for hiding this comment

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

snake_case please, we never use camelCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: I noticed that for a better integration with vscode, it's better to use camelCase instead
you can take a look at how it is rendered in the client side pr: gleam-lang/vscode-gleam#82


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?

@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 ?

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch from e468029 to f0a6273 Compare January 14, 2025 14:58
@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 7 times, most recently from 12b04fb to 69b3aae Compare March 7, 2025 19:33
@ascandone ascandone marked this pull request as draft March 7, 2025 23:50
@ascandone ascandone closed this Mar 8, 2025
@ascandone ascandone deleted the lsp/inlay-hints-on-pipe branch March 8, 2025 00:10
@ascandone ascandone restored the lsp/inlay-hints-on-pipe branch March 8, 2025 00:11
@ascandone ascandone reopened this Mar 8, 2025
@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch 2 times, most recently from 57ebc57 to d53e58c Compare March 8, 2025 00:43
@giacomocavalieri
Copy link
Member

giacomocavalieri commented Mar 8, 2025

Hello! The CI deps validation problem should be solved on main, it should go away if you rebase on main :)

@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch from d53e58c to a658af1 Compare March 9, 2025 02:25
@ascandone ascandone force-pushed the lsp/inlay-hints-on-pipe branch from a658af1 to fad6307 Compare March 9, 2025 20:40
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.

6 participants