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

[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:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+                 TypeHierarchyDirection Direction) {
----------------
nridge wrote:
> sammccall wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > sammccall wrote:
> > > > > The scope and complexity of this function seems unneccesarily large:
> > > > >  - it's (in future?) capable of resolving in both directions
> > > > >  - it calls itself recursively, modifying TypeHierarchyDirection to control the search
> > > > >  - it handles levels, even though this optimization is only important for derived types
> > > > > 
> > > > > Can we restrict this to retrieving (all) recursive base types?
> > > > > i.e. `Optional<TypeHierarchyItem> getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &Ctx)`
> > > > > Then later, subtype resolution can be the job of another function: `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > > 
> > > > > That way the recursion of getTypeAncestors is simpler to understand, as it has much smaller scope. And code sharing between the two LSP calls is clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call to `resolveTypeDescendants` (if direction is children or both, and resolve is > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.
> > > > If we remove "levels" here, should we introduce some kind of guard for infinite recursion, in case the user writes something like:
> > > > 
> > > > ```
> > > > template <int N>
> > > > struct S : S<N + 1> {};
> > > > 
> > > > S<0> s;
> > > > ```
> > > clang should be limiting recursive template instantiations. Also since we are just traversing the AST produced by clang, it should never be infinite, but still a nice test case can you add one?
> > I think there is a point here...
> > 
> > Consider our `S` template with the direction reversed and a base case specialized:
> > ```
> > template <int N>
> > struct S : S<N - 1> {};
> > template<>
> > struct S<0>{};
> > 
> > S<2> instance;
> > ```
> > 
> > Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : S<0>`.
> > However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the primary template, whose base is `S<N - 1>`, which is dependent[1], so the base's `CXXRecordDecl` is the primary template, whose base is `S<N - 1>`... and we never reach the base case.
> > 
> > Actually I'm not sure whether this happens if the base is dependent merely where it's spelled, or still dependent after instantiation? Even in the latter case one can construct examples where we'll infloop:
> > ```template <int N>
> > struct Foo {
> >   S<N> instance;
> > }```
> > Trying to get the hierarchy on `S<N>` could infloop. (I agree these should both be test cases)
> > 
> > What's the Right Thing?
> >  - not sure about a recursion limit, as showing 10 `S<N-1>`s in the hierarchy is silly.
> >  - not sure about bailing out on dependent types either, as knowing that e.g. my `SmallSet<T>` inherits from `SmallMap<T, bool>` is meaningful or useful.
> >  - maybe we should bail out once we see instantiations of the same template decl twice in a parent chain. I.e. keep the seen non-null `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop recursing if insertion fails.
> > 
> > However for this patch I'd suggest just bailing out on dependent types with a TODO as the simplest, this is an edge case that can be fixed later.
> > 
> The current patch does actually recurse infinitely on this test case, likely for the reasons Sam outlined (our handling of dependent types), and eventually runs into the segfault.
> 
> I will update the patch to address this.
I meant to say "eventually runs into a stack overflow".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370