[clangd] Add TemplateArgumentList into Symbol

[kadircet]

Summary

Part of re-landing rC356541 with D59599. Changes the way we store
template arguments, previous patch was storing them inside Name field of Symbol.
Which was violating the assumption:

Symbol::Scope+Symbol::Name == clang::clangd::printQualifiedName

which was made in multiple places inside codebase. This patch instead moves
those arguments into their own field. Currently the field is meant to be
human-readable, can be made structured if need be.

[ioeric]

should we update YAML?

clang-tools-extra/clangd/index/Symbol.h

48

How about TemplateSpecializationArgs?

Could you also put this field near ReturnType or Signature?

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

526

put this near ReturnType initialization.

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

396

this test is getting hard to read. could you only make minimum change to the existing test and add a new case for the new behavior?

[kadircet]

• Address comments

[kadircet]

In D59640#1438248, @ioeric wrote:

should we update YAML?

I suppose we are only keeping it alive for the sake of tests, but that seems like an enough reason updating that as well.

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

526

tried to move it to closest place I can get, can't put after this if since template specialization params are not indexed for code completion, we'll simply end up dropping those.

[kadircet]

Ping

[ilya-biryukov]

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

566

Any reason to not always fill in this field?

[kadircet]

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

566

It is simply because printTemplateSpecializationArgs should return an empty string in other cases. Since we mark all explicit specializations as *not* indexed for code completion.

[ilya-biryukov]

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

566

NIT: That's is not obvious. Could we fill it right after Name and Scope?
These 3 fields are related, therefore feels natural to have code filling them together.

[kadircet]

• Fill in the TemplateSpecializationArgs in all code paths.

[ilya-biryukov]

LGTM

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

 void writeSymbol(const Symbol &Sym, const StringTableOut &Strings,
                  llvm::raw_ostream &OS) {
   OS << Sym.ID.raw(); // TODO: once we start writing xrefs and posting lists,
                       // symbol IDs should probably be in a string table.
   OS.write(static_cast<uint8_t>(Sym.SymInfo.Kind));
   OS.write(static_cast<uint8_t>(Sym.SymInfo.Lang));
   writeVar(Strings.index(Sym.Name), OS);
   writeVar(Strings.index(Sym.Scope), OS);
+  writeVar(Strings.index(Sym.TemplateSpecializationArgs), OS);
   writeLocation(Sym.Definition, Strings, OS);
   writeLocation(Sym.CanonicalDeclaration, Strings, OS);
   writeVar(Sym.References, OS);
   OS.write(static_cast<uint8_t>(Sym.Flags));
   OS.write(static_cast<uint8_t>(Sym.Origin));
   writeVar(Strings.index(Sym.Signature), OS);
   writeVar(Strings.index(Sym.CompletionSnippetSuffix), OS);
   writeVar(Strings.index(Sym.Documentation), OS);

 
 Symbol readSymbol(Reader &Data, llvm::ArrayRef<llvm::StringRef> Strings) {
   Symbol Sym;
   Sym.ID = Data.consumeID();
   Sym.SymInfo.Kind = static_cast<index::SymbolKind>(Data.consume8());
   Sym.SymInfo.Lang = static_cast<index::SymbolLanguage>(Data.consume8());
   Sym.Name = Data.consumeString(Strings);
   Sym.Scope = Data.consumeString(Strings);
+  Sym.TemplateSpecializationArgs = Data.consumeString(Strings);
   Sym.Definition = readLocation(Data, Strings);
   Sym.CanonicalDeclaration = readLocation(Data, Strings);
   Sym.References = Data.consumeVar();
   Sym.Flags = static_cast<Symbol::SymbolFlag>(Data.consumeVar());
   Sym.Origin = static_cast<SymbolOrigin>(Data.consumeVar());
   Sym.Signature = Data.consumeString(Strings);
   Sym.CompletionSnippetSuffix = Data.consumeString(Strings);
   Sym.Documentation = Data.consumeString(Strings);

clang-tools-extra/clangd/index/Symbol.h

struct Symbol {
   index::SymbolInfo SymInfo = index::SymbolInfo();
   /// The unqualified name of the symbol, e.g. "bar" (for ns::bar).
   llvm::StringRef Name;
   /// The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
   llvm::StringRef Scope;
   /// The location of the symbol's definition, if one was found.
   /// This just covers the symbol name (e.g. without class/function body).
   SymbolLocation Definition;
   /// The location of the preferred declaration of the symbol.
   /// This just covers the symbol name.
   /// This may be the same as Definition.
   ///
   /// A C++ symbol may have multiple declarations, and we pick one to prefer.
   ///   * For classes, the canonical declaration should be the definition.
   ///   * For non-inline functions, the canonical declaration typically appears
   ///     in the ".h" file corresponding to the definition.
   SymbolLocation CanonicalDeclaration;
   /// The number of translation units that reference this symbol from their main
   /// file. This number is only meaningful if aggregated in an index.
   unsigned References = 0;
   /// Where this symbol came from. Usually an index provides a constant value.
   SymbolOrigin Origin = SymbolOrigin::Unknown;
   /// A brief description of the symbol that can be appended in the completion
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
   /// Only set when the symbol is indexed for completion.
   llvm::StringRef Signature;
+  /// Argument list in human-readable format, will be displayed to help
+  /// disambiguate between different specializations of a template. Empty for
+  /// non-specializations. Example: "<int, bool, 3>"
+  llvm::StringRef TemplateSpecializationArgs;
   /// What to insert when completing this symbol, after the symbol name.
   /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
   /// (When snippets are disabled, the symbol name alone is used).
   /// Only set when the symbol is indexed for completion.
   llvm::StringRef CompletionSnippetSuffix;
   /// Documentation including comment for the symbol declaration.
   llvm::StringRef Documentation;
   /// Type when this symbol is used in an expression. (Short display form).

 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, Symbol::SymbolFlag);
 
 /// Invokes Callback with each StringRef& contained in the Symbol.
 /// Useful for deduplicating backing strings.
 template <typename Callback> void visitStrings(Symbol &S, const Callback &CB) {
   CB(S.Name);
   CB(S.Scope);
+  CB(S.TemplateSpecializationArgs);
   CB(S.Signature);
   CB(S.CompletionSnippetSuffix);
   CB(S.Documentation);
   CB(S.ReturnType);
   CB(S.Type);
   auto RawCharPointerCB = [&CB](const char *&P) {
     llvm::StringRef S(P);
     CB(S);

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

 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
                                               bool IsMainFileOnly) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
   Symbol S;
   S.ID = std::move(ID);
   std::string QName = printQualifiedName(ND);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
 
   // We collect main-file symbols, but do not use them for code completion.
   if (!IsMainFileOnly && isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
   if (isImplementationDetail(&ND))
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
       *ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
       *CompletionTUInfo,
       /*IncludeBriefComments*/ false);
   std::string Documentation =
       formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
                                               /*CommentsFromHeaders=*/true));
   if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
+    std::string TemplateSpecializationArgs =
+        printTemplateSpecializationArgs(ND);
+    S.TemplateSpecializationArgs = TemplateSpecializationArgs;
     if (Opts.StoreAllDocumentation)
       S.Documentation = Documentation;
     Symbols.insert(S);
     return Symbols.find(S.ID);
   }
   S.Documentation = Documentation;
   std::string Signature;
   std::string SnippetSuffix;

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

static void mapping(IO &IO, Symbol &Sym) {
     IO.mapRequired("SymInfo", Sym.SymInfo);
     IO.mapOptional("CanonicalDeclaration", Sym.CanonicalDeclaration,
                    SymbolLocation());
     IO.mapOptional("Definition", Sym.Definition, SymbolLocation());
     IO.mapOptional("References", Sym.References, 0u);
     IO.mapOptional("Origin", NSymbolOrigin->Origin);
     IO.mapOptional("Flags", NSymbolFlag->Flag);
     IO.mapOptional("Signature", Sym.Signature);
+    IO.mapOptional("TemplateSpecializationArgs",
+                   Sym.TemplateSpecializationArgs);
     IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix);
     IO.mapOptional("Documentation", Sym.Documentation);
     IO.mapOptional("ReturnType", Sym.ReturnType);
     IO.mapOptional("Type", Sym.Type);
     IO.mapOptional("IncludeHeaders", Sym.IncludeHeaders);
   }
 };
 

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

MATCHER_P(Labeled, Label, "") {
   return (arg.Name + arg.Signature).str() == Label;
 }
 MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
 MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
 MATCHER_P(Snippet, S, "") {
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(TemplateArgs, TemplArgs, "") {
+  return arg.TemplateSpecializationArgs == TemplArgs;
+}
 MATCHER_P(DeclURI, P, "") {
   return StringRef(arg.CanonicalDeclaration.FileURI) == P;
 }
 MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
          (arg.IncludeHeaders.begin()->IncludeHeader == P);
 }
EXPECT_THAT(Symbols,
                   AllOf(QName("bar"), VisibleOutsideFile()),
                   AllOf(QName("a"), Not(VisibleOutsideFile())),
                   AllOf(QName("B"), Not(VisibleOutsideFile())),
                   AllOf(QName("c"), Not(VisibleOutsideFile())),
                   // 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>;
   )");
   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))));
 }
 
+TEST_F(SymbolCollectorTest, TemplateArgs) {
+  Annotations Header(R"(
+    template <class X> class $barclasstemp[[Bar]] {};
+    template <class T, class U, template<typename> class Z, int Q>
+    struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+
+    // template-template, non-type and type full spec
+    template <> struct $specdecl[[Tmpl]]<int, bool, Bar, 3> {};
+
+    // template-template, non-type and type partial spec
+    template <class U, int T> struct $partspecdecl[[Tmpl]]<bool, U, Bar, T> {};
+    // instantiation
+    extern template struct Tmpl<float, bool, Bar, 8>;
+    // instantiation
+    template struct Tmpl<double, bool, Bar, 2>;
+
+    template <typename ...> class $fooclasstemp[[Foo]] {};
+    // parameter-packs full spec
+    template<> class $parampack[[Foo]]<Bar<int>, int, double> {};
+    // parameter-packs partial spec
+    template<class T> class $parampackpartial[[Foo]]<T, T> {};
+
+    template <int ...> class $bazclasstemp[[Baz]] {};
+    // non-type parameter-packs full spec
+    template<> class $parampacknontype[[Baz]]<3, 5, 8> {};
+    // non-type parameter-packs partial spec
+    template<int T> class $parampacknontypepartial[[Baz]]<T, T> {};
+
+    template <template <class> class ...> class $fozclasstemp[[Foz]] {};
+    // template-template parameter-packs full spec
+    template<> class $parampacktempltempl[[Foz]]<Bar, Bar> {};
+    // template-template parameter-packs partial spec
+    template<template <class> class T>
+    class $parampacktempltemplpartial[[Foz]]<T, T> {};
+  )");
+  runSymbolCollector(Header.code(), /*Main=*/"");
+  EXPECT_THAT(
+      Symbols,
+      AllOf(
+          Contains(AllOf(QName("Tmpl"), TemplateArgs("<int, bool, Bar, 3>"),
+                         DeclRange(Header.range("specdecl")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Tmpl"), TemplateArgs("<bool, U, Bar, T>"),
+                         DeclRange(Header.range("partspecdecl")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Foo"), TemplateArgs("<Bar<int>, int, double>"),
+                         DeclRange(Header.range("parampack")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Foo"), TemplateArgs("<T, T>"),
+                         DeclRange(Header.range("parampackpartial")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Baz"), TemplateArgs("<3, 5, 8>"),
+                         DeclRange(Header.range("parampacknontype")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Baz"), TemplateArgs("<T, T>"),
+                         DeclRange(Header.range("parampacknontypepartial")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Foz"), TemplateArgs("<Bar, Bar>"),
+                         DeclRange(Header.range("parampacktempltempl")),
+                         ForCodeCompletion(false))),
+          Contains(AllOf(QName("Foz"), TemplateArgs("<T, T>"),
+                         DeclRange(Header.range("parampacktempltemplpartial")),
+                         ForCodeCompletion(false)))));
+}
+
 TEST_F(SymbolCollectorTest, ObjCSymbols) {
   const std::string Header = R"(
     @interface Person
     - (void)someMethodName:(void*)name1 lastName:(void*)lName;
     @end
 
     @implementation Person
     - (void)someMethodName:(void*)name1 lastName:(void*)lName{