Page MenuHomePhabricator

[clangd] Implement textDocument/codeLens
Needs ReviewPublic

Authored by lightmelodies on Sun, Nov 22, 5:32 AM.

Details

Summary

This commit adds support for https://microsoft.github.io/language-server-protocol/specification#textDocument_codeLens.
See also https://github.com/clangd/clangd/issues/442 and https://github.com/lightmelodies/vscode-clangd.

CodeLens is a popular feature in Visual Studio Code, which provides actionable contextual information interspersed when opening a file:

  • Contextual: codelens is displayed as a decoration and contains information of nearby code, e.g. show how many usages of a function without invoking findReferences manually.
  • Actionable: codelens has an associate command, so you can click it and tiggers some action.

Here is a simple gif to show how it works.


Implementation details:

  • getDocumentCodeLens: This function implements the textDocument/codeLens API.
    • use documentSymbol to determine where codelens can occur.
    • For each symbol, add a reference codelens which displays the count of references. To avoid unnecessary lookup, only resolve data will be returned to the client in this step. The actual command will be filled by codeLens/resolve when the symbol getting into the viewport.
    • For class and virtual methods, use getTypeHierarchy and getMemberHierarchy to get hierarchy codelens which displays the count of base and derived. We resolve the command directly since the overhead is little in general and '0 derived' seems to be weird and useless.
  • resolveCodeLens: This function implements the codeLens/resolve API. Just use findReferences to get the count of references according to the resolve data.
  • onCodeLensCommand: This function extends the onCommand function to support codeLens command. It just returns the location of related symbols. VSCode use editor.action.showReferences to show them, so the whole action will work like invoking Peek References.

Diff Detail

Event Timeline

lightmelodies created this revision.Sun, Nov 22, 5:32 AM
lightmelodies requested review of this revision.Sun, Nov 22, 5:32 AM
lightmelodies edited the summary of this revision. (Show Details)Sun, Nov 22, 5:43 AM

Please upload the diff with full context and fix the failing unit tests

lightmelodies edited the summary of this revision. (Show Details)Sun, Nov 22, 6:09 AM
lightmelodies edited the summary of this revision. (Show Details)
lightmelodies set the repository for this revision to rG LLVM Github Monorepo.Sun, Nov 22, 6:34 AM
lightmelodies set the repository for this revision to rG LLVM Github Monorepo.Sun, Nov 22, 7:11 AM

Thanks a lot for working on improving clangd!

Can you also give a high-level overview of what kind of functionality you are providing here? Looks like there's a lot going on here, and it would be nice to know what you are attempting to do, rather than inferring that from the code. It would also help future travellers, as they can just read the description rather than going through the whole patch.

lightmelodies edited the summary of this revision. (Show Details)Mon, Nov 23, 10:25 PM

Thanks a lot for working on improving clangd!

Can you also give a high-level overview of what kind of functionality you are providing here? Looks like there's a lot going on here, and it would be nice to know what you are attempting to do, rather than inferring that from the code. It would also help future travellers, as they can just read the description rather than going through the whole patch.

Thanks for your response. I have updated the summary and add some detailed explanation.

lightmelodies edited the summary of this revision. (Show Details)Mon, Nov 23, 10:29 PM

Thanks for the explanations. I've also had some discussions with @sammccall about this and would like to summarize them.

There are two important things to keep in mind:

  • Collection is performed for the whole document.
  • Resolve requests cannot fail. (there's no null for the return type)

So this forces us to be really certain about existence of codelenses during collection time, while making sure latency is low enough for clangd to not feel sluggish, as this needs to be served after inactivity (i.e. not typing for a while).
Hence first of all we can't surface these lenses for all kinds of symbols, limiting the scope to top-level symbols as you do in this patch sounds like a sweet-spot.

But it is not enough on its own, for example we are still performing multiple index queries per symbol to figure out derived classes/methods, which doesn't scale well to big TUs, especially in presence of remote-index where each query can have considerable RTT.
So we need to ensure total number of index queries are kept to a minimum, possibly by batching all of the queries and performing them at once.
This requires significant changes to current implementation, but is really a must to ensure scalability of the feature. WDYT about evolving the patch in such a direction?

Another point around the usefulness of the lenses in this patch; The information we can deduce using only the AST doesn't seem to be useful.
Particularly:

  • for bases, they are available next to the cursor and user can do go-to-definition on them.
  • for overridden methods, most of the time there are 0 or 1, and again they keyword override is usually an indicator of that (moreover users can do go-to-def on that, but it is not as visible).

So it might be better to just drop them to not clutter the UI.

I have also considered these problems, and here is some points of my view.
Latency
Consider llvm-project itself, textDocument/codeLens can be done in <500ms generally and <100ms if collect hierarchy is disabled in my notebook. Latency is significant in some rare case like Type.h which has too complex hierarchy relations. Meanwhile, vscode is smart to remember previous codelens's positions so the text rendering will not look too weird during long-time request. In fact, the biggest performance issue I find is that building AST when open a new file costs too much time, which makes textDocument/codeLens quite slow, as well as other action such as textDocument/documentSymbols. That's why I modified ClangdLSPServer.cpp to keep AST for recent closed files.

fileline countcostcost (without hierarchy)
ASTVector.h4114ms1ms
APValue.h69316 ms8ms
Decl.h4599213 ms40 ms
Expr.h6383447 ms61 ms
Type.h72223765 ms47 ms

Batch
That's definitely a problem with remote index since vscode can only do resolution per symbol. Maybe it will be better to resolve directly in textDocument/codeLens requests when deal with remote index. Just need add an option to allow users switch the strategy.

Useless Lens
Yes, we can see the base directly in class declaration. However c++ has forward declaration and type alias, in such case the base can not be seen directly. So someone may prefer to have these codelens. Typescript in vscode has options to allow users disable specific type of codelens. We can use similar strategy, e.g. -codelens=references,base,derived instead of -codelens=true.

I'm currently working on an implemention by traversing AST directly instead of using textDocument/documentSymbols, since symbols defined by macro are often reported a wrong range. It should save much time of findNamedDeclAt comparing to current implemention, and make it easier to add additional filters on symbols.