[clangd] Show template arguments in type hierarchy when possible

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/31

[kadircet]

I think this requires changes in other places too, for example when querying index for the children we rather want to query using the symbolid of template pattern, not the instantiation.

clang-tools-extra/clangd/XRefs.cpp

682

instead of doing the traversal twice, can we just sent both pattern and instantiation here, and then prefer the specializationdecl instead of just selecting decls[0] below ?

[nridge]

Address review comments

[nridge]

In D71533#1785376, @kadircet wrote:

I think this requires changes in other places too, for example when querying index for the children we rather want to query using the symbolid of template pattern, not the instantiation.

Good catch! I added a couple of additional test cases to exercise this.

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

693

maybe just

const Decl *D = Decls.front()
for(const auto *C : Decls) {
  if(isa<ClassTempl...>(C)) {
    D = C;
    break;
  }
}

771

nit: no need for braces

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

418

what about making use of template pattern in case of invalid instantiations?

as type hierarchy tries to provide information regarding bases and children, I think it is more important to show those instead of template instantiation arguments.

464

can you also add a case for deriving from explicit (partial) specialization and type hierarchy on instantiation of a different pattern?
e.g.

template <typename T> class X {};
template <typename T> class X<T*> {};

struct Child : X<int*> {};

X<int> fo^o;

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

693

now you can use explicitReferenceTargets instead, with only DeclRelation::Underlying set in the mask.

[nridge]

Address most review comments

[nridge]

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

418

Is there a way to query a ClassTemplateSpecializationDecl for whether instantiating its base specifier failed?

Or, should we fall back on the template pattern any time bases() is empty?

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

140

this will result in changes in behavior for other functionality (it is unfortunate if no tests regressed), for example findReferences did work on template patterns rather than instantiations now it won't receive pattern results when triggered on instantiations. whether we want that or not is up for debate, but definitely not part of this patch.

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

418

you can check for CTSD->isInvalidDecl() and fallback to pattern in such cases.

[nridge]

Address review comments

[merge_guards_bot]

Unit tests: pass. 61304 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

688

instead of creating a new function, could you rather inline that ? it is the only call site and almost the same with getDeclAtPosition.
the comment above (with little modifications) should suffice to explain why getDeclAtPosition is not used here.

[nridge]

Address latest review comment

[merge_guards_bot]

Unit tests: pass. 61729 tests passed, 0 failed and 779 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

Thanks, LGTM!

clang-tools-extra/clangd/XRefs.cpp

718

nit: redundant braces

[nridge]

Address nit

[merge_guards_bot]

Unit tests: pass. 61774 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-tools-extra/clangd/XRefs.cpp

 std::vector<const Decl *> getDeclAtPosition(ParsedAST &AST, SourceLocation Pos,
                                             DeclRelationSet Relations) {
   FileID FID;
   unsigned Offset;
   std::tie(FID, Offset) = AST.getSourceManager().getDecomposedSpellingLoc(Pos);
   SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
   std::vector<const Decl *> Result;
   if (const SelectionTree::Node *N = Selection.commonAncestor()) {
     auto Decls = targetDecl(N->ASTNode, Relations);
     Result.assign(Decls.begin(), Decls.end());
   }
   return Result;
 }
 
 llvm::Optional<Location> makeLocation(ASTContext &AST, SourceLocation TokLoc,
                                       llvm::StringRef TUPath) {
   const SourceManager &SourceMgr = AST.getSourceManager();
if (Pattern) {
     RPSet.erase(Pattern);
   }
 }
 
 const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
+  // First, try to find a template instantiation. This will give us
+  // the ClassTemplateSpecializationDecl for a non-dependent
+  // specialization, allowing us to produce a more accurate type
+  // hierarchy specific to that specialization.
   DeclRelationSet Relations =
-      DeclRelation::TemplatePattern | DeclRelation::Underlying;
+      DeclRelation::TemplateInstantiation | DeclRelation::Underlying;
   auto Decls = getDeclAtPosition(AST, SourceLocationBeg, Relations);
