-
Notifications
You must be signed in to change notification settings - Fork 202
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
Close #615: Add support for semantic tokens #839
base: master
Are you sure you want to change the base?
Conversation
The other day I tried this PR out on a very simple example and it worked perfectly. Also, at first glance, the implementation is quite solid. However, I'd wait for someone more knowledgeable for a proper review. Nevertheless, this a legally significant contribution. @AkibAzmain, have you completed your copyright paperwork? If not, you might like to start the process, because it may take a day or two. (Or months in rare cases.) I see that the PR is not feature complete, which is not a problem IMHO, because any support for semantic tokens is better than no support. I think adding some simple tests to eglot-tests.el would help the potential reviewer a lot. Thank you! |
@nemethf it is very good to hear that preliminary feedback. Agree with everything else you said. |
And of course thanks @AkibAzmain for this work :-) |
It looks like there race condition. When large edits are made or large files are opened, semantic tokens are probably requested before the server updates it's internal state and the server replies with data for the old state. Is it a bug of my LSP server (
What's the process? |
Please send email to emacs-devel@gnu.org very briefly describing this contribution to Eglot and requesting from the Emacs maintainers they send you a copyright assignment form off-list.
There was one a server that had such a problem, So, motivated by that, Eglot (jsonrpc.el, actually) has mechanisms to facilitate that sync, but first I'd like to see the race condition highlighted in as jsonrpc transcript snippet, and shown two versions of that snippet. One where the race is "lost" and the desirable (fabricated) case where it's "won" |
@joaotavora That's no longer needed, because the server has no problem. The problem is that the server is takes much time for large files and large edits, and the request times out. :-( Should I request again when a request times out? Or just ignore it? |
Not sure I know how to answer that, to be honest. What are the consequences of such a timeout? I suppose the "safe" thing to do is to somehow warn the user with an |
The consequence is that the colors of the buffer (I mean tokens) become outdated. Not a serious problem IMHO. So timeout is harmless. I think reporting it would just annoy the user, as it is very common when opening a file of a large project (atleast for me). We can also increase or disable the timeout, is that an option? |
I think the server doesn't send anything like that after didOpen. rust-analyzer has an experimental/serverStatus notification, but clients are discouraged to use the status to decide if it's worth sending a request to the server. Probably switching to textDocument/semanticTokens/range would solve the problem, wouldn't it? However, clangd doesn't support this. Maybe we should ask the developers of clangd to tell us how they imagine a client should handle large files with respect to semantic tokens. (If they answer the clients should implement workDoneProgress support, that would mean lot of work, unfortunately.) |
Marking the last two checkbox, as there is no problem on our side. |
@nemethf The request times out sometimes, and that's why the problem occurs. |
But the timeout occurs because the client asks the server to do lots of things at once. The client could ask less from the server like for example to calculate the tokens for just a small portion of the file, or it could let the server to send its answers in small pieces. Or at least that's my understanding of the specification. |
Possibly the client asks the server for tokens before its ready, and the server replies after its ready, in the meantime the client times out.
Yes, there is
Looks like a good idea, but I can't find any partial result implementation in Eglot. Implementing it would probably need changes in many things, including |
@AkibAzmain says:
@nemethf says:
I'm curious to learn more about this "loat of work" and "change many things". If the current proposed implementation is minimally useful/clean already, I don't think these things should block it. Should we make a discussion thread about them? |
@joaotavora says,
I was wrong. Implementing |
I think the current PR is at least minimally useful. A proper review is necessary, but I agree the timeout issue shouldn't block its acceptance.
(I don't think the discussion forum is a good fit for feature requests.) |
I'd have a request concerning this feature. It should be possible to disable certain kinds of highlighting (possibly by setting the appropriate entry of Motivation for this: I'm looking forward to something that can fontify lexical variables accurately, but since I want to actually be able to tell those things apart from the rest, I like to keep my syntax highlighting to a minimum. |
I think that's all possible with this PR, you just have to write hook functions to buffer-locally set the relevant variables. |
Well, with hooks and advices you can do just about anything. |
You can set |
What's the status on this? It would be nice to see this merged in time for the upcoming 29.1 release in a few months. Is there anything we can do to help? |
@leungbk It works well. AFAIK the only blocker is copyright assignment. I hope I can start the process within about 3 weeks. |
I've started using this with Ada, and Adacore ada_language_server. It uses a timer to trigger highlight, and requests a full buffer highlight at the start; this can easily time out on a large file. Instead, I think it should let font-lock request the highlight; that way only a small region will be done in one request, which should be fast. I'll fork this to work on that. |
@stephe-ada-guru: Nice idea. I really don't know enough about font lock, so I took the simplest way: add |
Now as Eglot is in Emacs core, do I need to take any additional steps? |
No. But the FSF copyright assignment is still not finished, or is it? At a certain point int the future I will have a better look at this PR and convert it from PR to patch format. But your authorship of the commits remain unaltered. At that point, we may start following up discussion on the patch on Emacs's bug tracker list, i.e. via email. But for now, no additional steps to take. |
@joaotavora, I've sent the copyright assignment form to assign@gnu.org. Waiting for reply. |
Sorry to disturb. What is the current status of this PR? |
@ksqsf Waiting for me to sign the copyright assignment paper. |
By the way, @joaotavora, what's the state of this repository? |
In the README.md no? I think you should submit this to the Emacs bug tracker at bug-gnu-emacs@gnu.org and we can review this there. There are lots of things to change here, but the original idea is acceptable. This FSF assignment is taking unusually long, i think you should ping the maintainers. |
@joaotavora may I ask what happened with this PR? Was it moved to emacs bugs? Working on a c project with heavy usage of ifdefs, would be great to have some LSP aware way to mark dead code. |
Last I looked at it it had some good parts but also some parts that needed to be rewritten. Doesn't seems like a mammoth effort though (I just don't have the time or much of motivation right now).
I guess I would like that too. Assuming you're using C/C++... Do you know for a fact that
No, but that's where it should live anyway. You can take the initiative and create a bug report there, link this patch, test it, summarize the situation, CC me in the email, ya know all that boring stuff :-). That helps get things rolling again. |
For a fact it's a stretch but looking around online, as far as I'm not mistaken, it seems clangd should already report dead ifdef branches as Comments but it will allow custom styling for inactive regions with the upcoming 17 release. See clangd/clangd#132 (comment) and the references in that comment. I never tested it but according to this answer lsp-mode should already do this (highlight as Comments) https://emacs.stackexchange.com/questions/63429/hide-ifdefor-gray-out-through-compile-command-json
Thanks I'll see what I can do! Time is pretty scarce so cannot guarantee anything though. |
@joaotavora Is this PR still relevant or did someone implemented the feature? It's really late I know, I'm going to sign the copyright papers (within a month or so I hope). |
@AkibAzmain the feature is still relevant. your patch is a starting point, but it will have in principle to be somewhat heavily modified. If you can sign the CA it is always a good thing, regardless. |
Hi, Was there any progress on the CA? I am looking forward to semantic token support in eglot, so I was hoping this would get merged soon. :) |
I've implemented semantic tokens (introduced in LSP 3.16) support. Closes #615.
Support full buffer highlighting (
textDocument/semanticTokens/full
).Support full buffer highlighting using token delta (
textDocument/semanticTokens/full/delta
). It mostly works, but becomes inaccurate when you do the following before the changes are sent to server:Delete
(str);
:printf
Write
(str);
:Setting
eglot-send-changes-idle-time
to 0 fixes it, but probably makes Emacs slow.Support partial buffer highlighting (
textDocument/semanticTokens/range
). I think it works, because full buffer highlighting and partial buffer highlighting is implemented in the same function.Support on-the-fly highlighting.
Support highlighting on server start.
Note: I've tested using only
clangd
12.0.1, which doesn't supporttextDocument/semanticTokens/range
.