[clangd] Make our printing policies for Hover more consistent, especially tags

[sammccall]

Summary

Different cases were using a bunch of different variants of the printing policy.
Each of these had something going for it, but the result was inconsistent.

Goals:

• single printing policy used (almost) everywhere
• avoid unidiomatic tags like class vector<class X>
• be informative and easy to understand

For tags, the solution I wound up with is: we print only the outer tag and only
in the simplest cases where this elaboration won't cause confusion.

For example:

• class X
• enum Foo
• vector<int>
• X*

This seems to strike a nice balance of providing plenty of info/context in common
cases while never being confusing.

[qchateau]

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (class Foo* instead of Foo*). I don't think Foo* is much more understandable than Foo, so we decide that printing class Foo is easier to understand for the user, we should print class Foo* as well.
But I agree it's even less idiomatic, so I won't argue with your choice.

[sammccall]

In D93553#2463577, @qchateau wrote:

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (class Foo* instead of Foo*). I don't think Foo* is much more understandable than Foo, so we decide that printing class Foo is easier to understand for the user, we should print class Foo* as well.
But I agree it's even less idiomatic, so I won't argue with your choice.

Yeah, class Foo * doesn't seem bad, though for some reason my brain struggles with class Foo && :-)
I really think I like drawing the line *somewhere* in terms of complexity, but maybe it's not in the ideal place.

There's a pragmatic reason to draw it here: simply prepending "class " doesn't work in the presence of qualifiers. So the current patch will print class Foo but const Foo, but at least the latter is rare. The inconsistency between class Foo* and const Foo* would be a bigger deal, I think. And getting const class Foo* without also producing class Foo<class Bar> would need changes to TypePrinter, which is a bit of a yak-shave.

So I think I'll land this as an improvement over the status quo, at least.

clang-tools-extra/clangd/Hover.cpp

 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
-  PrintingPolicy Policy(Base);
-
-  Policy.AnonymousTagLocations = false;
-  Policy.TerseOutput = true;
-  Policy.PolishForDeclaration = true;
-  Policy.ConstantsAsWritten = true;
-  Policy.SuppressTagKeyword = false;
-
-  return Policy;
+PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
+  Base.AnonymousTagLocations = false;
+  Base.TerseOutput = true;
+  Base.PolishForDeclaration = true;
+  Base.ConstantsAsWritten = true;
+  Base.SuppressTemplateArgsInCXXConstructors = true;
+  return Base;
 }
 
 /// Given a declaration \p D, return a human-readable string representing the
 /// local scope in which it is declared, i.e. class(es) and method name. Returns
 /// an empty string if it is not local.
 std::string getLocalScope(const Decl *D) {
   std::vector<std::string> Scopes;
   const DeclContext *DC = D->getDeclContext();
if (NSD->isInline() || NSD->isAnonymousNamespace())
       return getNamespaceScope(NSD);
   }
   if (const NamedDecl *ND = dyn_cast<NamedDecl>(DC))
     return printQualifiedName(*ND);
 
   return "";
 }
 
