Page MenuHomePhabricator

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

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

Details

Summary

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
clangd/ClangdLSPServer.h
40

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.

clangd/CodeComplete.cpp
615

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/CodeComplete.h
72

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.

74

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

clangd/index/MemIndex.cpp
56

s/Slab/Builder/?

Any reason not to pass in a SymbolSlab directly?

clangd/index/MemIndex.h
42

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

clangd/tool/ClangdMain.cpp
118

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

unittests/clangd/CodeCompleteTests.cpp
490

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
clangd/tool/ClangdMain.cpp
121

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.

clangd/ClangdLSPServer.h
40

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

clangd/CodeComplete.cpp
615

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
clangd/CodeComplete.cpp
615

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
clangd/ClangdServer.h
211

Please also add comment for this.

345

... in addition to the FileIdx above?

clangd/CodeComplete.cpp
599

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:

clang::Local1
clang::Local2
[G]clang::Global1
[G]clang::Global2

WDYT?

666–667

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?

clangd/CodeComplete.h
72

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
clangd/ClangdServer.h
345

Updated the comment.

clangd/CodeComplete.cpp
599

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

666–667

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

lgtm

clangd/CodeComplete.cpp
563

Maybe IndexSourceLabel?

598

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
clangd/ClangdLSPServer.h
40

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

clangd/CodeComplete.cpp
563

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.

615

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
clangd/ClangdLSPServer.h
40

+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.

clangd/CodeComplete.cpp
576

move this inside and only use for nontrivial case?

667

Shouldn't it already be false here?

668

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
clangd/CodeComplete.cpp
671

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.
clangd/CodeComplete.cpp
671

Thanks, that makes sense. Reverted it.

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