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

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge 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));
----------------
sammccall wrote:
> 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?
> Is there a clear reason to support resolve for parents?

Just what I said earlier about a hypothetical client that relies on it.

However, given the complications involved in implementing it, I'm happy with only being concerned with actual clients, not hypothetical ones. The only client implementation I currently know of is Theia, and I checked that it works fine without `resolve`, so I'm happy with deferring work on `resolve` for now.

If another client comes along that relies on `resolve`, we can revisit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370