[clangd] Print template arguments helper

[kadircet]

Summary

Prepares ground for printing template arguments as written in the
source code, part of re-landing rC356541 with D59599 applied.

[Eugene.Zelenko]

clang-tools-extra/clangd/AST.cpp

28

Functions should be static, not in anonymous namespace. See LLVM Coding Guidelines.

30

Return type is not obvious, so auto should not be used.

121

Return type is not obvious, so auto should not be used.

clang/lib/AST/TypePrinter.cpp

1642

Return type is not obvious, so auto should not be used.

[ioeric]

clang-tools-extra/clangd/AST.cpp

89–90

can we unify this with getTemplateSpecializationArgLocs somehow?

it seems that args as written would also be favorable here (see FIXME in line 112)

121

nit: I'd suggest keeping {} for symmetry.

123

why isn't this handled in getTemplateSpecializationArgLocs? Add a comment?

124

could you add comment/example explaining what's special about this case?

clang-tools-extra/clangd/AST.h

54

Maybe printTemplateSpecializationArgs since we are only handling template specialization?

I think we could drop AsWritten from the name. It should be clear to just explain in the comment.

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

16 ↗

(On Diff #191677)

I think this kind of tests would be more readable using TEST_P [1] with code and expected arguments as parameters.

Up to you though.

[1] https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters

clang/lib/AST/TypePrinter.cpp

1635

could you add clang tests for these changes?

1643

why?

/// Represents an empty template argument, e.g., one that has not
/// been deduced.

It seems legitimate.

Since this hot code path, we would want to avoid hard assertion if possible.

[ioeric]

clang/lib/AST/TypePrinter.cpp

1640

It's unclear to me what the new behavior is with changes in this file. Could you add comment?

It might make sense to split the clang change into a separate patch and get folks who are more familiar to take a look.

[ilya-biryukov]

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

1 ↗

(On Diff #191677)

NIT: add a licence header

clang-tools-extra/unittests/clangd/CMakeLists.txt

13

NIT: maybe call it PrintASTTests?
To avoid creating a dump of test for all the stuff we are too lazy to classify properly

[ilya-biryukov]

clang-tools-extra/clangd/AST.cpp

28

We tend to put internal functions to anonymous namespace quite a lot in clangd.
While technically a small violation of the LLVM style guide, this aligns with the rest of the code and we don't consider that to be a problem.

[kadircet]

clang-tools-extra/clangd/AST.cpp

89–90

See D59641

123

it is mentioned in getTemplateSpecializationArgLocs, would you rather move the comment here?

clang/lib/AST/TypePrinter.cpp

1635

We've also discussed this in the original comment and decided infrastructure necessary was too overwhelming. First we need to invoke compiler and get the AST, then write a Visitor to fetch relevant decls and only after that call printTemplateArguments ...

1640

it just prevents erasure from TypeLoc to Type when printing the argument, so that we can do a fine-grained printing if Location is already there.

1643

Sure, letting TemplateArgument::print handle that

[kadircet]

• Address comments

[Eugene.Zelenko]

clang-tools-extra/clangd/AST.cpp

28

I don't think it's reasonable to have one style per project. Even if clangd has such historical code, it'll be good idea to change it to confirm to common guidelines. Evolution of LLDB formatting style is good example.

[ioeric]

clang-tools-extra/clangd/AST.cpp

89–90

thought?

123

I think it would be clearer if we do something like:

if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
  // handle this specially because ...
} else {
  // use getTemplateSpecializationArgLocs to handle rest of cases.
}

136

I'm not quite sure if I understand this FIXME. In this else branch, we are handling this case. Do you mean this is not a proper fix and future work is needed here? Could you elaborate?

One thing that's worth commenting is why we could use Cls->getTemplateArgs().asArray() when Cls->getTypeAsWritten is null. It's not trivial from the code.

clang/lib/AST/TypePrinter.cpp

1635

No test at all is concerning... I think there are lit tests for AST printing in test/AST/. Is it possible to have some of those?

1640

could you add this into comment?

[kadircet]

• Address comments

clang-tools-extra/clangd/AST.cpp

123

I am not performing this in the order you mentioned since ClassTemplatePartialSpecializationDecl is also a ClassTemplateSpecializationDecl, but if you insist I can change the ordering by adding another condition(which just looks ugly).

136

Yes, this should rather be fixed in AST itself. Updating comment to explain the fallback strategy.

[kadircet]

• Update file comment for PrintASTTests.cpp

[ioeric]

lgtm

clang-tools-extra/clangd/AST.cpp

139

Could you also add a test case for this with the current behavior and a FIXME?

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

48

maybe s/TemplateArgs/TemplateArgsAtPoints/ for clarity

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/AST.cpp

139

Adding the test case, but the problem is there wouldn't be a change in behavior(at least not in the cases that I can think of) since a full specialization doesn't have any dependent types.

[thakis]

clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

16 ↗

(On Diff #194830)

Keep alphabetized?

clang-tools-extra/clangd/AST.h

 /// Returns the first enclosing namespace scope starting from \p DC.
 std::string printNamespaceScope(const DeclContext &DC);
 
 /// Prints unqualified name of the decl for the purpose of displaying it to the
 /// user. Anonymous decls return names of the form "(anonymous {kind})", e.g.
 /// "(anonymous struct)" or "(anonymous namespace)".
 std::string printName(const ASTContext &Ctx, const NamedDecl &ND);
 
+/// Prints template arguments of a decl as written in the source code, including
+/// enclosing '<' and '>', e.g for a partial specialization like: template
+/// <typename U> struct Foo<int, U> will return '<int, U>'. Returns an empty
+/// string if decl is not a template specialization.
+std::string printTemplateSpecializationArgs(const NamedDecl &ND);
+
 /// Gets the symbol ID for a declaration, if possible.
 llvm::Optional<SymbolID> getSymbolID(const Decl *D);
 
 /// Gets the symbol ID for a macro, if possible.
 /// Currently, this is an encoded USR of the macro, which incorporates macro
 /// locations (e.g. file name, offset in file).
 /// FIXME: the USR semantics might not be stable enough as the ID for index
 /// macro (e.g. a change in definition offset can result in a different USR). We

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/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 
+namespace {
+llvm::Optional<llvm::ArrayRef<TemplateArgumentLoc>>
+getTemplateSpecializationArgLocs(const NamedDecl &ND) {
+  if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND)) {
+    if (const ASTTemplateArgumentListInfo *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;
+}
+} // namespace
+
 // Returns true if the complete name of decl \p D is spelled in the source code.
 // This is not the case for:
 //   * symbols formed via macro concatenation, the spelling location will
 //     be "<scratch space>"
 //   * symbols controlled and defined by a compile command-line option
 //     `-DName=foo`, the spelling location will be "<command line>".
 bool isSpelledInSourceCode(const Decl *D) {
   const auto &SM = D->getASTContext().getSourceManager();
std::string printQualifiedName(const NamedDecl &ND) {
   // 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);
   OS.flush();
   assert(!StringRef(QName).startswith("::"));
   return QName;
 }
 
-static const TemplateArgumentList *
-getTemplateSpecializationArgs(const NamedDecl &ND) {
-  if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND))
-    return Func->getTemplateSpecializationArgs();
-  if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND))
-    return &Cls->getTemplateInstantiationArgs();
-  if (auto *Var = llvm::dyn_cast<VarTemplateSpecializationDecl>(&ND))
-    return &Var->getTemplateInstantiationArgs();
-  return nullptr;
-}
-
 std::string printName(const ASTContext &Ctx, const NamedDecl &ND) {
   std::string Name;
   llvm::raw_string_ostream Out(Name);
   PrintingPolicy PP(Ctx.getLangOpts());
   // Handle 'using namespace'. They all have the same name - <using-directive>.
   if (auto *UD = llvm::dyn_cast<UsingDirectiveDecl>(&ND)) {
     Out << "using namespace ";
     if (auto *Qual = UD->getQualifier())
       Qual->print(Out, PP);
     UD->getNominatedNamespaceAsWritten()->printName(Out);
     return Out.str();
   }
   ND.getDeclName().print(Out, PP);
   if (!Out.str().empty()) {
-    // FIXME(ibiryukov): do not show args not explicitly written by the user.
-    if (auto *ArgList = getTemplateSpecializationArgs(ND))
-      printTemplateArgumentList(Out, ArgList->asArray(), PP);
+    Out << printTemplateSpecializationArgs(ND);
     return Out.str();
   }
   // The name was empty, so present an anonymous entity.
   if (isa<NamespaceDecl>(ND))
     return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast<RecordDecl>(&ND))
     return ("(anonymous " + Cls->getKindName() + ")").str();
   if (isa<EnumDecl>(ND))
     return "(anonymous enum)";
   return "(anonymous)";
 }
 
