[clangd] Omit default template arguments from type hints

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/InlayHints.cpp

359–360

can we document why we need to policies here, probably something like:

We want to suppress default template arguments and print canonical types, unfortunately they're conflicting policies hence we can't have both.
For regular types, suppressing default template arguments is more important, whereas printing canonical types is crucial for structured bindings. Therefore we use 2 separate policies.

[nridge]

Address review comment

clang-tools-extra/clangd/InlayHints.cpp

 namespace clangd {
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
       : Results(Results), AST(AST.getASTContext()),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
-        TypeHintPolicy(this->AST.getPrintingPolicy()) {
+        TypeHintPolicy(this->AST.getPrintingPolicy()),
+        StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
     MainFileBuf = Invalid ? StringRef{} : Buf;
 
     TypeHintPolicy.SuppressScope = true; // keep type names short
     TypeHintPolicy.AnonymousTagLocations =
         false; // do not print lambda locations
-    // Print canonical types. Otherwise, SuppressScope would result in
-    // things like "metafunction<args>::type" being shorted to just "type",
-    // which is useless. This is particularly important for structured
-    // bindings that use the tuple_element protocol, where the non-canonical
-    // types would be "tuple_element<I, A>::type".
-    // Note, for "auto", we would often prefer sugared types, but the AST
-    // doesn't currently retain them in DeducedType anyways.
-    TypeHintPolicy.PrintCanonicalTypes = true;
+
+    // For structured bindings, print canonical types. This is important because
+    // for bindings that use the tuple_element protocol, the non-canonical types
+    // would be "tuple_element<I, A>::type".
+    // For "auto", we often prefer sugared types, but the AST doesn't currently
+    // retain them in DeducedType. However, not setting PrintCanonicalTypes for
+    // "auto" at least allows SuppressDefaultTemplateArgs (set by default) to
+    // have an effect.
+    StructuredBindingPolicy = TypeHintPolicy;
+    StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // Weed out constructor calls that don't look like a function call with
     // an argument list, by checking the validity of getParenOrBraceRange().
     // Also weed out std::initializer_list constructors as there are no names
     // for the individual arguments.
     if (!E->getParenOrBraceRange().isValid() ||
bool VisitFunctionDecl(FunctionDecl *D) {
     return true;
   }
 
   bool VisitVarDecl(VarDecl *D) {
     // Do not show hints for the aggregate in a structured binding,
     // but show hints for the individual bindings.
     if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
       for (auto *Binding : DD->bindings()) {
-        addTypeHint(Binding->getLocation(), Binding->getType(), ": ");
+        addTypeHint(Binding->getLocation(), Binding->getType(), ": ",
+                    StructuredBindingPolicy);
       }
       return true;
     }
 
     if (D->getType()->getContainedAutoType()) {
       if (!D->getType()->isDependentType()) {
         // Our current approach is to place the hint on the variable
         // and accordingly print the full type
void addInlayHint(SourceRange R, InlayHintKind Kind, llvm::StringRef Label) {
     Results.push_back(InlayHint{
         Range{
             sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
             sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())},
         Kind, Label.str()});
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+    addTypeHint(R, T, Prefix, TypeHintPolicy);
+  }
+
+  void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
+                   const PrintingPolicy &Policy) {
     // Do not print useless "NULL TYPE" hint.
     if (!T.getTypePtrOrNull())
       return;
 
-    std::string TypeName = T.getAsString(TypeHintPolicy);
+    std::string TypeName = T.getAsString(Policy);
     if (TypeName.length() < TypeNameLimit)
       addInlayHint(R, InlayHintKind::TypeHint, std::string(Prefix) + TypeName);
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
+  // We want to suppress default template arguments, but otherwise print
+  // canonical types. Unfortunately, they're conflicting policies so we can't
+  // have both. For regular types, suppressing template arguments is more
+  // important, whereas printing canonical types is crucial for structured
+  // bindings, so we use two separate policies. (See the constructor where
+  // the policies are initialized for more details.)
   PrintingPolicy TypeHintPolicy;
+  PrintingPolicy StructuredBindingPolicy;
 
   static const size_t TypeNameLimit = 32;
 };
 
 std::vector<InlayHint> inlayHints(ParsedAST &AST) {
   std::vector<InlayHint> Results;
   InlayHintVisitor Visitor(Results, AST);
   Visitor.TraverseAST(AST.getASTContext());
   return Results;
 }
 
 } // namespace clangd
 } // namespace clang

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

assertTypeHints(R"cpp(
     struct A {};
     struct MultipleWords {};
     A<MultipleWords, MultipleWords, MultipleWords> foo();
     // Omit type hint past a certain length (currently 32)
     auto var = foo();
   )cpp");
 }
 
+TEST(TypeHints, DefaultTemplateArgs) {
+  assertTypeHints(R"cpp(
+    template <typename, typename = int>
+    struct A {};
+    A<float> foo();
+    auto $var[[var]] = foo();
+  )cpp",
+                  ExpectedHint{": A<float>", "var"});
+}
+
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
 //  - auto x = static_cast<TypeName>(...);  // and other built-in casts
 
 // Annoyances for which a heuristic is not obvious:
 //  - auto x = llvm::dyn_cast<LongTypeName>(y);  // and similar
 //  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
 //    (For this one, perhaps we should omit type hints that start
 //     with a double underscore.)
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 No newline at end of file