[llvm-branch-commits] [clang-tools-extra] b061564 - [clangd] Make our printing policies for Hover more consistent, especially tags

Sam McCall via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Fri Dec 18 16:08:33 PST 2020


Author: Sam McCall
Date: 2020-12-19T00:52:55+01:00
New Revision: b0615642f647bea1483659f1e14515a836015254

URL: https://github.com/llvm/llvm-project/commit/b0615642f647bea1483659f1e14515a836015254
DIFF: https://github.com/llvm/llvm-project/commit/b0615642f647bea1483659f1e14515a836015254.diff

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

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.

Differential Revision: https://reviews.llvm.org/D93553

Added: 
    

Modified: 
    clang-tools-extra/clangd/Hover.cpp
    clang-tools-extra/clangd/unittests/HoverTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index b5eda93ddbbc..6d707c8d1521 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -49,16 +49,13 @@ 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
@@ -108,26 +105,33 @@ std::string getNamespaceScope(const Decl *D) {
   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) {
@@ -291,15 +295,15 @@ const Expr *getDefaultArg(const ParmVarDecl *PVD) {
 }
 
 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;
 }
@@ -307,10 +311,10 @@ HoverInfo::Param toHoverInfoParam(const ParmVarDecl *PVD,
 // 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.
@@ -320,11 +324,11 @@ void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
       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.
 }
 
@@ -492,7 +496,8 @@ std::string synthesizeDocumentation(const NamedDecl *ND) {
 }
 
 /// 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();
 
@@ -504,7 +509,6 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
   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);
@@ -517,25 +521,25 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
   // 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)) {
@@ -547,7 +551,7 @@ HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
       HI.Value = ECD->getInitVal().toString(10);
   }
 
-  HI.Definition = printDefinition(D);
+  HI.Definition = printDefinition(D, PP);
   return HI;
 }
 
@@ -587,7 +591,8 @@ HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) {
 }
 
 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
@@ -597,30 +602,26 @@ llvm::Optional<HoverInfo> getThisExprHoverContents(const CXXThisExpr *CTE,
   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);
@@ -657,6 +658,7 @@ llvm::StringLiteral getNameForExpr(const Expr *E) {
 // 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.
@@ -666,14 +668,11 @@ llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
   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;
@@ -744,7 +743,7 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
 // 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;
@@ -768,7 +767,7 @@ void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
 
     // 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)
@@ -832,6 +831,8 @@ void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
 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) {
@@ -863,7 +864,7 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
       }
     } 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;
@@ -888,16 +889,16 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
       // 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

diff  --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index b72b500ee8b5..9650940b66cb 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -239,7 +239,7 @@ class Foo {})cpp";
          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},
@@ -611,7 +611,7 @@ class Foo {})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(
@@ -718,8 +718,8 @@ class Foo {})cpp";
          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";
@@ -1943,7 +1943,7 @@ TEST(Hover, All) {
             HI.Kind = index::SymbolKind::Variable;
             HI.NamespaceScope = "";
             HI.LocalScope = "test::";
-            HI.Type = "struct Test &&";
+            HI.Type = "Test &&";
             HI.Definition = "Test &&test = {}";
           }},
       {
@@ -2211,7 +2211,7 @@ TEST(Hover, All) {
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "this";
-            HI.Definition = "Foo *";
+            HI.Definition = "ns::Foo *";
           }},
       {
           R"cpp(// this expr for template class


        


More information about the llvm-branch-commits mailing list