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

Semantic tokens highlighting #6102

Closed

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Feb 25, 2023

This PR introduces Semantic Highlighting capabilities to Helix.

Unresolved question

In this first version, all semantic highlighting is done via a dumb list like this:

Output of :semantic-token command in Helix with this PR

But VsCode does it like this (https://rust-analyzer.github.io/manual.html#semantic-style-customizations):

{
  "editor.semanticTokenColorCustomizations": {
    "rules": {
      "*.mutable": {
        "fontStyle": "underline", // underline is the default
      },
    }
  },
}

I much prefer VsCode's version since it allows applying modifiers only to certain elements, like variable.mutable,
without too much repetions.

In a first version I could simply concatenate everything under semantic.<type>.<mod1>.<mod2>... so that people can
at least narrow it like in VsCode (though it will need dozens of similar lines).

Do we want the "glob-like" patterns in this PR or in a follow-up ?

@poliorcetics
Copy link
Contributor Author

This PR is a draft: I need to write docs, cleanup the code and resolve the issue explained in the first message, but if people want to try it out, it should work nicely, though you'll need to explore with your LSPs and read the code to see what to set in themes to see it working.

@kirawi kirawi added A-language-server Area: Language server client A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 27, 2023
@LouisGariepy
Copy link

There's historically been reluctance to implement LSP highlighting in Helix (for performance reasons IIUC), but this was proposed as a potential solution #5589 (comment)

Tree-sitter will be the default highlighter, while range-based LSP syntax highlighting will be merged asynchronously.

Does this PR implement this functionality? Or has the Helix team's stance on LSP highlighting changed?

@kirawi
Copy link
Member

kirawi commented Apr 25, 2023

This PR does not implement that feature, and AFAIK the stance hasn't changed. It's uncertain what kind of discussion might happen around this PR with respect to that.

@poliorcetics
Copy link
Contributor Author

poliorcetics commented Apr 25, 2023

This PR does not implement that feature

Yes it does, this PR used the tree sitter coloring by default, only adding the LSP colors when it got them

Anyway, I'm not interested in working on helix right now, I just forgot to close this one

@poliorcetics poliorcetics deleted the semantic-tokens branch April 25, 2023 06:54
@kirawi
Copy link
Member

kirawi commented Apr 25, 2023

Ah, good to know if you or anyone else picks this up then.

@Eliemer
Copy link

Eliemer commented Jul 17, 2023

F# for example relies pretty heavily on the LSP to distinguish variables from functions as the only way to figure that out is with types. They look the same in declaration. It doesnt have to be 100% LSP highlights; this specific problem can be solved if i give the LSP every identifier token to classify.

let x: int = 10
let y: unit -> int = fun () -> 20
let z a b = a + b // z: 'a -> 'a -> 'a

@kirawi
Copy link
Member

kirawi commented Sep 22, 2023

I'll take a look at picking this up.

@EricHenry
Copy link
Contributor

@kirawi depending on how busy you are. I’d love to help out with this one if possible.

@LouisGariepy
Copy link

LouisGariepy commented Sep 27, 2023

I've been thinking about this a bit more, and I started having some doubts about the "start with tree-sitter then merge semantic highlighting when it's ready" approach.

Instead, why not simply have an option to toggle semantic highlighting? So people who only want tree-sitter don't pay for semantic highlighting, and people who only want semantic highlighting don't pay for tree-sitter.

In addition to avoid wasting compute power, I imagine this would also be easier to implement overall.

Thoughts @kirawi ?

@pascalkuthe
Copy link
Member

we don't want multiple alternative syntax highlighting implementations in core.

If we are going to support semantic highlighting doingit asynchrounsly and only for a few selected scopes is the only thing we are going to accept. Every vscode extension has a basic regex based highlighting so that delays in the LSP highlighting (which is always computed async so often not in the same frame) don't cause parts of the text to be unhighlited.

Some of these issues can be solved using word-based range mapping like in #6447 but with lots of syntax highlighting ranges that quickly becomes a performance problem. Similarly low timeout requests of syntax highlights is pretty wasteful (since there is no way to only request some scopes) so there should be a a fairly high request timeout (which makes range mapping event more important).

So to get acceptable performance for core you will need to request highlights rarely and only track very few highlight spans inside the editor.

@kirawi you should likely base the highlight requests on #8021 (you want to be pretty granular here). The implementation should likely be quite similar to inlay hints so you could consider doing that in one PR.

@LouisGariepy
Copy link

LouisGariepy commented Sep 27, 2023

@pascalkuthe

we don't want multiple alternative syntax highlighting implementations in core.

If we are going to support semantic highlighting doingit asynchrounsly and only for a few selected scopes is the only thing we are going to accept.

Can you expand on why that is? IMO that should be a decision left to the user based on how much they value semantic highlighting and how performant their machine is.

I think such an entrenched position on the side of the project should at least have clearly stated technical reasons.

(To be clear, I'm not trying to argue for a different position here, I just think the reasoning should be documented somewhere)

@pascalkuthe
Copy link
Member

@pascalkuthe

we don't want multiple alternative syntax highlighting implementations in core.
If we are going to support semantic highlighting doingit asynchrounsly and only for a few selected scopes is the only thing we are going to accept.

Can you expand on why that is? IMO that should be a decision left to the user based on how much they value semantic highlighting and how fast their machine is.

I think such an entrenched position on the side of the project should at least have clearly stated technical reasons.

As @archseer said recently on matrix about opinions like this in general: just "adding an option to let the user decide" when we have made a different design decision (in this case using tree sitter) is not something we accept usually. From the user perspective it just "lets them do what they want" but for us it means reviewing and maintaining a bunch of extra code. Code that lives in helix core should be up to a certain quality (including performance) so adding a bunch of different alternatives is a lot of work.

The technical reason in this case: its an inefficient/slow protocol and requires additional regex highlighting (which we also don't want to add/maintain) to avoid UI flickering. Tree sitter is used for many additional things inside helix besides syntax highlighting so it's a core editor component. We don't want to add/maintain alternatives to core editor components.

This is documented in the original issue for semantic highlighting, by the comment from @archseer there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-language-server Area: Language server client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants