[clangd] Print arguments in template specializations

[ilya-biryukov]

clang-tools-extra/clangd/AST.cpp

59

For ClassTemplateSpecializationDecl, could we try extracting the arguments from the result of getTypeAsWritten?

clang/lib/AST/TypePrinter.cpp

1636

NIT: clang-format the diff

1644

Maybe print the result of getTypeLoc() here, if it's available?
Would produce results closer to the ones written in the code.

[kadircet]

• Address comments
• Add template specializations to fuzzyFind results

[kadircet]

clang/lib/AST/TypePrinter.cpp

1644

unfortunately it is not available.

[kadircet]

• Get rid of debug printing
• Update tests

[ilya-biryukov]

Mostly nits, but could you elaborate why can't we print the type-loc when printing the template argument (see the corresponding comment)?

clang-tools-extra/clangd/AST.cpp

66

NIT: add a note that we return null for ClassTemplateSpecializationDecl because we can't construct an ArrayRef for it

86

NIT: use SmallVector<8> or some other small-enough number to avoid most allocs.

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

18

NIT: maybe remove it? clangd keeps adding those, but I don't think we actually want it: gmock.h should be enough

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

417

Does it mean typing bool could potentially match vector<bool> in workspaceSymbols now?
If that's the case we might start producing some noise.

clang/lib/AST/TypePrinter.cpp

1644

you mean the function to print a type loc or the type loc itself?

1646

Maybe simplify the switch to:

if (A.getKind() == TemplateArgument::ArgKind::Type) {
    A.getTypeSourceInfo()->getType().print(OS, PP);
    return;
}
A.getArgument().print(PP, OS);

[kadircet]

clang-tools-extra/clangd/AST.cpp

86

calling reserve beforehand

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

18

Should we add IWYU pragmas to those files? https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h

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

417

unfortunately yes it does, what do you suggest? it seems like we can perform a "isprefix" check for symbols with template specs kind?

clang/lib/AST/TypePrinter.cpp

1644

function to print a typeloc

[kadircet]

• Address some of the comments

[kadircet]

clang/lib/AST/TypePrinter.cpp

1646

It was rather to catch any changes in the ArgKind at compile-time, still can do if you think this should not cause any problems

[ilya-biryukov]

The only important comment is about test coverage

clang-tools-extra/clangd/AST.cpp

86

SmallVector is even better because it keeps the elements on the stack and won't need to do dynamic allocations (most of the time).
My suggestion would be to use it here anyway.

Up to you, though.

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

18

I think we should, but some projects don't like those pragmas in their headers.
Trying to add this won't hurt for sure!

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

417

Let's land this and see if this creates too much noise.
If it will, we might want to drop the argument lists from the words we match and only keep them for presentation to the user.

But let's not worry about this too much for now, the current behavior LG unless proven otherwise :-)

clang/lib/AST/TypePrinter.cpp

1646

I think listing all the cases here does not actually buy us anything since we call a function that should somehow print the arguments anyway.

But up to you.

1655

Now that you mentioned other kinds of parameters, could you add tests for other kinds of parameters?
In particular, I expect we'll definitely need to handle packs here and probably all other kinds of arguments too.

• Non-type and template template parameters.

template <int I, int J, template<class> class TT, template <class> class JJ>
struct Foo {};

template <int X, template <class> class UU>
struct Foo<X, X, UU, UU> {};

• Parameter packs.

template <class ...T>
struct Bar {};

template <class T>
struct Bar<T, T> {};

template <int ...I>
struct Baz {};

template <int I>
struct Baz<I, I> {};

template <template <class> class...TT>
struct Qux {};

template <template <class> class TT>
struct Qux<TT, TT> {};

[ilya-biryukov]

clang/lib/AST/TypePrinter.cpp

1644

Thanks for the clarification.
Intuitively, I've expected it to be there, but it seems that we can only print the types.

[kadircet]

• Add more tests
• Replace switch with if
• Use SmallVector

clang/lib/AST/TypePrinter.cpp

1655

I've already had tests for these cases, but adding a few more for partial specs as well.

