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)!: vim.lsp.inlay_hint.get() #25512

Merged
merged 1 commit into from Nov 12, 2023
Merged

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Oct 5, 2023

resolves #25069

refactor!: vim.lsp.inlay_hint() -> vim.lsp.inlay_hint.enable()

Problem

The LSP specification allows inlay hints to include tooltips, clickable label parts, and code actions; but Neovim provides no API to query for these.

Solution

Add minimal viable extension point from which plugins can query for inlay hints in a range, in order to build functionality on top of.

Possible Next Steps

  • Add virt_text_idx field to vim.fn.getmousepos() return value, for usage in mappings of <LeftMouse>, <C-LeftMouse>, etc

@github-actions github-actions bot added the lsp label Oct 5, 2023
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

Here's the sample plugin using this API: https://github.com/llllvvuu/interactive-inlay.nvim

I looked at how others are doing interactive inlay hints, and noticed that @MariaSolOs authored the tsserver stuff so probably has smart things to say (and probably knows more about our conventions on the LSP side, which I know nothing about)

@gpanders
Copy link
Member

gpanders commented Oct 5, 2023

cc @mfussenegger @p00f

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

Dumping some more notes:

Edge cases where inlay hint tooltip/click is more efficient than regular hover / go to type definition:

  • Variable type inlay hint where type has a type inside of it
  • Return type inlay hint

In theory, language servers could include these info/prompts in regular hover docs too. lua_ls for example has typedoc in variable hover doc, but not in function hover doc

The efficiency difference is probably smaller in Neovim because everything is fast in Neovim.

@p00f
Copy link
Contributor

p00f commented Oct 5, 2023

I think clickable should just be clickable, instead of "querying" a hint

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

I think clickable should just be clickable, instead of "querying" a hint

For TUI, mouse click/hover may not be feasible, in which case the question one has to figure out is how to "DWIM" based on a key press. Range query would be the most general tool that plugins could use to do that.

(also users might just prefer to use keyboard)

How hard would it be to make virtual text clickable? Like setting the "cursor: pointer" and everything?

@gpanders
Copy link
Member

gpanders commented Oct 5, 2023

How hard would it be to make virtual text clickable? Like setting the "cursor: pointer" and everything?

OSC 8 is what you need to communicate "clickability" to the terminal, but even that is just for hyperlinks. I don't think it's possible to make an OSC 8 link do something like write text back to the running application.

There might be some way to do something hacky here. Maybe using an empty "link" so that the terminal still gives you the pointer cursor, but the click event still goes through to Nvim. No idea if that'd work, I'm just brainstorming.

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

Maybe using an empty "link" so that the terminal still gives you the pointer cursor, but the click event still goes through to Nvim. No idea if that'd work, I'm just brainstorming.

Very interesting, seems worth looking into.

For handling the click event itself, I played around with vim.fn.getmousepos() just now. I don't think any of the fields it returns are enough to figure out which label was clicked; perhaps a virt_text_idx field would do the trick?

Then on the plugin side, I could add mappings for <LeftMouse>, <C-LeftMouse>, etc which get the vim.lsp.buf.inlay_hints() at the mouse { row, col } and finds the label via virt_text_idx

EDIT: The mouse handler does need to be Neovim core if the clickability indicator is in core (which it has to be, I think)

TODO: I haven't thought through how this all plays with GUI frontends like Neovide

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

Some research on mouse hover: #9534

@mfussenegger
Copy link
Member

mfussenegger commented Oct 5, 2023

regarding api/location

Part of the motivation for a dedicated module for inlay_hints was that it would allow us to add new methods to it. So big -1 for now adding a buf.inlay_hints that is ending up calling a different method on inlay_hints.

This should be a vim.lsp.inlay_hints.get(opts) if we want to expose the data. Where opts provides various filters. Similar to vim.diagnostic.get() or vim.lsp.codelens.get().

regarding resolve

I think ideally we'd handle this transparently, without users of higher level APIs knowing anything about it.


The LSP specification allows inlay hints to include tooltips, clickable label parts, and code actions;

They do not have code actions, but can contain textEdits that one can apply (I think the intention here is to persist the inlay hint into actual text).

