Page MenuHomePhabricator

[clangd] Add static index for the global code completion.

Authored by hokein on Jan 2 2018, 9:08 AM.



Use the YAML-format symbols (generated by the global-symbol-builder tool) to
do the global code completion.
It is experimental only , but it allows us to experience global code
completion on a relatively small project.

Tested with LLVM project.

Diff Detail

Event Timeline

hokein created this revision.Jan 2 2018, 9:08 AM
ioeric added inline comments.Jan 3 2018, 12:21 AM

We are calling this global index and static index in the patch. I think we should be consistent with the naming. Generally, I think this is static index, which might be global index or, say, a set of symbols for test purpose, so I am inclined to static index.


I would suggest also addressing this FIXME in this patch, or at least make it possible to tell from which index source a candidate comes on the client side. Simple concatenation from both indexes would make the results hard to interpret, especially when we don't have a good merging algorithm at this point.


clangd client is a confusing term? Do you mean clangd library users? Also, I think this field is only set internally by clangd in this patch.


This depends on users, so I would drop this line.



Any reason not to pass in a SymbolSlab directly?


I'd make this a factory method and call it Build.


Maybe YamlSymbolFile or something similar without "global"? Yaml could potentially be used for other static indexes.


We should also have a test where results come from both static index and ast index.

You have mentioned that YAML data source is experimental only in the patch summary, but we should also mention this in the code.

ilya-biryukov added inline comments.Jan 3 2018, 2:41 AM

Let's also add a mention here in the documentation that the option is experimental, will be removed in the future and users should not rely on it.

hokein updated this revision to Diff 128513.Jan 3 2018, 3:19 AM
hokein marked 8 inline comments as done.

Address review comments.

hokein added a comment.Jan 3 2018, 3:22 AM

Thanks for the comments.


+1 static index -- I forgot to rename the global index.


The FIXME is mainly about the merging algorithm.

I still prefer to a follow-up on telling the client from which index source a completion item comes. I can foresee we might need to add an new field (like Source) in the CompleitionItem. Let's keep this patch simple at the moment.

ioeric added inline comments.Jan 3 2018, 3:30 AM

I don't think we need additional field for this. We could manipulate the label or detail of the completion item (e.g. adding a tag), which shouldn't be too much extra work for this patch. IMO, concatenated results might easily be more confusing than useful for our testing purpose at this point, given no merging algorithm.

hokein updated this revision to Diff 128603.Jan 4 2018, 1:11 AM
hokein marked an inline comment as done.

Add index source information to the completion item.

ioeric added inline comments.Jan 5 2018, 1:12 AM

Please also add comment for this.


... in addition to the FileIdx above?


AFAIK, Detail is not visible in the completion item list unless you hover on an item. I'd suggest throwing a "[G]" prefix to labels of the global symbols. For completion clang::^, the list would look like:




We might not want to lose (non-index-based) AST symbols if we only have StaticIndex. Maybe only use static index augment the dynamic index?


It's unclear whether this is set internally or expected to be set by users?

hokein updated this revision to Diff 128741.Jan 5 2018, 5:50 AM
hokein marked 5 inline comments as done.

Address comments.

hokein added inline comments.Jan 5 2018, 5:53 AM

Updated the comment.


Yeah, this would make debugging easier. We could figure out a better shown way to the end users in the future.


The Opts.StaticIndex check is not need here, removed it -- the StaticIndex is set when enable-index-based-completion is on.

ioeric accepted this revision.Jan 5 2018, 5:59 AM



Maybe IndexSourceLabel?


Is this blank line intended?

This revision is now accepted and ready to land.Jan 5 2018, 5:59 AM
hokein updated this revision to Diff 129047.Jan 9 2018, 2:04 AM
hokein marked 2 inline comments as done.

Fix remaining comments.

sammccall added inline comments.Jan 9 2018, 3:04 AM

is there a reason ClangdServer should own this? I can imagine this complicating life for embedders. (vs a raw pointer)


Actually, can we give this a more generic name like DebuggingLabel?
That will make things less confusing as we store slightly different info here over time (e.g. this text will definitely change when merging happens). It also indicates that this isn't functionally important.

If you don't mind, I'd also change the value so you just pass "G" and it gets formatted into "[G] " by this function. That means we can easily format it differently (e.g. hide it from the UI actually include it in the JSON as an extension field) in a way that makes sense.


nit: this comment implies that we should do a two-way merge here between static/dynamic index. I don't think that's the case.
We're also going want to merge with AST based results, and probably want to do that *before* generating CompletionItems, because scoring should take signals from all sources into account.

I'd suggest reverting this function to use just one index, and calling it twice.
Then this comment about merging would live near the call site, and cover all three sources - it'd suggest the fix where we should be applying it.

ilya-biryukov added inline comments.Jan 9 2018, 3:19 AM

+1, we had a design where ClangdServer owns everything before and having a raw reference turned out to be much more flexible!

hokein updated this revision to Diff 129081.Jan 9 2018, 7:22 AM
hokein marked 4 inline comments as done.

Address review comments.

sammccall accepted this revision.Jan 9 2018, 9:19 AM

Ilya's suggestion to have a single index on the CodeCompleteOptions that encapsulates the merging seems reasonable to me, but not urgent.
So let's get this in as-is and we can make that change whenever.


move this inside and only use for nontrivial case?


Shouldn't it already be false here?


do you only want to use the static index if the dynamic index is also present?
(That logic may exist at a higher level, but I'm not sure it makes sense to have it here)

I'd suggest to remove the result items empty check and the clear(), and then just put the two index runs in independent if statements

hokein updated this revision to Diff 129245.Jan 10 2018, 4:39 AM
hokein marked 3 inline comments as done.
  • make static index independent with dynamic index
  • don't clear results from clang's completion engine as we also intend to merge

them into the final results eventually.

sammccall accepted this revision.Jan 10 2018, 5:43 AM

Nice! LG

hokein updated this revision to Diff 129265.Jan 10 2018, 5:58 AM
  • Update comment
  • Only use Sema results if the scope specifier (ns::^) is not a namespace scope specifier.
sammccall added inline comments.Jan 10 2018, 6:16 AM

I don't think we need this - we plan to avoid overlap by splitting on current file vs other files - i'll put together a patch to make sema do what we want.
Until we've got that done, having some duplicate results is OK (this feature is experimental still).

hokein updated this revision to Diff 129272.Jan 10 2018, 6:38 AM

Remove the temporary fix.

hokein marked an inline comment as done.Jan 10 2018, 6:39 AM
hokein added inline comments.

Thanks, that makes sense. Reverted it.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.