[ilya-biryukov]

LGTM with a few NITs.
Thanks for fixing this!

clang-tools-extra/clangd/AST.cpp

85

NIT: maybe inline TL?

if (auto STL = Cls->getTypeAsWritten()->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
  /// ...
}

88

Tiny NIT: prefer preincrement

clang/lib/AST/TypePrinter.cpp

1635

NIT: consider also adding a test in the clang code.
Some developers don't build tools-extra.

[ilya-biryukov]

clang/lib/AST/TypePrinter.cpp

1635

It seems the amount of test infrastructure for running this in clang might be overwhelming.
It's just not worth it probably, having a test in clangd is good enough!

[kadircet]

• Address comments

clang-tools-extra/clangd/AST.cpp

 //===--- AST.cpp - Utility AST functions  -----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/TemplateBase.h"
+#include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 namespace clang {
 namespace clangd {

 SourceLocation findNameLoc(const clang::Decl *D) {
   const auto &SM = D->getASTContext().getSourceManager();
   if (!isSpelledInSourceCode(D))
     // Use the expansion location as spelling location is not interesting.
     return SM.getExpansionRange(D->getLocation()).getBegin();
   return SM.getSpellingLoc(D->getLocation());
 }
 
+static llvm::Optional<llvm::ArrayRef<TemplateArgumentLoc>>
+getTemplateSpecializationArgLocs(const NamedDecl &ND) {
+  if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND)) {
+    if (auto *Args = Func->getTemplateSpecializationArgsAsWritten())
+      return Args->arguments();
+  } else if (auto *Cls =
+                 llvm::dyn_cast<ClassTemplatePartialSpecializationDecl>(&ND)) {
+    if (auto *Args = Cls->getTemplateArgsAsWritten())
+      return Args->arguments();
+  } else if (auto *Var = llvm::dyn_cast<VarTemplateSpecializationDecl>(&ND))
+    return Var->getTemplateArgsInfo().arguments();
+  // We return None for ClassTemplateSpecializationDecls because it does not
+  // contain TemplateArgumentLoc information.
+  return llvm::None;
+}
+
 std::string printQualifiedName(const NamedDecl &ND) {
   std::string QName;
   llvm::raw_string_ostream OS(QName);
   PrintingPolicy Policy(ND.getASTContext().getLangOpts());
   // Note that inline namespaces are treated as transparent scopes. This
   // reflects the way they're most commonly used for lookup. Ideally we'd
   // include them, but at query time it's hard to find all the inline
   // namespaces to query: the preamble doesn't have a dedicated list.
   Policy.SuppressUnwrittenScope = true;
   ND.printQualifiedName(OS, Policy);
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+    printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
+    auto TL = Cls->getTypeAsWritten()->getTypeLoc();
+    if (auto STL = TL.getAs<TemplateSpecializationTypeLoc>()) {
+      std::vector<TemplateArgumentLoc> ArgLocs;
+      ArgLocs.reserve(STL.getNumArgs());
+      for (unsigned I = 0; I < STL.getNumArgs(); I++)
+        ArgLocs.push_back(STL.getArgLoc(I));
+      printTemplateArgumentList(OS, ArgLocs, Policy);
+    }
+  }
   OS.flush();
   assert(!StringRef(QName).startswith("::"));
   return QName;
 }
 
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl &ND) {
   if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND))

clang-tools-extra/clangd/index/MemIndex.cpp

