[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+      return CB(InpAST.takeError());
+    CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+                                Direction));
----------------
nridge wrote:
> sammccall wrote:
> > relying on the item range to resolve the actual symbol isn't reliable:
> >  - the source code may have changed
> >  - the range may not be within the TU, and might be e.g. in an indexed header we don't have a compile command for.
> Good point. I appreciate a bit better now why you suggested trying to avoid `resolve` for now :)
> 
> What do you think of the following implementation approach:
> 
>  * Use the `data` field of `TypeHierarchyItem` (which the client will send back to the server for `resolve` queries) to send the `SymbolID` of the item to the client.
>  * When the client sends back the `SymbolID` in the `resolve` request, look up the symbol in the index, and use the source range from the symbol.
> 
> (Later, when we start storing base <--> derived relationships in the index for subtype support, we could just answer the entire `resolve` query using the index.)
Thanks for exploring all the options here!

Generally we've tried to avoid relying on the index unless it's needed, using the AST where possible. There are failure modes here:
 - the base type is in the current file, which is actively edited. The ranges in the index may be off due to staleness.
 - the base type is not indexed, because it is e.g. a member class inside a class template
 - there's no index (`-index=0`, though I'm not sure why we still support this) or the index is stale and the type is missing (we're working on making index updates more async)
 - the base type is not encodable.

There are just more moving pieces here, I think. Is there a clear reason to support resolve for parents?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370