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

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1649
+  // into the same CallHierarchyIncomingCall.
+  llvm::DenseMap<SymbolID, CallHierarchyIncomingCall> ResultMap;
+  // We can populate the keys of ResultMap, and the FromRanges fields of
----------------
kadircet wrote:
> 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.
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.


================
Comment at: clang-tools-extra/clangd/XRefs.h:109
+/// Get call hierarchy information at \p Pos.
+llvm::Optional<std::vector<CallHierarchyItem>>
+prepareCallHierarchy(ParsedAST &AST, Position Pos, const SymbolIndex *Index,
----------------
kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > kadircet wrote:
> > > > > what's the distinction between empty and none return values ? (same for others)
> > > > 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 `CallHierarchyItem`s 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".
> > > > 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.
> > The protocol [specifies](https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#textDocument_prepareCallHierarchy) 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).
> 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.
Ok, removed the Optional for consistency with other requests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91122/new/

https://reviews.llvm.org/D91122