bool MemIndex::fuzzyFind(
 
   TopN<std::pair<float, const Symbol *>> Top(
       Req.Limit ? *Req.Limit : std::numeric_limits<size_t>::max());
   FuzzyMatcher Filter(Req.Query);
   bool More = false;
   for (const auto Pair : Index) {
     const Symbol *Sym = Pair.second;
 
-    // FIXME: Enable fuzzy find on template specializations once we start
-    // storing template arguments in the name. Currently we only store name for
-    // class template, which would cause duplication in the results.
-    if (Sym->SymInfo.Properties &
-        (static_cast<index::SymbolPropertySet>(
-             index::SymbolProperty::TemplateSpecialization) |
-         static_cast<index::SymbolPropertySet>(
-             index::SymbolProperty::TemplatePartialSpecialization)))
-      continue;
     // Exact match against all possible scopes.
     if (!Req.AnyScope && !llvm::is_contained(Req.Scopes, Sym->Scope))
       continue;
     if (Req.RestrictForCodeCompletion &&
         !(Sym->Flags & Symbol::IndexedForCodeCompletion))
       continue;
 
     if (auto Score = Filter.match(Sym->Name))

clang-tools-extra/clangd/index/dex/Dex.cpp

for (size_t I = 0; I < ScoredSymbols.size(); ++I) {
     SymbolQuality[I] = ScoredSymbols[I].first;
     Symbols[I] = ScoredSymbols[I].second;
   }
 
   // Populate TempInvertedIndex with lists for index symbols.
   llvm::DenseMap<Token, std::vector<DocID>> TempInvertedIndex;
   for (DocID SymbolRank = 0; SymbolRank < Symbols.size(); ++SymbolRank) {
     const auto *Sym = Symbols[SymbolRank];
-    // FIXME: Enable fuzzy find on template specializations once we start
-    // storing template arguments in the name. Currently we only store name for
-    // class template, which would cause duplication in the results.
-    if (Sym->SymInfo.Properties &
-        (static_cast<index::SymbolPropertySet>(
-             index::SymbolProperty::TemplateSpecialization) |
-         static_cast<index::SymbolPropertySet>(
-             index::SymbolProperty::TemplatePartialSpecialization)))
-      continue;
     for (const auto &Token : generateSearchTokens(*Sym))
       TempInvertedIndex[Token].push_back(SymbolRank);
   }
 
   // Convert lists of items to posting lists.
   for (const auto &TokenToPostingList : TempInvertedIndex)
     InvertedIndex.insert(
         {TokenToPostingList.first, PostingList(TokenToPostingList.second)});

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

TEST(DexTest, PreferredTypesBoosting) {
   Req.PreferredTypes = {Sym2.Type};
   EXPECT_THAT(match(I, Req), ElementsAre("t2"));
 }
 
 TEST(DexTest, TemplateSpecialization) {
   SymbolSlab::Builder B;
 
   Symbol S = symbol("TempSpec");
-  S.ID = SymbolID("0");
   B.insert(S);
 
-  S = symbol("TempSpec");
-  S.ID = SymbolID("1");
+  S = symbol("TempSpec<int, bool>");
   S.SymInfo.Properties = static_cast<index::SymbolPropertySet>(
       index::SymbolProperty::TemplateSpecialization);
   B.insert(S);
 
-  S = symbol("TempSpec");
-  S.ID = SymbolID("2");
+  S = symbol("TempSpec<int, U>");
   S.SymInfo.Properties = static_cast<index::SymbolPropertySet>(
       index::SymbolProperty::TemplatePartialSpecialization);
   B.insert(S);
 
   auto I = dex::Dex::build(std::move(B).build(), RefSlab());
   FuzzyFindRequest Req;
-  Req.Query = "TempSpec";
   Req.AnyScope = true;
 
-  std::vector<Symbol> Symbols;
-  I->fuzzyFind(Req, [&Symbols](const Symbol &Sym) { Symbols.push_back(Sym); });
-  EXPECT_EQ(Symbols.size(), 1U);
-  EXPECT_FALSE(Symbols.front().SymInfo.Properties &
-               static_cast<index::SymbolPropertySet>(
-                   index::SymbolProperty::TemplateSpecialization));
-  EXPECT_FALSE(Symbols.front().SymInfo.Properties &
-               static_cast<index::SymbolPropertySet>(
-                   index::SymbolProperty::TemplatePartialSpecialization));
+  Req.Query = "TempSpec";
+  EXPECT_THAT(match(*I, Req),
+              UnorderedElementsAre("TempSpec", "TempSpec<int, bool>",
+                                   "TempSpec<int, U>"));
+
+  Req.Query = "TempSpec<int";
+  EXPECT_THAT(match(*I, Req),
+              UnorderedElementsAre("TempSpec<int, bool>", "TempSpec<int, U>"));
 }
 
 } // namespace
 } // namespace dex
 } // namespace clangd
 } // namespace clang

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

 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/MemIndex.h"
 #include "index/Merge.h"
 #include "index/Symbol.h"
 #include "clang/Index/IndexSymbol.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using testing::_;
 using testing::AllOf;
 using testing::AnyOf;
 using testing::ElementsAre;
 using testing::Pair;
 using testing::Pointee;