-std::string printDefinition(const Decl *D) {
+std::string printDefinition(const Decl *D, const PrintingPolicy &PP) {
   std::string Definition;
   llvm::raw_string_ostream OS(Definition);
-  PrintingPolicy Policy =
-      printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
-  Policy.IncludeTagDefinition = false;
-  Policy.SuppressTemplateArgsInCXXConstructors = true;
-  Policy.SuppressTagKeyword = true;
-  D->print(OS, Policy);
+  D->print(OS, PP);
   OS.flush();
   return Definition;
 }
 
-std::string printType(QualType QT, const PrintingPolicy &Policy) {
+std::string printType(QualType QT, const PrintingPolicy &PP) {
   // TypePrinter doesn't resolve decltypes, so resolve them here.
   // FIXME: This doesn't handle composite types that contain a decltype in them.
   // We should rather have a printing policy for that.
   while (!QT.isNull() && QT->isDecltypeType())
     QT = QT->getAs<DecltypeType>()->getUnderlyingType();
-  return QT.getAsString(Policy);
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  // Special case: if the outer type is a tag type without qualifiers, then
+  // include the tag for extra clarity.
+  // This isn't very idiomatic, so don't attempt it for complex cases, including
+  // pointers/references, template specializations, etc.
+  if (!QT.isNull() && !QT.hasQualifiers() && PP.SuppressTagKeyword) {
+    if (auto *TT = llvm::dyn_cast<TagType>(QT.getTypePtr()))
+      OS << TT->getDecl()->getKindName() << " ";
+  }
+  OS.flush();
+  QT.print(OS, PP);
+  return Result;
 }
 
 std::string printType(const TemplateTypeParmDecl *TTP) {
   std::string Res = TTP->wasDeclaredWithTypename() ? "typename" : "class";
   if (TTP->isParameterPack())
     Res += "...";
   return Res;
 }
const Expr *getDefaultArg(const ParmVarDecl *PVD) {
   // the expression is still valid.
   if (!PVD->hasDefaultArg() || PVD->hasUnparsedDefaultArg())
     return nullptr;
   return PVD->hasUninstantiatedDefaultArg() ? PVD->getUninstantiatedDefaultArg()
                                             : PVD->getDefaultArg();
 }
 
 HoverInfo::Param toHoverInfoParam(const ParmVarDecl *PVD,
-                                  const PrintingPolicy &Policy) {
+                                  const PrintingPolicy &PP) {
   HoverInfo::Param Out;
-  Out.Type = printType(PVD->getType(), Policy);
+  Out.Type = printType(PVD->getType(), PP);
   if (!PVD->getName().empty())
     Out.Name = PVD->getNameAsString();
   if (const Expr *DefArg = getDefaultArg(PVD)) {
     Out.Default.emplace();
     llvm::raw_string_ostream OS(*Out.Default);
-    DefArg->printPretty(OS, nullptr, Policy);
+    DefArg->printPretty(OS, nullptr, PP);
   }
   return Out;
 }
 
 // Populates Type, ReturnType, and Parameters for function-like decls.
 void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
                                const FunctionDecl *FD,
-                               const PrintingPolicy &Policy) {
+                               const PrintingPolicy &PP) {
   HI.Parameters.emplace();
   for (const ParmVarDecl *PVD : FD->parameters())
-    HI.Parameters->emplace_back(toHoverInfoParam(PVD, Policy));
+    HI.Parameters->emplace_back(toHoverInfoParam(PVD, PP));
 
   // We don't want any type info, if name already contains it. This is true for
   // constructors/destructors and conversion operators.
   const auto NK = FD->getDeclName().getNameKind();
   if (NK == DeclarationName::CXXConstructorName ||
       NK == DeclarationName::CXXDestructorName ||
       NK == DeclarationName::CXXConversionFunctionName)
     return;
 
-  HI.ReturnType = printType(FD->getReturnType(), Policy);
+  HI.ReturnType = printType(FD->getReturnType(), PP);
   QualType QT = FD->getType();
   if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) // Lambdas
     QT = VD->getType().getDesugaredType(D->getASTContext());
-  HI.Type = printType(QT, Policy);
+  HI.Type = printType(QT, PP);
   // FIXME: handle variadics.
 }
 
 llvm::Optional<std::string> printExprValue(const Expr *E,
                                            const ASTContext &Ctx) {
   // InitListExpr has two forms, syntactic and semantic. They are the same thing
   // (refer to a same AST node) in most cases.
   // When they are different, RAV returns the syntactic form, and we should feed
if (CMD->getDeclName().isIdentifier() && !CMD->isStatic() &&
       if (const auto SetterField = setterVariableName(CMD))
         return llvm::formatv("Trivial setter for `{0}`.", *SetterField);
     }
   }
   return "";
 }
 
 /// Generate a \p Hover object given the declaration \p D.
-HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
+HoverInfo getHoverContents(const NamedDecl *D, const PrintingPolicy &PP,
+                           const SymbolIndex *Index) {
   HoverInfo HI;
   const ASTContext &Ctx = D->getASTContext();
 
   HI.AccessSpecifier = getAccessSpelling(D->getAccess()).str();
   HI.NamespaceScope = getNamespaceScope(D);
   if (!HI.NamespaceScope->empty())
     HI.NamespaceScope->append("::");
   HI.LocalScope = getLocalScope(D);
   if (!HI.LocalScope.empty())
     HI.LocalScope.append("::");
 
-  PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
   HI.Name = printName(Ctx, *D);
   const auto *CommentD = getDeclForComment(D);
   HI.Documentation = getDeclComment(Ctx, *CommentD);
   enhanceFromIndex(HI, *CommentD, Index);
   if (HI.Documentation.empty())
     HI.Documentation = synthesizeDocumentation(D);
 
   HI.Kind = index::getSymbolInfo(D).Kind;
 
   // Fill in template params.
   if (const TemplateDecl *TD = D->getDescribedTemplate()) {
     HI.TemplateParameters =
-        fetchTemplateParameters(TD->getTemplateParameters(), Policy);
+        fetchTemplateParameters(TD->getTemplateParameters(), PP);
     D = TD;
   } else if (const FunctionDecl *FD = D->getAsFunction()) {
     if (const auto *FTD = FD->getDescribedTemplate()) {
       HI.TemplateParameters =
-          fetchTemplateParameters(FTD->getTemplateParameters(), Policy);
+          fetchTemplateParameters(FTD->getTemplateParameters(), PP);
       D = FTD;
     }
   }
 
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
-    fillFunctionTypeAndParams(HI, D, FD, Policy);
+    fillFunctionTypeAndParams(HI, D, FD, PP);
   else if (const auto *VD = dyn_cast<ValueDecl>(D))
-    HI.Type = printType(VD->getType(), Policy);
+    HI.Type = printType(VD->getType(), PP);
   else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
     HI.Type = TTP->wasDeclaredWithTypename() ? "typename" : "class";
   else if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(D))
-    HI.Type = printType(TTP, Policy);
+    HI.Type = printType(TTP, PP);
 
   // Fill in value with evaluated initializer if possible.
   if (const auto *Var = dyn_cast<VarDecl>(D)) {
     if (const Expr *Init = Var->getInit())
       HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(D)) {
     // Dependent enums (e.g. nested in template classes) don't have values yet.
     if (!ECD->getType()->isDependentType())
       HI.Value = ECD->getInitVal().toString(10);
   }
 
-  HI.Definition = printDefinition(D);
+  HI.Definition = printDefinition(D, PP);
   return HI;
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
 HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) {
   HoverInfo HI;
   SourceManager &SM = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
if (!Invalid) {
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
 
 llvm::Optional<HoverInfo> getThisExprHoverContents(const CXXThisExpr *CTE,
-                                                   ASTContext &ASTCtx) {
+                                                   ASTContext &ASTCtx,
+                                                   const PrintingPolicy &PP) {
   QualType OriginThisType = CTE->getType()->getPointeeType();
   QualType ClassType = declaredType(OriginThisType->getAsTagDecl());
   // For partial specialization class, origin `this` pointee type will be
   // parsed as `InjectedClassNameType`, which will ouput template arguments
   // like "type-parameter-0-0". So we retrieve user written class type in this
   // case.
   QualType PrettyThisType = ASTCtx.getPointerType(
       QualType(ClassType.getTypePtr(), OriginThisType.getCVRQualifiers()));
 
-  auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
-  Policy.SuppressTagKeyword = true;
-  Policy.SuppressScope = true;
   HoverInfo HI;
   HI.Name = "this";
-  HI.Definition = PrettyThisType.getAsString(Policy);
+  HI.Definition = printType(PrettyThisType, PP);
   return HI;
 }
 
 /// Generate a HoverInfo object given the deduced type \p QT
 HoverInfo getDeducedTypeHoverContents(QualType QT, const syntax::Token &Tok,
                                       ASTContext &ASTCtx,
+                                      const PrintingPolicy &PP,
                                       const SymbolIndex *Index) {
   HoverInfo HI;
   // FIXME: distinguish decltype(auto) vs decltype(expr)
   HI.Name = tok::getTokenName(Tok.kind());
   HI.Kind = index::SymbolKind::TypeAlias;
 
-  auto PP = printingPolicyForDecls(ASTCtx.getLangOpts());
-
   if (QT->isUndeducedAutoType()) {
     HI.Definition = "/* not deduced */";
   } else {
-    HI.Definition = QT.getAsString(PP);
+    HI.Definition = printType(QT, PP);
 
     if (const auto *D = QT->getAsTagDecl()) {
       const auto *CommentD = getDeclForComment(D);
       HI.Documentation = getDeclComment(ASTCtx, *CommentD);
       enhanceFromIndex(HI, *CommentD, Index);
     }
   }
 
llvm::StringLiteral getNameForExpr(const Expr *E) {
   // TODO: Once GCC5 is fully retired and not the minimal requirement as stated
   // in `GettingStarted`, please remove the explicit conversion constructor.
   return llvm::StringLiteral("expression");
 }
 
 // Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
 llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
+                                           const PrintingPolicy &PP,
                                            const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
     return llvm::None;
 
   HoverInfo HI;
   // For `this` expr we currently generate hover with pointee type.
   if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E))
-    return getThisExprHoverContents(CTE, AST.getASTContext());
+    return getThisExprHoverContents(CTE, AST.getASTContext(), PP);
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
-    auto Policy =
-        printingPolicyForDecls(AST.getASTContext().getPrintingPolicy());
-    Policy.SuppressTagKeyword = true;
-    HI.Type = printType(E->getType(), Policy);
+    HI.Type = printType(E->getType(), PP);
     HI.Value = *Val;
     HI.Name = std::string(getNameForExpr(E));
     return HI;
   }
   return llvm::None;
 }
 
 bool isParagraphBreak(llvm::StringRef Rest) {
if (const auto *FD = llvm::dyn_cast<FieldDecl>(&ND)) {
     }
     return;
   }
 }
 
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
-                           const PrintingPolicy &Policy) {
+                           const PrintingPolicy &PP) {
   const auto &OuterNode = N->outerImplicit();
   if (!OuterNode.Parent)
     return;
   const auto *CE = OuterNode.Parent->ASTNode.get<CallExpr>();
   if (!CE)
     return;
   const FunctionDecl *FD = CE->getDirectCallee();
   // For non-function-call-like operatators (e.g. operator+, operator<<) it's
   // not immediattely obvious what the "passed as" would refer to and, given
   // fixed function signature, the value would be very low anyway, so we choose
   // to not support that.
   // Both variadic functions and operator() (especially relevant for lambdas)
   // should be supported in the future.
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
     return;
 
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
     if (CE->getArg(I) != OuterNode.ASTNode.get<Expr>())
       continue;
 
     // Extract matching argument from function declaration.
     if (const ParmVarDecl *PVD = FD->getParamDecl(I))
-      HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
+      HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
     break;
   }
   if (!HI.CalleeArgInfo)
     return;
 
   // If we found a matching argument, also figure out if it's a
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
   HI.CallPassType.emplace(PassType);
 }
 
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
+  PrintingPolicy PP =
+      getPrintingPolicy(AST.getASTContext().getPrintingPolicy());
   const SourceManager &SM = AST.getSourceManager();
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     llvm::consumeError(CurLoc.takeError());
     return llvm::None;
   }
   const auto &TB = AST.getTokens();
   auto TokensTouchingCursor = syntax::spelledTokensTouching(*CurLoc, TB);
if (Tok.kind() == tok::identifier) {
       // Prefer the identifier token as a fallback highlighting range.
       HighlightRange = Tok.range(SM).toCharRange(SM);
       if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) {
         HI = getHoverContents(*M, AST);
         break;
       }
     } else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
       if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
-        HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(),
+        HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(), PP,
                                          Index);
         HighlightRange = Tok.range(SM).toCharRange(SM);
         break;
       }
 
       // If we can't find interesting hover information for this
       // auto/decltype keyword, return nothing to avoid showing
       // irrelevant or incorrect informations.
       return llvm::None;
     }
   }
 
   // If it wasn't auto/decltype or macro, look for decls and expressions.
   if (!HI) {
     auto Offset = SM.getFileOffset(*CurLoc);
     // Editors send the position on the left of the hovered character.
     // So our selection tree should be biased right. (Tested with VSCode).
     SelectionTree ST =
         SelectionTree::createRight(AST.getASTContext(), TB, Offset, Offset);
     std::vector<const Decl *> Result;
     if (const SelectionTree::Node *N = ST.commonAncestor()) {
       // FIXME: Fill in HighlightRange with range coming from N->ASTNode.
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
       if (!Decls.empty()) {
-        HI = getHoverContents(Decls.front(), Index);
+        HI = getHoverContents(Decls.front(), PP, Index);
         // Layout info only shown when hovering on the field/class itself.
         if (Decls.front() == N->ASTNode.get<Decl>())
           addLayoutInfo(*Decls.front(), *HI);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
-        maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
+        maybeAddCalleeArgInfo(N, *HI, PP);
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
-        HI = getHoverContents(E, AST, Index);
+        HI = getHoverContents(E, AST, PP, Index);
       }
       // FIXME: support hovers for other nodes?
       //  - built-in types
     }
   }
 
   if (!HI)
     return llvm::None;

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

class Foo {})cpp";
         }
         )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.LocalScope = "foo::";
          HI.Name = "c";
          HI.Kind = index::SymbolKind::Variable;
          HI.Definition = "auto *c = &b";
-         HI.Type = "class (lambda) **";
+         HI.Type = "(lambda) **";
          HI.ReturnType = "bool";
          HI.Parameters = {
              {std::string("int"), std::string("T"), llvm::None},
              {std::string("bool"), std::string("B"), llvm::None},
          };
          return HI;
        }},
       // Lambda parameter with decltype reference
class Foo {})cpp";
           class X;
           void foo() {
             [[^auto]] x = Foo<X>();
           }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "auto";
             HI.Kind = index::SymbolKind::TypeAlias;
-            HI.Definition = "class Foo<class X>";
+            HI.Definition = "class Foo<X>";
           }},
       {// Falls back to primary template, when the type is not instantiated.
        R"cpp(
           // comment from primary
           template <typename T> class Foo {};
           // comment from specialization
           template <typename T> class Foo<T*> {};
           void foo() {
class Foo {})cpp";
           )cpp",
        [](HoverInfo &HI) {
          HI.Name = "setY";
          HI.Kind = index::SymbolKind::InstanceMethod;
          HI.NamespaceScope = "";
          HI.Definition = "X &setY(float v)";
          HI.LocalScope = "X::";
          HI.Documentation = "Trivial setter for `Y`.";
-         HI.Type = "struct X &(float)";
-         HI.ReturnType = "struct X &";
+         HI.Type = "X &(float)";
+         HI.ReturnType = "X &";
          HI.Parameters.emplace();
          HI.Parameters->emplace_back();
          HI.Parameters->back().Type = "float";
          HI.Parameters->back().Name = "v";
          HI.AccessSpecifier = "public";
        }},
       {// Setter (move)
        R"cpp(
const std::function<void(HoverInfo &)> ExpectedBuilder;
             struct Test {};
             void test() { Test && [[te^st]] = {}; }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "test";
             HI.Kind = index::SymbolKind::Variable;
             HI.NamespaceScope = "";
             HI.LocalScope = "test::";
-            HI.Type = "struct Test &&";
+            HI.Type = "Test &&";
             HI.Definition = "Test &&test = {}";
           }},
       {
           R"cpp(// auto on alias
           typedef int int_type;
           ^[[auto]] x = int_type();
           )cpp",
           [](HoverInfo &HI) {

               Foo* bar() {
                 return [[t^his]];
               }
             };
           }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "this";
-            HI.Definition = "Foo *";
+            HI.Definition = "ns::Foo *";
           }},
       {
           R"cpp(// this expr for template class
           namespace ns {
             template <typename T>
             class Foo {
               Foo* bar() const {
                 return [[t^his]];