Page MenuHomePhabricator

[clangd] Call hierarchy (XRefs layer, incoming calls)

Authored by nridge on Nov 9 2020, 9:53 PM.



Support for outgoing calls is left for a future change.

Diff Detail

Event Timeline

nridge created this revision.Nov 9 2020, 9:53 PM
nridge requested review of this revision.Nov 9 2020, 9:53 PM
nridge updated this revision to Diff 304048.Nov 9 2020, 9:56 PM

Update a comment

kadircet added inline comments.Nov 10 2020, 1:52 PM

that's a great change to have, but maybe leave it out of this patch?


can we rather have a template like:

template <typename HierarchyItem>
HierarchyItem fromDecl(const NamedDecl &ND) {
HierarchyItem Result;
auto &Ctx = ND.getASTContext();
return Result;

To merge this one with the declToCallHierarchy ? The only separate bit seems to be around deprecated. They can be set explicitly outside. (looks like we forgot to add the SymbolTag for deprecated here)


can we log the error instead?


nit: early exit if (!FuncOrFuncTempl)continue;


this can be merged with the typehierarchy implementation using a similar template trick mentioned above.


let's elog instead. I think just saying Failed to convert symbol to hierarchy item: {0} should be fine in the merged implementation.


the error needs to be consumed (preferably by logging)


i suppose the main problem is we might have false positives:

void foo();
void bar() {

bar looks like it is calling foo, but it isn't. but i think it is useful, and probably the only place we can point out a potential indirect call without crazy static analysis. So even if we had separate kinds for call and mere references, i believe we would still surface both. Hence, can we just mention this caveat rather than putting a fixme here?


early exit, also the error needs to be consumed


instead of performing a lookup per reference, can we just accumulate the symbolids and do a single lookup after finding all the unique containers?


what's the distinction between empty and none return values ? (same for others)


why do we need the index in this one?


In theory AST will have more up-to-date information about ranges/occurrences within the current file. As dynamic index for it might be stale. (the same goes for typehierarchy, we've just forgot about it at the time).

But going from SymbolID to an AST node isn't cheap, especially when the declaration isn't inside the main file. So I suppose it is OK to move with an index only implementation for now, but we should leave a FIXME to remind us about the situation.

nridge updated this revision to Diff 305400.Nov 15 2020, 6:17 PM
nridge marked 12 inline comments as done.

Address review comments


Since we are unifying the function with symbolToCallHierarchyItem, and that didn't have an Index parameter, it made more sense to keep the removal.


Generally speaking, a None result means "the input was not recognized in some way", while an empty vector result means "there are no results for this input".

For prepareCallHierarchy, the inputs encode a source location, and the operation asks "give me CallHierarchyItems for suitable declarations (i.e. functions) at this location. So, None means "the source location could not be recognized" (sourceLocationInMainFile failed), while an empty result means "there are no declarations of functions at this location".

For incomingCalls and outgoingCalls, the inputs encode a declaration of a function, and the operation asks "give me its callers / callees". So, a None means "could not locate the function (i.e. symbol)", while an empty result means "this function has no callers / callees".

nridge updated this revision to Diff 305407.Nov 15 2020, 8:07 PM

Update variable names and remove a commented declaration

mostly LG, i haven't looked at the tests yet though.


nit: redundant braces


nit: merge the two conditions and drop the braces.


can you qualify Optional here and elsewhere?


nit: drop std::move


nit: redundant braces


nit: again merge conditions and drop braces


Results.emplace_back(std::move(*CHI)) ?


nit: auto ID


... by SymbolID of the caller.


nit: s/ResultMap/CallsIn ?

I would also suggest changing value type to SmallVector<Range, 1> that way rather than having a "partially filled" IncomingCall objects in the middle, you can create valid ones directly within the lookup.


nit: auto It = ResultMap.try_emplace(R.Container, CallHierarchyIncomingCall{}).first


why don't we assert this instead?


Generally speaking, a None result means "the input was not recognized in some way", while an empty vector result means "there are no results for this input".

Makes sense, but that sounds like an implementation detail. I don't see any difference between the two from caller's point of view. Even the logging happens inside the implementation. As for interpreting llvm::None by the caller, i believe it is subject to change, so unless we return an Expected, there's not much value in returning an Optional, and even in such a case, caller probably can't do much but propagate the error (maybe also try with Pos+/-1, but usually that's handled within the XRefs as well).

Returning an empty container is also coherent with rest of the APIs we have in XRefs.h.

So I believe we should drop the optionals here.

nridge added inline comments.Nov 18 2020, 12:49 AM

The protocol specifies return types of CallHierarchyItem[] | null for prepareCallHierarchy and CallHierarchyIncomingCall[] | null for incomingCalls.

The | null in there suggests that the protocol intends for there to be a semantic difference between an empty array and null, and that clients may want to do things differently in the two caes (e.g. show an "unable to compute call hierarchy" error dialog vs. show an emty tree).

kadircet added inline comments.Nov 21 2020, 5:36 AM

yes, that's indeed a possibility, and the same goes for other features like textDocument/{definition,declaration,references}. AFAICT, we don't signal the difference between "no results" and "something went wrong" for those, and haven't received any complaints yet. So I'd rather keep them coherent, and replace all of them if we see that there are clients taking different actions for real.

nridge updated this revision to Diff 306951.Nov 22 2020, 6:26 PM
nridge marked 14 inline comments as done.

Address review comments

nridge marked an inline comment as done.Nov 22 2020, 6:28 PM
nridge added inline comments.

I went with std::vector<Range> as we eventually need that for the final result, and we can move it from one place to another.


Ok, removed the Optional for consistency with other requests.

kadircet accepted this revision.Nov 23 2020, 1:46 AM

This looks great! Thanks a lot for bearing with me and doing all of this!


please put the deref inside std::move. as move semantics of Optional are optional (no pun intended :P), i.e. depends on LLVM_HAS_RVALUE_REFERENCE_THIS.

This revision is now accepted and ready to land.Nov 23 2020, 1:46 AM
nridge updated this revision to Diff 307223.Nov 23 2020, 5:42 PM
nridge marked 2 inline comments as done.

Address final review comment

Thank you for the reviews!

This revision was landed with ongoing or failed builds.Nov 23 2020, 5:44 PM
This revision was automatically updated to reflect the committed changes.

It looks like CallHierarchy.IncomingOneFile in failing on a few of the buildbots (e.g. clang-ppc64be-linux and clang-s390x-linux), while passing on most of them. I cannot reproduce the failure locally.

Any suggestions for what might be the cause, or how to investigate?

kadircet added inline comments.Nov 24 2020, 12:11 AM

the order of elements in IncomingLevel2 is not sorted, so on some buildbots you are receiving "caller3" as the first element of the result, and "caller2" in others.

sorry for missing it the first time, it might make sense to sort containers by name before returning from incomingCalls

nridge marked an inline comment as done.Nov 24 2020, 12:21 AM
nridge added inline comments.

Thanks for catching that! Patch at

nridge marked an inline comment as done.Nov 24 2020, 12:22 AM
nridge added inline comments.

Sorry, wrong link, correct link is: