[clangd] Parameter hints for template specialization

[v1nh1shungry]

Summary

Relevant issue: https://github.com/clangd/clangd/issues/1387

[v1nh1shungry]

This patch implements:

• Template specialization type

• Class template specialization declaration

• Variable template specialization declaration

• Function specialization declaration

It would be sweet to also implement:

template <class T> constexpr int value = 0;
int I = value</*T: */int>;

template <class T> T add(T lhs, T rhs);
add</*T: */int>(1, 2);

But I got stuck on implementing these. I tried VisitDeclRefExpr but failed and have no idea. I prefer to implement these in future patches. But yes, if anyone could give me some hints I am happy to update the patch.

[sammccall]

The code looks pretty good and this makes a lot of logical sense.

However, there's an ugly practical issue: it's overwhelmingly common for template parameters to have cryptic and useless names.
It seems like turning vector<int> into vector</*Tp: */int> actually makes things worse.

And this gets yet worse: because this is type parameter and controlled by that config option, so people can't disable this without disabling the (very practically useful) function param hints.

Have you been using this for a while? Do you find it to be a practical help? Spammy?

clang-tools-extra/clangd/InlayHints.cpp

378

we need a bailout if (!Cfg.InlayHints.Parameters)

434

we're missing some mangling of the name to remove the leading _
(stripLeadingUnderscore?)

553

I think we should avoid running the printer on every arg. Instead probably switch over the types of templatearguments, and delegate to shouldHintName(Expr, handle TagType/Typedef/other common nodes specifically, as done in shouldHintName).

[v1nh1shungry]

@sammccall Thank you for reviewing and giving suggestions!

I must admit I didn't use it for very long. But I do think this is helpful, at least for templates I'm unfamiliar with.

Yes, there is a common situation where people use a meaningless template parameter name, but I think the same for functions. I have seen many meaningless parameter names like D, E even in the LLVM codebase. Since we can tolerate these, why can't we bear the template parameter?

And yes, it is a serious problem this is unconfigurable.

clang-tools-extra/clangd/InlayHints.cpp

434

I don't think we should, since we don't do this for function parameters, are there any special reasons for us to do this for template parameters?

[sammccall]

In D138425#3941183, @v1nh1shungry wrote:

@sammccall Thank you for reviewing and giving suggestions!

I must admit I didn't use it for very long. But I do think this is helpful, at least for templates I'm unfamiliar with.

That sounds good, maybe you can give some examples?
I'm thinking about a heuristic like "show hints where the param name is >2 chars long" or something, at least as a starting point - would be good to see whether this would still provide the value you're seeing.

Yes, there is a common situation where people use a meaningless template parameter name, but I think the same for functions.

I don't think it's the same: my wild guess would be 30% of function params have useless names, and 90% of template params do.
If this were accurate, it seems function param hints are usually (potentially) useful, while template params are almost always useless.

I have seen many meaningless parameter names like D, E even in the LLVM codebase.

Agree. LLVM does this (much) more than the other codebases I'm familiar with.
I'd personally be very happy if we could somehow detect and suppress hints for such params.
But it's not clear-cut: template<class K, class V> class map can be meaningful, and with push-to-show-hints interaction the lack of a hint can be confusing.

Since we can tolerate these, why can't we bear the template parameter?

If template params are useless a larger fraction of the time, then enabling them without doing some filtering might be a net negative in practice.

And yes, it is a serious problem this is unconfigurable.

We can add a new config category for these (use Config::InlayHints::TemplateParameters), it's just a bit ugly to do so.

clang-tools-extra/clangd/InlayHints.cpp

434

We do this for function params, see line 574 of the LHS

[tom-anders]

In D138425#3941206, @sammccall wrote:

In D138425#3941183, @v1nh1shungry wrote:

Yes, there is a common situation where people use a meaningless template parameter name, but I think the same for functions.

I don't think it's the same: my wild guess would be 30% of function params have useless names, and 90% of template params do.
If this were accurate, it seems function param hints are usually (potentially) useful, while template params are almost always useless.

I agree that is usually useless and too much noise, especially for container like types. But, it could maybe be useful for template parameters that have a default value? For example, I'll never need an inlay hint for something like `std::map<K, V>, but when passing a custom comparator and allocator as the 3rd and 4th template type, inlay hints might actually be helpful there.

[nridge]

FWIW, in codebases/components where I do code reviews, I tend to insist on template parameters having meaningful (and longer-than-one-character) names, and would find this quite useful.

I agree that while template parameters are technically a kind of parameter, they're different enough from function parameters that configuring this type of hint separately (and thus giving them their own kind) would make sense.

[v1nh1shungry]

Sorry for the delay.

I'd say I was convinced and about to abandon this patch.

Sincere apologies for wasting everyone's time.

clang-tools-extra/clangd/InlayHints.cpp

public:
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *FPT =
             llvm::dyn_cast<FunctionProtoType>(D->getType().getTypePtr())) {
       if (!FPT->hasTrailingReturn()) {
         if (auto FTL = D->getFunctionTypeLoc())
           addReturnTypeHint(D, FTL.getRParenLoc());
       }
     }
+    if (D->isFunctionTemplateSpecialization()) {
+      auto TP = D->getPrimaryTemplate()->getTemplateParameters()->asArray();
+      if (auto *Args = D->getTemplateSpecializationArgsAsWritten())
+        addTemplateArgHint(TP, Args->arguments());
+    }
     return true;
   }
 
   bool VisitLambdaExpr(LambdaExpr *E) {
     FunctionDecl *D = E->getCallOperator();
     if (!E->hasExplicitResultType())
       addReturnTypeHint(D, E->hasExplicitParameters()
                                ? D->getFunctionTypeLoc().getRParenLoc()
for (const Expr *Init : Syn->inits()) {
       auto It = Designators.find(Init->getBeginLoc());
       if (It != Designators.end() &&
           !isPrecededByParamNameComment(Init, It->second))
         addDesignatorHint(Init->getSourceRange(), It->second);
     }
     return true;
   }
 
+  bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc L) {
+    auto TP = L.getTypePtr()
+                  ->getTemplateName()
+                  .getAsTemplateDecl()
+                  ->getTemplateParameters()
+                  ->asArray();
+    llvm::SmallVector<TemplateArgumentLoc, 1> TA;
+    for (std::size_t I = 0; I < L.getNumArgs(); ++I)
+      TA.push_back(L.getArgLoc(I));
+    addTemplateArgHint(TP, TA);
+    return true;
+  }
+
+  bool VisitConceptSpecializationExpr(const ConceptSpecializationExpr *E) {
+    auto TP = E->getNamedConcept()->getTemplateParameters()->asArray();
+    auto TA = E->getTemplateArgsAsWritten()->arguments();
+    addTemplateArgHint(TP, TA);
+    return true;
+  }
+
+  bool VisitClassTemplatePartialSpecializationDecl(
+      const ClassTemplatePartialSpecializationDecl *D) {
+    auto TP = D->getSpecializedTemplate()->getTemplateParameters()->asArray();
+    auto TA = D->getTemplateArgsAsWritten()->arguments();
+    addTemplateArgHint(TP, TA);
+    return true;
+  }
+
+  bool
+  VisitVarTemplateSpecializationDecl(const VarTemplateSpecializationDecl *D) {
+    auto TP = D->getSpecializedTemplate()->getTemplateParameters()->asArray();
+    if (const auto *VTPSD =
+            llvm::dyn_cast<VarTemplatePartialSpecializationDecl>(D))
+      addTemplateArgHint(TP, VTPSD->getTemplateArgsAsWritten()->arguments());
+    else
+      addTemplateArgHint(TP, D->getTemplateArgsInfo()->arguments());
+    return true;
+  }
+
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
+  void addTemplateArgHint(llvm::ArrayRef<NamedDecl *> TP,
+                          llvm::ArrayRef<TemplateArgumentLoc> TA) {
+    std::size_t I = 0;
+    for (; I < TA.size(); ++I) {
+      const auto &Arg = TA[I].getArgument();
+      // Pack expansion expressions cause the 1:1 mapping between arguments and
+      // parameters to break down, so we don't add further inlay hints if we
+      // encounter one.
+      if (Arg.isPackExpansion())
+        return;
+      if (TP[I]->isTemplateParameterPack())
+        break;
+      if (auto Name = TP[I]->getName(); shouldHintName(TA[I], Name))
+        addInlayHint(TA[I].getSourceRange(), HintSide::Left,
+                     InlayHintKind::Parameter, "", Name, ": ");
+    }
+    // Handle the parameter pack, if there is one
+    if (I < TA.size()) {
+      auto Name = TP[I]->getName();
+      for (std::size_t J = 0; I < TA.size(); ++I, ++J) {
+        const auto &Arg = TA[I].getArgument();
+        if (Arg.isPackExpansion())
+          return;
+        addInlayHint(TA[I].getSourceRange(), HintSide::Left,
+                     InlayHintKind::Parameter, "",
+                     llvm::formatv("{0}[{1}]", Name, J).str(), ": ");
+      }
+    }
+  }
+
   void processCall(const FunctionDecl *Callee,
                    llvm::ArrayRef<const Expr *> Args) {
     if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
       return;
 
     // The parameter name of a move or copy constructor is not very interesting.
     if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
       if (Ctor->isCopyOrMoveConstructor())
bool shouldHintName(const Expr *Arg, StringRef ParamName) {
 
     // Exclude argument expressions preceded by a /*paramName*/.
     if (isPrecededByParamNameComment(Arg, ParamName))
       return false;
 
     return true;
   }
 
+  bool shouldHintName(const TemplateArgumentLoc &TA, StringRef ParamName) {
+    if (ParamName.empty())
+      return false;
+    std::string ArgContent;
+    llvm::raw_string_ostream Out(ArgContent);
+    TA.getArgument().print(AST.getPrintingPolicy(), Out, false);
+    if (ArgContent == ParamName)
+      return false;
+    return true;
+  }
+
   bool shouldHintReference(const ParmVarDecl *Param,
                            const ParmVarDecl *ForwardedParam) {
     // We add a & hint only when the argument is passed as mutable reference.
     // For parameters that are not part of an expanded pack, this is
     // straightforward. For expanded pack parameters, it's likely that they will
     // be forwarded to another function. In this situation, we only want to add
     // the reference hint if the argument is actually being used via mutable
     // reference. This means we need to check

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

TEST(ParameterHints, VariadicNameFromSpecialization) {
   // specialization.
   assertParameterHints(R"cpp(
     void foo(int a);
     template <typename... Args>
     void bar(Args... args) {
       foo(args...);
     }
     template <>
-    void bar<int>(int b);
+    void bar<$par0[[int]]>(int b);
     void baz() {
-      bar($param[[42]]);
+      bar($par1[[42]]);
     }
   )cpp",
-                       ExpectedHint{"b: ", "param"});
+                       ExpectedHint{"Args[0]: ", "par0"},
+                       ExpectedHint{"b: ", "par1"});
 }
 
 TEST(ParameterHints, VariadicNameFromSpecializationRecursive) {
   // We don't try to resolve forwarding parameters inside a forwarding function
   // call if that function call uses a specialization.
   assertParameterHints(R"cpp(
     void foo2(int a);
     template <typename... Args>
     void foo(Args... args) {
       foo2(args...);
     }
     template <typename... Args>
     void bar(Args... args) {
       foo(args...);
     }
     template <>
-    void foo<int>(int b);
+    void foo<$par0[[int]]>(int b);
     void baz() {
-      bar($param[[42]]);
+      bar($par1[[42]]);
     }
   )cpp",
-                       ExpectedHint{"b: ", "param"});
+                       ExpectedHint{"Args[0]: ", "par0"},
+                       ExpectedHint{"b: ", "par1"});
 }
 
 TEST(ParameterHints, VariadicOverloaded) {
   // Name for variadic parameter for an overloaded function with unique number
   // of parameters.
   // This prototype of std::forward is sufficient for clang to recognize it
   assertParameterHints(
       R"cpp(
struct container {
       template <typename... Args>
       void emplace(Args&&... args) {
         alloc a;
         auto ptr = a.template allocate<T>();
         a.construct(ptr, std::forward<Args>(args)...);
       }
     };
     void foo() {
-      container<S> c;
+      container<$param0[[S]]> c;
       c.emplace($param1[[1]]);
       c.emplace($param2[[2]], $param3[[3]]);
     }
   )cpp",
-      ExpectedHint{"A: ", "param1"}, ExpectedHint{"B: ", "param2"},
-      ExpectedHint{"C: ", "param3"});
+      ExpectedHint{"T: ", "param0"}, ExpectedHint{"A: ", "param1"},
+      ExpectedHint{"B: ", "param2"}, ExpectedHint{"C: ", "param3"});
 }
 
 TEST(ParameterHints, VariadicReferenceHint) {
   assertParameterHints(R"cpp(
     void foo(int&);
     template <typename... Args>
     void bar(Args... args) { return foo(args...); }
     void baz() {
TEST(ParameterHints, IncludeAtNonGlobalScope) {
 
   auto AST = Workspace.openFile("foo.cc");
   ASSERT_TRUE(bool(AST));
 
   // Ensure the hint for the call in foo.inc is NOT materialized in foo.cc.
   EXPECT_EQ(hintsOfKind(*AST, InlayHintKind::Parameter).size(), 0u);
 }
 
+TEST(ParameterHints, TemplateSpecialization) {
+  assertParameterHints(
+      R"cpp(
+    template <class T> struct A {};
+    template <class> struct B {};
+    template <class... Ts> struct C {};
+    template <class...> struct D {};
+
+    template <class T>
+    using E = A<T>;
+    template <class U>
+    using F = A<$par0[[U]]>;
+    template <class... Ts>
+    using G = C<$par1[[int]], $par2[[float]], Ts...>;
+
+    B<int> b;
+    D<$par3[[int]], $par4[[float]]> d;
+  )cpp",
+      ExpectedHint{"T: ", "par0"}, ExpectedHint{"Ts[0]: ", "par1"},
+      ExpectedHint{"Ts[1]: ", "par2"}, ExpectedHint{"[0]: ", "par3"},
+      ExpectedHint{"[1]: ", "par4"});
+}
+
+TEST(ParameterHints, ConceptSpecialization) {
+  assertParameterHints(R"cpp(
+    template <class T> concept A = true;
+    bool a = A<$par0[[int]]>;
+  )cpp",
+                       ExpectedHint{"T: ", "par0"});
+}
+
+TEST(ParameterHints, ClassSpecialization) {
+  assertParameterHints(R"cpp(
+    template <class T, class U> struct A {};
+    template <class T> struct A<$par0[[int]], $par1[[T]]> {};
+    template <> struct A<$par2[[int]], $par3[[float]]> {};
+  )cpp",
+                       ExpectedHint{"T: ", "par0"}, ExpectedHint{"U: ", "par1"},
+                       ExpectedHint{"T: ", "par2"},
+                       ExpectedHint{"U: ", "par3"});
+}
+
+TEST(ParameterHints, VarSpecialization) {
+  assertParameterHints(R"cpp(
+    template <class T, class U>
+    constexpr int value = 0;
+    template <class T>
+    constexpr int value<$par0[[int]], $par1[[T]]> = 0;
+    template <>
+    constexpr int value<$par2[[int]], $par3[[float]]> = 0;
+                       )cpp",
+                       ExpectedHint{"T: ", "par0"}, ExpectedHint{"U: ", "par1"},
+                       ExpectedHint{"T: ", "par2"},
+                       ExpectedHint{"U: ", "par3"});
+}
+
+TEST(ParameterHints, FunctionSpecialization) {
+  assertParameterHints(R"cpp(
+    template <class T> T add(T lhs, T rhs);
+    template <> int add<$par0[[int]]>(int lhs, int rhs);
+  )cpp",
+                       ExpectedHint{"T: ", "par0"});
+}
+
 TEST(TypeHints, Smoke) {
   assertTypeHints(R"cpp(
     auto $waldo[[waldo]] = 42;
   )cpp",
                   ExpectedHint{": int", "waldo"});
 }
 
 TEST(TypeHints, Decorations) {