This could be exposed via a inlay_hints.apply() or inlay_hints.run() or something like that.

The label can also contain a command. I have no idea what the intention here is. Do language servers use this? If so, what for?


I wonder if neovim should have a more generic tooltip mechanism?

@llllvvuu llllvvuu changed the title feat(lsp): vim.lsp.buf.inlay_hints() feat(lsp): vim.lsp.inlay_hint.get() Oct 5, 2023
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

This should be a vim.lsp.inlay_hints.get(opts) if we want to expose the data. Where opts provides various filters. Similar to vim.diagnostic.get() or vim.lsp.codelens.get().

Thanks, that's exactly the kind of advice I'm looking for regarding the conventions (I'm not familiar with what's the "old way" vs "new way"). GitHub is being slow to update PRs, but I just updated the branch to make it this way.

The only awkward thing is that __call does not show up in generated vimdoc. It works perfectly in LSP hover docs though.

This could be exposed via a inlay_hints.apply() or inlay_hints.run() or something like that.

I wanted to keep initial drafts conservative by only doing trivial low-level stuff first, but if y'all are open to it we can also add these additional utilities as a next step.

I wonder if neovim should have a more generic tooltip mechanism?

Related to your comment on Matrix about vim.ui.config?

I was going to try using open_floating_preview for my plugin, will report back on how well it works.

@llllvvuu llllvvuu force-pushed the feat/inlay_hints branch 5 times, most recently from 032ab9d to 2d0fcf7 Compare October 5, 2023 15:43
@mfussenegger
Copy link
Member

I wanted to keep initial drafts conservative by only doing trivial low-level stuff first, but if y'all are open to it we can also add these additional utilities as a next step.

Good call 👍

I didn't mean to imply that we should do that now/at the same time. Just as a rough outline of what we could do later.

I wonder if neovim should have a more generic tooltip mechanism?

Related to your comment on Matrix about vim.ui.config?

I didn't have anything concrete in mind - that was just thinking out loud.

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

The only awkward thing is that __call does not show up in generated vimdoc. It works perfectly in LSP hover docs though.

We'll need some way to bring back the vim.lsp.inlay_hint() docs, but no idea how.

runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/inlay_hint.lua Outdated Show resolved Hide resolved
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 5, 2023

Just as a rough outline of what we could do later.

Indeed, now that I think about it, it does seem correct to have Neovim handle the text edits / commands part given that we already have TextEdit, Command, etc logic

So the only complexity outsourced to plugins would be triggering the inlay hint w/o mouse

@MariaSolOs
Copy link
Member

Indeed, now that I think about it, it does seem correct to have Neovim handle the text edits / commands part given that we already have TextEdit, Command, etc logic

@llllvvuu Might not be relevant but do note that when using inlay hints as a "go to definition", there could be multiple definitions and so one needs to consider how to handle that. In VS Code for example, when control-clicking on an inlay hints that has multiple definitions, the "Peek definition" hover window is displayed to show all the results. In Neovim this could mean hooking up to vim.ui.select.

Just a heads-up because this might not be immediately obvious from the specification of label parts.

@llllvvuu llllvvuu force-pushed the feat/inlay_hints branch 3 times, most recently from 848f826 to d5dd4ed Compare October 6, 2023 06:49
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Oct 6, 2023

I figured out a janky way to keep the vimdoc intact for the __call. Just updated the PR.

Might not be relevant but do note that when using inlay hints as a "go to definition", there could be multiple definitions

It's very relevant! Currently I'm tracking all the UI stuff in my plugin roadmap, and starting with headless stuff here. If the UI stuff gets good reception downstream then I'll start proposing to upstream.

vim.ui.select is a great suggestion, if we want to put some UI in core that would be the way probably. I didn't realize it because I use Telescope for everything (and would probably use Telescope for the plugin too), but it looks like Neovim even includes a UI for references using loclist.

So maybe core UI (if it exists) can be loclist for locations + vim.ui.select for commands

I think ideally we'd handle this transparently, without users of higher level APIs knowing anything about it.