EXPECT_THAT(lookup(*I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}),
               UnorderedElementsAre("ns::xyz"));
   EXPECT_THAT(lookup(*I, SymbolID("ns::nonono")), UnorderedElementsAre());
 }
 
 TEST(MemIndexTest, TemplateSpecialization) {
   SymbolSlab::Builder B;
 
   Symbol S = symbol("TempSpec");
-  S.ID = SymbolID("0");
   B.insert(S);
 
-  S = symbol("TempSpec");
-  S.ID = SymbolID("1");
+  S = symbol("TempSpec<int, bool>");
   S.SymInfo.Properties = static_cast<index::SymbolPropertySet>(
       index::SymbolProperty::TemplateSpecialization);
   B.insert(S);
 
-  S = symbol("TempSpec");
-  S.ID = SymbolID("2");
+  S = symbol("TempSpec<int, U>");
   S.SymInfo.Properties = static_cast<index::SymbolPropertySet>(
       index::SymbolProperty::TemplatePartialSpecialization);
   B.insert(S);
 
   auto I = MemIndex::build(std::move(B).build(), RefSlab());
   FuzzyFindRequest Req;
-  Req.Query = "TempSpec";
   Req.AnyScope = true;
 
-  std::vector<Symbol> Symbols;
-  I->fuzzyFind(Req, [&Symbols](const Symbol &Sym) { Symbols.push_back(Sym); });
-  EXPECT_EQ(Symbols.size(), 1U);
-  EXPECT_FALSE(Symbols.front().SymInfo.Properties &
-               static_cast<index::SymbolPropertySet>(
-                   index::SymbolProperty::TemplateSpecialization));
-  EXPECT_FALSE(Symbols.front().SymInfo.Properties &
-               static_cast<index::SymbolPropertySet>(
-                   index::SymbolProperty::TemplatePartialSpecialization));
+  Req.Query = "TempSpec";
+  EXPECT_THAT(match(*I, Req),
+              UnorderedElementsAre("TempSpec", "TempSpec<int, bool>",
+                                   "TempSpec<int, U>"));
+
+  Req.Query = "TempSpec<int";
+  EXPECT_THAT(match(*I, Req),
+              UnorderedElementsAre("TempSpec<int, bool>", "TempSpec<int, U>"));
 }
 
 TEST(MergeIndexTest, Lookup) {
   auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"}), RefSlab()),
        J = MemIndex::build(generateSymbols({"ns::B", "ns::C"}), RefSlab());
   MergedIndex M(I.get(), J.get());
   EXPECT_THAT(lookup(M, SymbolID("ns::A")), UnorderedElementsAre("ns::A"));
   EXPECT_THAT(lookup(M, SymbolID("ns::B")), UnorderedElementsAre("ns::B"));

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

EXPECT_THAT(Symbols,
                   // FIXME: ForwardDecl likely *is* visible outside.
                   AllOf(QName("ForwardDecl"), Not(VisibleOutsideFile()))));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
     // Primary template and explicit specialization are indexed, instantiation
     // is not.
