[clangd] Parameter hints for calls through function pointers

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/1734

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

491

The reason I didn't unify them in this patch is less that I'm lazy, and more that I'm not sure where to put code that will be shared by SemaCodeComplete.cpp and clangd.

Do we have some sort of dumping ground for miscellaneous AST utilities, like clang-tools-extra/clangd/AST.h but also usable in libSema?

[nridge]

Fix naming nit

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

491

I guess the function is performing a heuristic / best-effort operation, so HeuristicResolver could be the place for it once that is upstreamed.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

533

Calling this "callee" confuses me a bit - originally the callee was an expr and always present, so this must be a different idea: CalleeDecl?.

However I also think this struct has an unneccesarily broad scope: we're not really gaining anything by bundling Args into it rather than continuing to pass that as a separate param.
This suggests:

struct Callee {
  Decl *Declaration;
  FunctionProtoTypeLoc Loc;
};

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

491

(I'm happy with this being where it is, for lack of a really good option)

I don't have a good answer here - sometimes people seem to dumb these as methods on Expr etc itself, but that seems terrible. AST is probably the best existing library for it if we want to use it from Sema, but it feels a little... impure.

It doesn't feel like a perfect fit for HeuristicResolver but in practice if we're going to move that into AST then it seems best overall for this function to come along for the ride.

[nridge]

Address review comments

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

533

Good point, updated

clang-tools-extra/clangd/InlayHints.cpp

QualType maybeDesugar(ASTContext &AST, QualType QT) {
   if (const AutoType *AT = QT->getContainedAutoType())
     if (!AT->getDeducedType().isNull() &&
         AT->getDeducedType()->isDecltypeType())
       return QT.getCanonicalType();
 
   return QT;
 }
 
+// Given a callee expression `Fn`, if the call is through a function pointer,
+// try to find the declaration of the corresponding function pointer type,
+// so that we can recover argument names from it.
+// FIXME: This function is mostly duplicated in SemaCodeComplete.cpp; unify.
+static FunctionProtoTypeLoc getPrototypeLoc(Expr *Fn) {
+  TypeLoc Target;
+  Expr *NakedFn = Fn->IgnoreParenCasts();
+  if (const auto *T = NakedFn->getType().getTypePtr()->getAs<TypedefType>()) {
+    Target = T->getDecl()->getTypeSourceInfo()->getTypeLoc();
+  } else if (const auto *DR = dyn_cast<DeclRefExpr>(NakedFn)) {
+    const auto *D = DR->getDecl();
+    if (const auto *const VD = dyn_cast<VarDecl>(D)) {
+      Target = VD->getTypeSourceInfo()->getTypeLoc();
+    }
+  }
+
+  if (!Target)
+    return {};
+
+  // Unwrap types that may be wrapping the function type
+  while (true) {
+    if (auto P = Target.getAs<PointerTypeLoc>()) {
+      Target = P.getPointeeLoc();
+      continue;
+    }
+    if (auto A = Target.getAs<AttributedTypeLoc>()) {
+      Target = A.getModifiedLoc();
+      continue;
+    }
+    if (auto P = Target.getAs<ParenTypeLoc>()) {
+      Target = P.getInnerLoc();
+      continue;
+    }
+    break;
+  }
+
+  if (auto F = Target.getAs<FunctionProtoTypeLoc>()) {
+    return F;
+  }
+
+  return {};
+}
+
+struct Callee {
+  // Only one of Decl or Loc is set.
+  // Loc is for calls through function pointers.
+  const FunctionDecl *Decl = nullptr;
+  FunctionProtoTypeLoc Loc;
+};
+
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
                    const Config &Cfg, std::optional<Range> RestrictRange)
       : Results(Results), AST(AST.getASTContext()), Tokens(AST.getTokens()),
         Cfg(Cfg), RestrictRange(std::move(RestrictRange)),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // 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() ||
         E->isStdInitListInitialization()) {
       return true;
     }
 
-    processCall(E->getConstructor(), {E->getArgs(), E->getNumArgs()});
+    Callee Callee;
+    Callee.Decl = E->getConstructor();
+    if (!Callee.Decl)
+      return true;
+    processCall(Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitCallExpr(CallExpr *E) {
     if (!Cfg.InlayHints.Parameters)
       return true;
 
     // Do not show parameter hints for operator calls written using operator
     // syntax or user-defined literals. (Among other reasons, the resulting
     // hints can look awkward, e.g. the expression can itself be a function
     // argument and then we'd get two hints side by side).
     if (isa<CXXOperatorCallExpr>(E) || isa<UserDefinedLiteral>(E))
       return true;
 
     auto CalleeDecls = Resolver->resolveCalleeOfCallExpr(E);
     if (CalleeDecls.size() != 1)
       return true;
-    const FunctionDecl *Callee = nullptr;
+
+    Callee Callee;
     if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecls[0]))
-      Callee = FD;
+      Callee.Decl = FD;
     else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CalleeDecls[0]))
-      Callee = FTD->getTemplatedDecl();
-    if (!Callee)
+      Callee.Decl = FTD->getTemplatedDecl();
+    else if (FunctionProtoTypeLoc Loc = getPrototypeLoc(E->getCallee()))
+      Callee.Loc = Loc;
+    else
       return true;
 
     processCall(Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *FPT =
bool VisitInitListExpr(InitListExpr *Syn) {
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
-  void processCall(const FunctionDecl *Callee,
-                   llvm::ArrayRef<const Expr *> Args) {
-    if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
+  void processCall(Callee Callee, llvm::ArrayRef<const Expr *> Args) {
+    assert(Callee.Decl || Callee.Loc);
+
+    if (!Cfg.InlayHints.Parameters || Args.size() == 0)
       return;
 
     // The parameter name of a move or copy constructor is not very interesting.
-    if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
+    if (Callee.Decl)
+      if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee.Decl))
         if (Ctor->isCopyOrMoveConstructor())
           return;
 
+    auto Params =
+        Callee.Decl ? Callee.Decl->parameters() : Callee.Loc.getParams();
+
     // Resolve parameter packs to their forwarded parameter
-    auto ForwardedParams = resolveForwardingParameters(Callee);
+    SmallVector<const ParmVarDecl *> ForwardedParams;
+    if (Callee.Decl)
+      ForwardedParams = resolveForwardingParameters(Callee.Decl);
+    else
+      ForwardedParams = {Params.begin(), Params.end()};
 
     NameVec ParameterNames = chooseParameterNames(ForwardedParams);
 
     // Exclude setters (i.e. functions with one argument whose name begins with
     // "set"), and builtins like std::move/forward/... as their parameter name
     // is also not likely to be interesting.
-    if (isSetter(Callee, ParameterNames) || isSimpleBuiltin(Callee))
+    if (Callee.Decl &&
+        (isSetter(Callee.Decl, ParameterNames) || isSimpleBuiltin(Callee.Decl)))
       return;
 
     for (size_t I = 0; I < ParameterNames.size() && I < Args.size(); ++I) {
       // 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 (isa<PackExpansionExpr>(Args[I])) {
         break;
       }
 
       StringRef Name = ParameterNames[I];
       bool NameHint = shouldHintName(Args[I], Name);
-      bool ReferenceHint =
-          shouldHintReference(Callee->getParamDecl(I), ForwardedParams[I]);
+      bool ReferenceHint = shouldHintReference(Params[I], ForwardedParams[I]);
 
       if (NameHint || ReferenceHint) {
         addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
                      InlayHintKind::Parameter, ReferenceHint ? "&" : "",
                      NameHint ? Name : "", ": ");
       }
     }
   }

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

S foo() {
       // Do not show hint for implicit constructor call in argument.
       bar(42);
       // Do not show hint for implicit constructor call in return.
       return 42;
     }
   )cpp");
 }
 
+TEST(ParameterHints, FunctionPointer) {
+  assertParameterHints(
+      R"cpp(
+    void (*f1)(int param);
+    void (__stdcall *f2)(int param);
+    using f3_t = void(*)(int param);
+    f3_t f3;
+    using f4_t = void(__stdcall *)(int param);
+    f4_t f4;
+    void bar() {
+      f1($f1[[42]]);
+      f2($f2[[42]]);
+      f3($f3[[42]]);
+      f4($f4[[42]]);
+    }
+  )cpp",
+      ExpectedHint{"param: ", "f1"}, ExpectedHint{"param: ", "f2"},
+      ExpectedHint{"param: ", "f3"}, ExpectedHint{"param: ", "f4"});
+}
+
 TEST(ParameterHints, ArgMatchesParam) {
   assertParameterHints(R"cpp(
     void foo(int param);
     struct S {
       static const int param = 42;
     };
     void bar() {
       int param = 42;