+std::string printTemplateSpecializationArgs(const NamedDecl &ND) {
+  std::string TemplateArgs;
+  llvm::raw_string_ostream OS(TemplateArgs);
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (llvm::Optional<llvm::ArrayRef<TemplateArgumentLoc>> Args =
+          getTemplateSpecializationArgLocs(ND)) {
+    printTemplateArgumentList(OS, *Args, Policy);
+  } else if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
+    if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
+      // ClassTemplateSpecializationDecls do not contain
+      // TemplateArgumentTypeLocs, they only have TemplateArgumentTypes. So we
+      // create a new argument location list from TypeSourceInfo.
+      auto STL = TSI->getTypeLoc().getAs<TemplateSpecializationTypeLoc>();
+      llvm::SmallVector<TemplateArgumentLoc, 8> ArgLocs;
+      ArgLocs.reserve(STL.getNumArgs());
+      for (unsigned I = 0; I < STL.getNumArgs(); ++I)
+        ArgLocs.push_back(STL.getArgLoc(I));
+      printTemplateArgumentList(OS, ArgLocs, Policy);
+    } else {
+      // FIXME: Fix cases when getTypeAsWritten returns null inside clang AST,
+      // e.g. friend decls. Currently we fallback to Template Arguments without
+      // location information.
+      printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
+    }
+  }
+  OS.flush();
+  return TemplateArgs;
+}
+
 std::string printNamespaceScope(const DeclContext &DC) {
   for (const auto *Ctx = &DC; Ctx != nullptr; Ctx = Ctx->getParent())
     if (const auto *NS = dyn_cast<NamespaceDecl>(Ctx))
       if (!NS->isAnonymousNamespace() && !NS->isInlineNamespace())
         return printQualifiedName(*NS) + "::";
   return "";
 }
 

clang-tools-extra/unittests/clangd/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANGD_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clangd REALPATH)
 include_directories(
   ${CLANGD_SOURCE_DIR}
   )
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  PrintASTTests.cpp
   BackgroundIndexTests.cpp
   CancellationTests.cpp
   ClangdTests.cpp
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
   CodeCompletionStringsTests.cpp
   ContextTests.cpp
   DexTests.cpp

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

EXPECT_THAT(
                 Children(AllOf(WithName("x"), WithKind(SymbolKind::Field)))),
           AllOf(WithName("Tmpl<int>"), WithKind(SymbolKind::Struct),
                 Children(WithName("y"))),
           AllOf(WithName("Tmpl<float>"), WithKind(SymbolKind::Struct),
                 Children()),
           AllOf(WithName("Tmpl<double>"), WithKind(SymbolKind::Struct),
                 Children()),
           AllOf(WithName("funcTmpl"), Children()),
-          // FIXME(ibiryukov): template args should be <int> to match the code.
-          AllOf(WithName("funcTmpl<int, double, float>"), Children()),
+          AllOf(WithName("funcTmpl<int>"), Children()),
           AllOf(WithName("varTmpl"), Children()),
-          // FIXME(ibiryukov): template args should be <int> to match the code.
-          AllOf(WithName("varTmpl<int, double>"), Children())));
+          AllOf(WithName("varTmpl<int>"), Children())));
 }
 
 TEST_F(DocumentSymbolsTest, Namespaces) {
   std::string FilePath = testPath("foo.cpp");
   addFile(FilePath, R"cpp(
       namespace ans1 {
         int ai1;
       namespace ans2 {

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

+//===--- ASTUtilsTests.cpp ----------------------------------------- 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 "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector<const char *> Expected;
+};
+class ASTUtils : public testing::Test,
+                 public ::testing::WithParamInterface<Case> {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor<Visitor> {
+    Visitor(std::vector<Position> Points) : Points(std::move(Points)) {}
+    bool VisitNamedDecl(const NamedDecl *ND) {
+      auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+                                     ND->getLocation());
+      if (Pos != Points[TemplateArgs.size()])
+        return true;
+      TemplateArgs.push_back(printTemplateSpecializationArgs(*ND));
+      return true;
+    }
+    std::vector<std::string> TemplateArgs;
+    const std::vector<Position> Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgs, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+                        testing::ValuesIn(std::vector<Case>({
+                            {
+                                R"cpp(
+                                  template <class X> class Bar {};
+                                  template <> class ^Bar<double> {};)cpp",
+                                {"<double>"}},
+                            {
+                                R"cpp(
+                                  template <class X> class Bar {};
+                                  template <class T, class U,
+                                  template<typename> class Z, int Q>
+                                  struct Foo {};
+                                  template struct ^Foo<int, bool, Bar, 8>;
+                                  template <typename T>
+                                  struct ^Foo<T *, T, Bar, 3> {};)cpp",
+                                {"<int, bool, Bar, 8>", "<T *, T, Bar, 3>"}},
+                            {
+                                R"cpp(
+                                  template <int ...> void Foz() {};
+                                  template <> void ^Foz<3, 5, 8>() {};)cpp",
+                                {"<3, 5, 8>"}},
+                            {
+                                R"cpp(
+                                  template <class X> class Bar {};
+                                  template <template <class> class ...>
+                                  class Aux {};
+                                  template <> class ^Aux<Bar, Bar> {};
+                                  template <template <class> T>
+                                  class ^Aux<T, T> {};)cpp",
+                                {"<Bar, Bar>", "<T, T>"}},
+                            {
+                                R"cpp(
+                                  template <typename T> T var = 1234;
+                                  template <> int ^var<int> = 1;)cpp",
+                                {"<int>"}},
+                        })));
+} // namespace
+} // namespace clangd
+} // namespace clang

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) {
+  const TemplateArgument::ArgKind &Kind = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+    return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 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);
+      // Tries to print the argument with location info if exists.
+      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 << ' ';