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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
                               Callback<tooling::Replacements> CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-                            std::string TweakID,
-                            Expected<InputsAndAST> InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+                      Expected<InputsAndAST> InpAST) {
----------------
kadircet wrote:
> Can you revert this change?
I tried, but clang-format automatically makes the change again.

Is there some particular clang-format configuration I should apply, so it produces the same results? I don't currently have any configuration, i.e. I think it's just reading the .clang-format file in the monorepo root.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+                 TypeHierarchyDirection Direction) {
----------------
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.


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+    const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+    EXPECT_EQ(nullptr, RD);
+  }
----------------
kadircet wrote:
> Can you put a TODO?
I added a comment to clarify that a field does not unambiguously specify a record type. i don't think there's anything further "to do" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370