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

nvim use priorities to show the most relevant line #61

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

AIDIGIT
Copy link

@AIDIGIT AIDIGIT commented Feb 25, 2025

Always use the most relevant highlight group, i.e. if a token has both lifetime and imm_borrow highlights, use the latter, i.e. make it blue, not green.

Note:
I've tried using vim.api.nvim_buf_set_extmark but it didn't work properly (and even had some issue with lsp hints), but this simpler approach worked best.
I've started with priority 200 as the lowest because that's what we usually get from tresitter, but in this case any will work, only the ordering is important.

@cordx56
Copy link
Owner

cordx56 commented Mar 1, 2025

Those priority calculations are handled by the LSP server, but there may be some bugs.
https://github.com/cordx56/rustowl/blob/main/rustowl/src/bin/cargo-owlsp.rs#L312

If you encounter any issues related to underline priority, please open an issue.

@AIDIGIT
Copy link
Author

AIDIGIT commented Mar 1, 2025

But no priorities are being passed to neovim in vim.highlight.range, which actually makes the nvim highlight be non-deterministic, i.e. sometimes the underline will be green and sometimes blue for the same variable, if the var happens to have been annotated with both lifetime and imm_borrow highlights.

If we don't pass the priorities to nvim, the color of the last decoration used by vim.highlight.range will be the one displayed.

Maybe there is a way to get the priorities from the LSP on the nvim side and use those when doing the for deco loop, that would also work.

@cordx56
Copy link
Owner

cordx56 commented Mar 1, 2025

The priority is determined in the LSP server, and resolved decorations are returned.
This means editors do not need to calculate priority, allowing all editors to receive a consistent result.

So if you find any bugs related to priority, it is an issue with the LSP server.

@AIDIGIT
Copy link
Author

AIDIGIT commented Mar 1, 2025

I hope the screenshot makes the problem I am trying to describe easier to understand.
What you are saying is correct, in that the LSP computes/defines the decorations and their priorities.
But what I am saying is that the editor (nvim) receives ALL decorations and applies them in a for-loop using vim.highlight.range BUT IT DOES NOT SPECIFY the priority.

As per the neovim docs, vim.highlight.range() can receive a priority parameter.
That is how the LSP needs to communicate to the editor the priorities.

So this is how it works at the moment, i.e. wrong color ...
problem

... and this is how it should look, and what this PR does.
after


Also, looking at the deco vars in this for loop, they look like this, i.e. not including the priority.
If they did, we could use deco['priority'] inside vim.highlight.range(), instead of what I did in this PR, i.e. define priorities on the lua side.

  hover_text = "immutable borrow",
  is_display = true,
  ["local"] = {
    fn_id = 3,
    id = 14
  },
  range = {
    ["end"] = {
      character = 28,
      line = 3
    },
    start = {
      character = 19,
      line = 3
    }
  },
  type = "imm_borrow"
}

Please let me know if this makes the problem easier to understand.

@cordx56
Copy link
Owner

cordx56 commented Mar 2, 2025

Thank you for your detailed and insightful explanation.

I tried your example, examined the LSP response, and now I understand.

First, the LSP server calculates decoration ranges so that decorations never overlap.
This means editors don't need to worry about decoration priority.
But then, why does this issue occur here?

The problem in your example is with is_display.
The LSP server returns hover text that should not be underlined as is_display: false.
This is a function for VSCode to display hidden hover text.

In your example, the response includes two decorations: immutable borrow with is_display: true, and lifetime with is_display: false.

The Neovim plugin should handle is_display, but currently, it does not.

Admittedly, the name is_display is a bit confusing.
However, changing it would break compatibility, so I need to be careful.

Did my response address your concern?
If you’d like, please feel free to fix this issue in this PR!

@AIDIGIT
Copy link
Author

AIDIGIT commented Mar 2, 2025

Thank you for the explanation! In that case we can just check if deco['is_display'] == true - I've updated the PR.
This will also make it so that the annotations displayed by :Inspect (in nvim, not in vs-code; seen at the bottom in my screenshots) contain only 1 annotation, not multiple - which is probably what is intended anyway.

Copy link
Owner

@cordx56 cordx56 left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@cordx56 cordx56 merged commit 3b65871 into cordx56:main Mar 3, 2025
3 checks passed
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.

2 participants