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

add semanticHighlighting support #27

Merged
merged 5 commits into from
Mar 25, 2020
Merged

Conversation

fannheyward
Copy link
Member

I'm working on semanticHighlighting on coc-clangd, here is a demo:

test.cpp
#include <iostream>

class Bar {
public:
  void f();
};

int foo(int x, int y) { return x + y; }

int main(int argc, char *argv[]) {
  Bar bar;
  bar.f();

  return 0;
}

Without semantic highlighting:

Screen Shot 2020-03-23 at 22 55 31

With semantic highlighting + vim-lsp-cxx-highlight
Screen Shot 2020-03-23 at 22 56 09

Still needs some improvement: currently, I just parse semantic tokens from clangd to vim-lsp-cxx-highlight's symbol format, then call lsp_cxx_hl#hl#notify_symbols to do highlight. For example, clangd's token:

{
  line: 11,
  tokens: [
    { character: 6, scopeIndex: 2, length: 1 }
  ]
}

to

{
    id: 0,
    kind: 'Method',
    ranges: [ start: { line: 11, character: 6 }, end: { line: 11, character: 7 } ],
    parentKind: 'Unknown',
    storage: 'None'
  }

But I can't get id, storage and parentKind from clangd's token, I just set to Unknown and None. Anyone can help me to correct the token format?

@jackguo380
Copy link

jackguo380 commented Mar 23, 2020

Hey @fannheyward, this is awesome, the highlighting looks really good already.

So about the missing fields:

id

This field comes from libclang and is effectively random from vim-lsp-cxx-highlight's perspective. You can set it to 0 or a random value.

storage, parentKind

You did the right thing to set these to unknown. In fact by default the highlighting in vim-lsp-cxx-highlight doesn't use them. Technically the feature exists but I personally don't use it and I am not sure how many people actually use something other than the default.

I did notice however that you are decoding variable.other.field.static.cpp to StaticField which is technically not a valid kind, it should be decoded to Field with storage class Static. Weirdly enough StaticMethod is a valid kind (and your code is handling it correctly), cquery (and ccls as well) did a bit of a weird hack by adding the StaticMethod extension but it really should have been MethodStatic.

Incremental highlighting

I'm not sure if you already saw this issue but if you start changing lines you will run into issues with the incremental highlighting behavior. The specification allows a semantic highlighting notification to only send "some" lines, but what exactly is sent is kind of underspecified.

From what I understand, for clangd:
If a single lines are edited (No insert/delete), only the highlighting for those lines are sent. Note that it groups these edits by time. So if you quickly search and replace a variable appearing on lines 4, 8, 22 then it will send highlighting for those 3 lines in a single message. If you do it slowly then it will send individual messages for 4, 8, and 22.

If a line is inserted/deleted, all the lines including and after that insert/delete are sent.

Edit: its also worth noting that clangd 9 doesn't do incremental highlighting, only clangd 10 and 11 have this implemented.

This is completely different from cquery/ccls which always sends the highlighting of the entire document. As a result vim-lsp-cxx-highlight makes that assumption as well. It may be best to cache the last message and combine it with the received one and then send the result to vim-lsp-cxx-highlight.

@sam-mccall
Copy link
Member

This is awesome!
AFAIK the only clients of this API at the moment are VSCode and Theia.
It looks like LSP 3.16 will have an official protocol for semantic highlighting: https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.semanticTokens.proposed.ts https://github.com/microsoft/vscode/wiki/Semantic-Highlighting-Overview. I'd expect we'll want to switch the server to that and update the clients to support both.

id

This field comes from libclang and is effectively random from vim-lsp-cxx-highlight's perspective.
You can set it to 0 or a random value.

OOC, what's the intended semantics here? All occurrences of a var get the same ID to allow each var to be a different color? We discussed adding this as an extension to clangd, and could do this if it seems popular.
But we probably want to defer extensions until semanticTokens is in LSP.

Incremental highlighting

I'm not sure if you already saw this issue but if you start changing lines you will run into issues with the incremental highlighting behavior. The specification allows a semantic highlighting notification to only send "some" lines, but what exactly is sent is kind of underspecified.

From what I understand, for clangd:
If a single lines are edited (No insert/delete), only the highlighting for those lines are sent.

Our read of the highlighting proposal we implemented was: imagine an infinite array containing the highlightings for each line number. Initially, each line has no highlightings: LineState[∞] highlights = { none, none, ... }
Each update includes a map from i => LineState[i] for the changed entries. Between updates, clients can interpolate as they like.

This means:

  • edits that don't change the number of lines will send highlights for the affected lines
  • inserting/deleting lines sends a cascade of updates for lines below
  • when the file grows, we send highlights for the new lines at the end. When the file shrinks, we send an empty highlight set for the lines that no longer exist.

Note that the LSP 3.16 proposal has a different delta scheme that seems a lot more sensible (delta-encoding for lines/chars of successive tokens).