+  if (Decls.empty()) {
+    // For a dependent specialization, there is no specialization Decl
+    // to produce, so get the template pattern instead.
+    Relations = DeclRelation::TemplatePattern | DeclRelation::Underlying;
+    Decls = getDeclAtPosition(AST, SourceLocationBeg, Relations);
+  }
   if (Decls.empty())
     return nullptr;
 
   const Decl *D = Decls[0];
 
   if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // If this is a variable, use the type of the variable.
     return VD->getType().getTypePtr()->getAsCXXRecordDecl();
   }
 
   if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(D)) {
     // If this is a method, use the type of the class.
     return Method->getParent();
   }
 
   // We don't handle FieldDecl because it's not clear what behaviour
   // the user would expect: the enclosing class type (as with a
   // method), or the field's type (as with a variable).
 
   return dyn_cast<CXXRecordDecl>(D);
 }
 
 std::vector<const CXXRecordDecl *> typeParents(const CXXRecordDecl *CXXRD) {
   std::vector<const CXXRecordDecl *> Result;
 
   for (auto Base : CXXRD->bases()) {
     const CXXRecordDecl *ParentDecl = nullptr;
 
     const Type *Type = Base.getType().getTypePtr();
     if (const RecordType *RT = Type->getAs<RecordType>()) {
       ParentDecl = RT->getAsCXXRecordDecl();
     }
 
     if (!ParentDecl) {
       // Handle a dependent base such as "Base<T>" by using the primary
       // template.
       if (const TemplateSpecializationType *TS =
getTypeHierarchy(ParsedAST &AST, Position Pos, int ResolveLevels,
   if ((Direction == TypeHierarchyDirection::Children ||
        Direction == TypeHierarchyDirection::Both) &&
       ResolveLevels > 0) {
     Result->children.emplace();
 
     if (Index) {
       if (Optional<SymbolID> ID = getSymbolID(CXXRD))
         fillSubTypes(*ID, *Result->children, Index, ResolveLevels, TUPath);
     }
   }
 
   return Result;
 }
 
 void resolveTypeHierarchy(TypeHierarchyItem &Item, int ResolveLevels,
                           TypeHierarchyDirection Direction,
                           const SymbolIndex *Index) {

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

TEST(TypeHierarchy, RecursiveHierarchyUnbounded) {
   TU.ExtraArgs.push_back("-ftemplate-depth=10");
   auto AST = TU.build();
 
   // The compiler should produce a diagnostic for hitting the
   // template instantiation depth.
   ASSERT_TRUE(!AST.getDiagnostics().empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
-  // FIXME(nridge): It would be preferable if the type hierarchy gave us type
-  // names (e.g. "S<0>" for the child and "S<1>" for the parent) rather than
-  // template names (e.g. "S").
+  // Here, we actually don't get any parents, because the unbounded hierarchy
+  // causes instantiation of the base specifier to fail.
   llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
       AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
-  EXPECT_THAT(
-      *Result,
-      AllOf(WithName("S"), WithKind(SymbolKind::Struct),
-            Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
-                          SelectionRangeIs(Source.range("SDef")), Parents()))));
+  EXPECT_THAT(*Result,
+              AllOf(WithName("S<0>"), WithKind(SymbolKind::Struct), Parents()));
 }
 
 TEST(TypeHierarchy, RecursiveHierarchyBounded) {
   Annotations Source(R"cpp(
   template <int N>
   struct $SDef[[S]] : S<N - 1> {};
 
   template <>
TEST(TypeHierarchy, RecursiveHierarchyBounded) {
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion
   // for either a concrete starting point or a dependent starting point.
   llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
       AST, Source.point("SRefConcrete"), 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
   EXPECT_THAT(
       *Result,
-      AllOf(WithName("S"), WithKind(SymbolKind::Struct),
-            Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
-                          SelectionRangeIs(Source.range("SDef")), Parents()))));
+      AllOf(WithName("S<2>"), WithKind(SymbolKind::Struct),
+            Parents(AllOf(
+                WithName("S<1>"), WithKind(SymbolKind::Struct),
+                SelectionRangeIs(Source.range("SDef")),
+                Parents(AllOf(WithName("S<0>"), WithKind(SymbolKind::Struct),
+                              Parents()))))));
   Result = getTypeHierarchy(AST, Source.point("SRefDependent"), 0,
                             TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));
   EXPECT_THAT(
       *Result,
       AllOf(WithName("S"), WithKind(SymbolKind::Struct),
             Parents(AllOf(WithName("S"), WithKind(SymbolKind::Struct),
                           SelectionRangeIs(Source.range("SDef")), Parents()))));
 }
 
 SymbolID findSymbolIDByName(SymbolIndex *Index, llvm::StringRef Name,
                             llvm::StringRef TemplateArgs = "") {
   SymbolID Result;
   FuzzyFindRequest Request;
   Request.Query = Name;
   Request.AnyScope = true;
   bool GotResult = false;
   Index->fuzzyFind(Request, [&](const Symbol &S) {
     if (TemplateArgs == S.TemplateSpecializationArgs) {