This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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
clang-tools-extra/clangd/XRefs.cpp
1394

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

1651

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)

1711

can we log the error instead?

1716

nit: early exit if (!FuncOrFuncTempl)continue;

1724

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

1728

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

1755

the error needs to be consumed (preferably by logging)

1759

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

void foo();
void bar() {
 funcTakingFunc(&foo);
}

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?

1767

early exit, also the error needs to be consumed

1770

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

clang-tools-extra/clangd/XRefs.h
114

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

115

why do we need the index in this one?

119

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

clang-tools-extra/clangd/XRefs.cpp
1394

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

clang-tools-extra/clangd/XRefs.h
114

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.

clang-tools-extra/clangd/XRefs.cpp
1396

nit: redundant braces

1404

nit: merge the two conditions and drop the braces.

1412

can you qualify Optional here and elsewhere?

1433–1446

nit: drop std::move

1439

nit: redundant braces

1448

nit: again merge conditions and drop braces

1665

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

1674

nit: auto ID

1691

... by SymbolID of the caller.

1694

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.

1705

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

1716

why don't we assert this instead?

clang-tools-extra/clangd/XRefs.h
114

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
clang-tools-extra/clangd/XRefs.h
114

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
clang-tools-extra/clangd/XRefs.h
114

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.
clang-tools-extra/clangd/XRefs.cpp
1694

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.

clang-tools-extra/clangd/XRefs.h
114

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!

clang-tools-extra/clangd/XRefs.cpp
1665

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
clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
86

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.
clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
86

Thanks for catching that! Patch at https://reviews.llvm.org/D92000

nridge marked an inline comment as done.Nov 24 2020, 12:22 AM
nridge added inline comments.
clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
86

Sorry, wrong link, correct link is: https://reviews.llvm.org/D92009