Note that it groups these edits by time. So if you quickly search and replace a variable appearing on lines 4, 8, 22 then it will send highlighting for those 3 lines in a single message. If you do it slowly then it will send individual messages for 4, 8, and 22
Right - clangd doesn't send highlights for every version of the file it sees, but it should eventually catch up if you stop typing. (This is because it doesn't build an AST for every version of the file)

If a line is inserted/deleted, all the lines including and after that insert/delete are sent.

Edit: its also worth noting that clangd 9 doesn't do incremental highlighting, only clangd 10 and 11 have this implemented.

This is completely different from cquery/ccls which always sends the highlighting of the entire document. As a result vim-lsp-cxx-highlight makes that assumption as well. It may be best to cache the last message and combine it with the received one and then send the result to vim-lsp-cxx-highlight.

Outside the scope of this patch, but my read of the LSP 3.16 proposal is that it actually enables clients to only do work proportional to the tokens actually changed in most cases. I.e. given a small edit, SemanticTokensEdits should be small and you should only need to do a limited amount of work on it. In this case inflating to a full token set seems like it would be bad for performance, so it might be worth vim-lsp-cxx-highlight having more direct support for the delta format if performance is a concern.

@sam-mccall
Copy link
Member

In case it's interesting/useful, I've prepared a very simple implementation of the LSP 3.16 protocol for clangd: https://reviews.llvm.org/D76663

(No support yet for sending deltas only)

@jackguo380
Copy link

AFAIK the only clients of this API at the moment are VSCode and Theia.

Are you referring to the new proposal or the current one? For Vim/Neovim the current proposal is supported by vim-lsp and LanguageClient-neovim (implemented by me). For servers the only ones that I know of are clangd (9+) and eclipse.jdt.ls.

For id I asked the author of ccls before. I think it is as you say that the id can be used to implement rainbow highlighting as both cquery and ccls' vscode extensions support that feature. Here's his reply with how it is implemented. Currently vim-lsp-cxx-highlight doesn't support it and I never received a feature request so maybe its not very popular?

In terms of performance, currently its not a concern as often it takes significantly longer for the language server to perform analysis than it takes for vim-lsp-cxx-highlight to apply highlighing. But I may revisit this after the new standard is ready.

@sam-mccall
Copy link
Member

Are you referring to the new proposal or the current one? For Vim/Neovim the current proposal is supported by vim-lsp and LanguageClient-neovim (implemented by me). For servers the only ones that I know of are clangd (9+) and eclipse.jdt.ls.

Oh nice, I wasn't aware other clients were using it. OK, then we may have to keep around support for both protocols for a while, as I don't think it's safe to assume everyone has autoupdate for their extensions.

Currently vim-lsp-cxx-highlight doesn't support it and I never received a feature request so maybe its not very popular?
In terms of performance, currently its not a concern as often it takes significantly longer for the language server to perform analysis than it takes for vim-lsp-cxx-highlight to apply highlighing.

Great, no rush for us to worry about these then.

Protocol-wise, I'll try to get basic support for the new LSP protocol landed soon, since it's the future. It's missed the cut for clangd 10. I'd expect clangd 11 to support both, and clangd 12 to maybe drop support for the old/theia extension.

@fannheyward
Copy link
Member Author

I'd like to add this old protocol semanticHighlighting to coc-clangd if this PR is OK. After LSP 3.16 released, maybe I'll add SemanticTokens to coc.nvim core.

package.json Outdated Show resolved Hide resolved
src/ctx.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/semantic-highlighting.ts Show resolved Hide resolved
src/semantic-highlighting.ts Outdated Show resolved Hide resolved
src/semantic-highlighting.ts Show resolved Hide resolved
src/semantic-highlighting.ts Show resolved Hide resolved
jackguo380 added a commit to jackguo380/vim-lsp-cxx-highlight that referenced this pull request Mar 24, 2020
@mellery451
Copy link

sorry to spam this closed PR - @fannheyward do you know if the current implementation supports clangd-10 for inactive regions in particular ? The release notes for clangd indicate that inactive region support is included, but I can't seem to get it to work. If I enable ccls then inactive regions are shown, so I'm thinking there might be a subtle protocol difference. Thoughts?

@fannheyward
Copy link
Member Author

clangd-10 for inactive regions in particular

cc @sam-mccall

@kadircet
Copy link
Member

kadircet commented Jul 3, 2020

the extension seems to be sending necessary notifications in https://github.com/clangd/coc-clangd/pull/27/files#diff-95f1bf2920f2b2f4cd79be925f7acae9R99 and inactive region support is included in clangd-10 as release notes indicate (it was landed in late september, and release was in Feb).

@fannheyward
Copy link
Member Author

fannheyward commented Jan 21, 2021

截屏2021-01-21 18 27 16

coc + clangd 11 with the new SemanticTokens support.

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.

None yet

5 participants