-    template <class T, class U> struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-    template <> struct $specdecl[[Tmpl]]<int, bool> {};
-    template <class U> struct $partspecdecl[[Tmpl]]<bool, U> {};
-    extern template struct Tmpl<float, bool>;
-    template struct Tmpl<double, bool>;
+    template <class X> class $barclasstemp[[Bar]] {};
+    template <class T, class U, template<typename> class Z, int Q>
+    struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+    template <> struct $specdecl[[Tmpl]]<int, bool, Bar, 3> {};
+    template <class U, int T> struct $partspecdecl[[Tmpl]]<bool, U, Bar, T> {};
+    extern template struct Tmpl<float, bool, Bar, 8>;
+    template struct Tmpl<double, bool, Bar, 2>;
+    template <typename ...> class $fooclasstemp[[Foo]] {};
+    template<> class $templtempl[[Foo]]<Bar<int>, int, double> {};
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(
-                  AllOf(QName("Tmpl"), DeclRange(Header.range()),
-                        ForCodeCompletion(true)),
-                  AllOf(QName("Tmpl"), DeclRange(Header.range("specdecl")),
-                        ForCodeCompletion(false)),
-                  AllOf(QName("Tmpl"), DeclRange(Header.range("partspecdecl")),
-                        ForCodeCompletion(false)),
-                  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
-                        ForCodeCompletion(false))));
+  EXPECT_THAT(
+      Symbols,
+          AllOf(QName("Bar"), DeclRange(Header.range("barclasstemp")),
+                ForCodeCompletion(true)),
+          AllOf(QName("Foo"), DeclRange(Header.range("fooclasstemp")),
+                ForCodeCompletion(true)),
+          AllOf(QName("Tmpl<int, bool, Bar, 3>"),
+                DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),
+          AllOf(QName("Tmpl<bool, U, Bar, T>"),
+                DeclRange(Header.range("partspecdecl")),
+          AllOf(QName("Foo<Bar<int>, int, double>"),
+                DeclRange(Header.range("templtempl")),
 }
 
 TEST_F(SymbolCollectorTest, ObjCSymbols) {
   const std::string Header = R"(
     @interface Person
     - (void)someMethodName:(void*)name1 lastName:(void*)lName;
     @end
 

clang/lib/AST/TypePrinter.cpp

 
 static
 const TemplateArgument &getArgument(const TemplateArgument &A) { return A; }
 
 static const TemplateArgument &getArgument(const TemplateArgumentLoc &A) {
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+                          llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+                          const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  switch (A.getArgument().getKind()) {
+  case TemplateArgument::ArgKind::Null:
+    assert(false && "TemplateArgumentKind can not be null!");
+    break;
+  case TemplateArgument::ArgKind::Type:
+    A.getTypeSourceInfo()->getType().print(OS, PP);
+    break;
+  case TemplateArgument::ArgKind::Declaration:
+  case TemplateArgument::ArgKind::NullPtr:
+  case TemplateArgument::ArgKind::Integral:
+  case TemplateArgument::ArgKind::Template:
+  case TemplateArgument::ArgKind::TemplateExpansion:
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+    A.getArgument().print(PP, OS);
+    break;
+  }
+}
+
 template<typename TA>
 static void printTo(raw_ostream &OS, ArrayRef<TA> Args,
                     const PrintingPolicy &Policy, bool SkipBrackets) {
   const char *Comma = Policy.MSVCFormatting ? "," : ", ";
   if (!SkipBrackets)
     OS << '<';
 
   bool NeedSpace = false;
   bool FirstArg = true;
   for (const auto &Arg : Args) {
     // Print the argument into a string.
     SmallString<128> Buf;
     llvm::raw_svector_ostream ArgOS(Buf);
     const TemplateArgument &Argument = getArgument(Arg);
     if (Argument.getKind() == TemplateArgument::Pack) {
       if (Argument.pack_size() && !FirstArg)
         OS << Comma;
       printTo(ArgOS, Argument.getPackAsArray(), Policy, true);
     } else {
       if (!FirstArg)
         OS << Comma;
-      Argument.print(Policy, ArgOS);
+      printArgument(Arg, Policy, ArgOS);
     }
     StringRef ArgString = ArgOS.str();
 
     // If this is the first argument and its string representation
     // begins with the global scope specifier ('::foo'), add a space
     // to avoid printing the diagraph '<:'.
     if (FirstArg && !ArgString.empty() && ArgString[0] == ':')
       OS << ' ';