Thinking about this a bit. If we want to do resolution instead of the user doing it, we need to figure out what the right trigger for it is. Do we automatically resolve everything that we return from vim.lsp.inlay_hint.get()?

In any case, it looks like manual resolving is already possible without any extra work from our part (by calling client.request directly). Yay!

@llllvvuu llllvvuu force-pushed the feat/inlay_hints branch 2 times, most recently from 8c04a5c to ac8bd8d Compare October 6, 2023 09:05
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Nov 3, 2023

Sorry for the delay (haven't had Wi-fi for a while), made changes:

  • docs changes
  • replace toggle() with enable() and is_enabled()
  • remove client_name, client_id, and client filters

@llllvvuu llllvvuu force-pushed the feat/inlay_hints branch 2 times, most recently from b876959 to 7a5327d Compare November 3, 2023 03:15
Enable/disable/toggle inlay hints for a buffer

Parameters: ~
• {bufnr} (integer|nil) Buffer handle, or 0 or nil for current
Copy link
Member

Choose a reason for hiding this comment

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

Typo? Isn't nil for using all buffers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For .get() yes, but .enable(), no

runtime/doc/lsp.txt Show resolved Hide resolved
refactor!: `vim.lsp.inlay_hint()` -> `vim.lsp.inlay_hint.enable()`

Problem
---
The LSP specification allows inlay hints to include tooltips, clickable
label parts, and code actions; but Neovim provides no API to query for
these.

Solution
---
Add minimal viable extension point from which plugins can query for
inlay hints in a range, in order to build functionality on top of.

Possible Next Steps
---

- Add `virt_text_idx` field to `vim.fn.getmousepos()` return value, for
  usage in mappings of `<LeftMouse>`, `<C-LeftMouse>`, etc
@justinmk justinmk merged commit 448907f into neovim:master Nov 12, 2023
24 checks passed
accidentaldevelopment added a commit to accidentaldevelopment/dotfiles that referenced this pull request Nov 13, 2023
Neovim merged [a breaking PR][1] that modified `vim.lsp.inlay_hints`
into a module with functions. This required a config update to fix inlay
hints.

[1]: neovim/neovim#25512
accidentaldevelopment added a commit to accidentaldevelopment/dotfiles that referenced this pull request Nov 13, 2023
Neovim merged [a breaking PR][1] that modified `vim.lsp.inlay_hints`
Neovim merged [a breaking PR](neovim/neovim#25512) that modified `vim.lsp.inlay_hints`
into a module with functions. This required a config update to fix inlay
hints.

[1]: neovim/neovim#25512
accidentaldevelopment added a commit to accidentaldevelopment/dotfiles that referenced this pull request Nov 13, 2023
Neovim merged [a breaking PR][1] that modified `vim.lsp.inlay_hints`
into a module with functions. This required a config update to fix inlay
hints.

[1]: neovim/neovim#25512
accidentaldevelopment added a commit to accidentaldevelopment/dotfiles that referenced this pull request Nov 13, 2023
Neovim merged [a breaking PR][1] that modified `vim.lsp.inlay_hints`
into a module with functions. This required a config update to fix inlay
hints.

[1]: neovim/neovim#25512
ray-x added a commit to ray-x/go.nvim that referenced this pull request Nov 14, 2023
perrin4869 added a commit to perrin4869/dotfiles that referenced this pull request Nov 21, 2023
dundargoc pushed a commit to dundargoc/neovim that referenced this pull request Nov 27, 2023
dundargoc pushed a commit to dundargoc/neovim that referenced this pull request Nov 27, 2023
dundargoc pushed a commit to dundargoc/neovim that referenced this pull request Nov 27, 2023
ueaner added a commit to ueaner/nvimrc that referenced this pull request Dec 1, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Apr 7, 2024
Problem:
`vim.diagnostic.is_disabled` and `vim.diagnostic.disable` are unnecessary
and inconsistent with the "toggle" pattern (established starting with
`vim.lsp.inlay_hint`, see neovim#25512 (review)

As a reminder, the rationale is:
- we always need `enable()`
- we always end up needing `is_enabled()`
- "toggle" can be achieved via `enable(not is_enabled())`
- therefore,
    - `toggle()` and `disable()` are redundant
    - `is_disabled()` is a needless inconsistency

Solution:
- Introduce `vim.diagnostic.is_enabled`, and `vim.diagnostic.enable(…, enable:boolean)`
    - Note: Future improvement would be to add an `enable()` overload `enable(enable:boolean, opts: table)`.
- Deprecate `vim.diagnostic.is_disabled`, `vim.diagnostic.disable`
justinmk added a commit to justinmk/neovim that referenced this pull request Apr 7, 2024
Problem:
`vim.diagnostic.is_disabled` and `vim.diagnostic.disable` are unnecessary
and inconsistent with the "toggle" pattern (established starting with
`vim.lsp.inlay_hint`, see neovim#25512 (review)

As a reminder, the rationale is:
- we always need `enable()`
- we always end up needing `is_enabled()`
- "toggle" can be achieved via `enable(not is_enabled())`
- therefore,
    - `toggle()` and `disable()` are redundant
    - `is_disabled()` is a needless inconsistency

Solution:
- Introduce `vim.diagnostic.is_enabled`, and `vim.diagnostic.enable(…, enable:boolean)`
    - Note: Future improvement would be to add an `enable()` overload `enable(enable:boolean, opts: table)`.
- Deprecate `vim.diagnostic.is_disabled`, `vim.diagnostic.disable`
justinmk added a commit to justinmk/neovim that referenced this pull request Apr 7, 2024
Problem:
`vim.diagnostic.is_disabled` and `vim.diagnostic.disable` are unnecessary
and inconsistent with the "toggle" pattern (established starting with
`vim.lsp.inlay_hint`, see neovim#25512 (review)

As a reminder, the rationale is:
- we always need `enable()`
- we always end up needing `is_enabled()`
- "toggle" can be achieved via `enable(not is_enabled())`
- therefore,
    - `toggle()` and `disable()` are redundant
    - `is_disabled()` is a needless inconsistency

Solution:
- Introduce `vim.diagnostic.is_enabled`, and `vim.diagnostic.enable(…, enable:boolean)`
    - Note: Future improvement would be to add an `enable()` overload `enable(enable:boolean, opts: table)`.
- Deprecate `vim.diagnostic.is_disabled`, `vim.diagnostic.disable`
justinmk added a commit to justinmk/neovim that referenced this pull request Apr 8, 2024
Problem:
`vim.diagnostic.is_disabled` and `vim.diagnostic.disable` are unnecessary
and inconsistent with the "toggle" pattern (established starting with
`vim.lsp.inlay_hint`, see neovim#25512 (review)

As a reminder, the rationale is:
- we always need `enable()`
- we always end up needing `is_enabled()`
- "toggle" can be achieved via `enable(not is_enabled())`
- therefore,
    - `toggle()` and `disable()` are redundant
    - `is_disabled()` is a needless inconsistency

Solution:
- Introduce `vim.diagnostic.is_enabled`, and `vim.diagnostic.enable(…, enable:boolean)`
    - Note: Future improvement would be to add an `enable()` overload `enable(enable:boolean, opts: table)`.
- Deprecate `vim.diagnostic.is_disabled`, `vim.diagnostic.disable`
justinmk added a commit to justinmk/neovim that referenced this pull request Apr 15, 2024
Problem:
`vim.diagnostic.is_disabled` and `vim.diagnostic.disable` are unnecessary
and inconsistent with the "toggle" pattern (established starting with
`vim.lsp.inlay_hint`, see neovim#25512 (review)

As a reminder, the rationale is:
- we always need `enable()`
- we always end up needing `is_enabled()`
- "toggle" can be achieved via `enable(not is_enabled())`
- therefore,
    - `toggle()` and `disable()` are redundant
    - `is_disabled()` is a needless inconsistency

Solution:
- Introduce `vim.diagnostic.is_enabled`, and `vim.diagnostic.enable(…, enable:boolean)`
    - Note: Future improvement would be to add an `enable()` overload `enable(enable:boolean, opts: table)`.
- Deprecate `vim.diagnostic.is_disabled`, `vim.diagnostic.disable`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature(lsp): interactive inlay hints (tooltip, label[].location, textEdits)
10 participants