[clangd] Parameter hints for dependent calls

[nridge]

Add some more test cases

[nridge]

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

267

This is an interesting case. Clang builds a CXXDependentScopeMemberExpr for this callee, but HeuristicResolver currently assumes that such expressions are only built for non-static member accesses (since, for static member accesses, clang usually builds a DependentScopeDeclRefExpr instead).

The CXXDependentScopeMemberExpr is created here, and I note the dependence on whether the enclosing context is an instance method. I guess it must think that, after instantiation, A<T> could turn out to be a base class, and thus this could be a "non-static member access with qualifier".

I don't see anything obvious on CXXDependentScopeMemberExpr that would let us tell apart "definitely a non-static member" from "maybe static, maybe non-static", so I guess the appropriate solution is to drop the NonStaticFilter here altogether?

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

56

should we conservatively make this size != 1?
e.g. if the callee ends up being an OverloadExpr of some kind, picking the first overload seems pretty arbitrary and maybe misleading.

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

247

can we add a test involving overloads?

i think:

void foo(int anInt);
void foo(double aDouble);
template <class T> go {
  foo(T{}); // expect no hints
}

267

I guess it must think that, after instantiation, A<T> could turn out to be a base class, and thus this could be a "non-static member access with qualifier".

Argh, C++ is so tricky :-( That sounds plausible to me.

drop the NonStaticFilter here altogether

Yeah. The other thing is that some_instance.some_static_member is perfectly legal I think? So the NonStaticFilter is probably not correct anyway.

[nridge]

Address review comments

[nridge]

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

267

This is sufficiently non-trivial to fix (requires more than just removing the filter) that I'll leave it for a separate patch / review.

clang-tools-extra/clangd/HeuristicResolver.h

public:
   // types to one or more likely-referenced declarations.
   std::vector<const NamedDecl *>
   resolveMemberExpr(const CXXDependentScopeMemberExpr *ME) const;
   std::vector<const NamedDecl *>
   resolveDeclRefExpr(const DependentScopeDeclRefExpr *RE) const;
   std::vector<const NamedDecl *>
   resolveTypeOfCallExpr(const CallExpr *CE) const;
   std::vector<const NamedDecl *>
+  resolveCalleeOfCallExpr(const CallExpr *CE) const;
+  std::vector<const NamedDecl *>
   resolveUsingValueDecl(const UnresolvedUsingValueDecl *UUVD) const;
   std::vector<const NamedDecl *>
   resolveDependentNameType(const DependentNameType *DNT) const;
   std::vector<const NamedDecl *> resolveTemplateSpecializationType(
       const DependentTemplateSpecializationType *DTST) const;
 
   // Try to heuristically resolve a dependent nested name specifier
   // to the type it likely denotes. Note that *dependent* name specifiers always
private:
   // for a call to vector<T>::size().
   std::vector<const NamedDecl *> resolveDependentMember(
       const Type *T, DeclarationName Name,
       llvm::function_ref<bool(const NamedDecl *ND)> Filter) const;
 
   // Try to heuristically resolve the type of a possibly-dependent expression
   // `E`.
   const Type *resolveExprToType(const Expr *E) const;
+  std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E) const;
 
   // Given the type T of a dependent expression that appears of the LHS of a
   // "->", heuristically find a corresponding pointee type in whose scope we
   // could look up the name appearing on the RHS.
   const Type *getPointeeType(const Type *T) const;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/HeuristicResolver.cpp

if (const FunctionType *FnType = CalleeType->getAs<FunctionType>()) {
     if (const auto *D =
             resolveTypeToRecordDecl(FnType->getReturnType().getTypePtr())) {
       return {D};
     }
   }
   return {};
 }
 
+std::vector<const NamedDecl *>
+HeuristicResolver::resolveCalleeOfCallExpr(const CallExpr *CE) const {
+  if (const auto *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
+    return {ND};
+  }
+
+  return resolveExprToDecls(CE->getCallee());
+}
+
 std::vector<const NamedDecl *> HeuristicResolver::resolveUsingValueDecl(
     const UnresolvedUsingValueDecl *UUVD) const {
   return resolveDependentMember(UUVD->getQualifier()->getAsType(),
                                 UUVD->getNameInfo().getName(), ValueFilter);
 }
 
 std::vector<const NamedDecl *> HeuristicResolver::resolveDependentNameType(
     const DependentNameType *DNT) const {
if (const auto *TD = dyn_cast<TypeDecl>(Decls[0])) {
     return TD->getTypeForDecl();
   }
   if (const auto *VD = dyn_cast<ValueDecl>(Decls[0])) {
     return VD->getType().getTypePtrOrNull();
   }
   return nullptr;
 }
 
-const Type *HeuristicResolver::resolveExprToType(const Expr *E) const {
+std::vector<const NamedDecl *>
+HeuristicResolver::resolveExprToDecls(const Expr *E) const {
   if (const auto *ME = dyn_cast<CXXDependentScopeMemberExpr>(E)) {
-    return resolveDeclsToType(resolveMemberExpr(ME));
+    return resolveMemberExpr(ME);
   }
   if (const auto *RE = dyn_cast<DependentScopeDeclRefExpr>(E)) {
-    return resolveDeclsToType(resolveDeclRefExpr(RE));
+    return resolveDeclRefExpr(RE);
+  }
+  if (const auto *OE = dyn_cast<OverloadExpr>(E)) {
+    return {OE->decls_begin(), OE->decls_end()};
   }
   if (const auto *CE = dyn_cast<CallExpr>(E)) {
-    return resolveDeclsToType(resolveTypeOfCallExpr(CE));
+    return resolveTypeOfCallExpr(CE);
   }
   if (const auto *ME = dyn_cast<MemberExpr>(E))
-    return resolveDeclsToType({ME->getMemberDecl()});
+    return {ME->getMemberDecl()};
+
+  return {};
+}
+
+const Type *HeuristicResolver::resolveExprToType(const Expr *E) const {
+  std::vector<const NamedDecl *> Decls = resolveExprToDecls(E);
+  if (!Decls.empty())
+    return resolveDeclsToType(Decls);
 
   return E->getType().getTypePtr();
 }
 
 const Type *HeuristicResolver::resolveNestedNameSpecifierToType(
     const NestedNameSpecifier *NNS) const {
   if (!NNS)
     return nullptr;
if (auto *RD = resolveTypeToRecordDecl(T)) {
     RD = RD->getDefinition();
     return RD->lookupDependentName(Name, Filter);
   }
   return {};
 }
 
 } // namespace clangd
 } // namespace clang
 No newline at end of file

clang-tools-extra/clangd/InlayHints.cpp

 //===--- InlayHints.cpp ------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "InlayHints.h"
+#include "HeuristicResolver.h"
 #include "ParsedAST.h"
 #include "support/Logger.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
 namespace clangd {
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
       : Results(Results), AST(AST.getASTContext()),
-        MainFileID(AST.getSourceManager().getMainFileID()) {
+        MainFileID(AST.getSourceManager().getMainFileID()),
+        Resolver(AST.getHeuristicResolver()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
     MainFileBuf = Invalid ? StringRef{} : Buf;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // Weed out constructor calls that don't look like a function call with
public:
   bool VisitCallExpr(CallExpr *E) {
     // Do not show parameter hints for operator calls written using operator
     // syntax or user-defined literals. (Among other reasons, the resulting
     // hints can look awkard, 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;
 
-    processCall(E->getRParenLoc(),
-                dyn_cast_or_null<FunctionDecl>(E->getCalleeDecl()),
-                {E->getArgs(), E->getNumArgs()});
+    auto CalleeDecls = Resolver->resolveCalleeOfCallExpr(E);
+    if (CalleeDecls.size() != 1)
+      return true;
+    const FunctionDecl *Callee = nullptr;
+    if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecls[0]))
+      Callee = FD;
+    else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CalleeDecls[0]))
+      Callee = FTD->getTemplatedDecl();
+    if (!Callee)
+      return true;
+
+    processCall(E->getRParenLoc(), Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
Results.push_back(InlayHint{
             sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())},
         Kind, Label.str()});
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
   FileID MainFileID;
   StringRef MainFileBuf;
+  const HeuristicResolver *Resolver;
 };
 
 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

assertParameterHints(R"cpp(
     void bar() {
       foo($p1[[41]], $p2[[42]], $p3[[43]]);
     }
   )cpp",
                        ExpectedHint{"p1: ", "p1"}, ExpectedHint{"p2: ", "p2"},
                        ExpectedHint{"p3: ", "p3"});
 }
 
-TEST(ParameterHints, DependentCall) {
-  // FIXME: This doesn't currently produce a hint but should.
+TEST(ParameterHints, DependentCalls) {
   assertParameterHints(R"cpp(
     template <typename T>
-    void foo(T param);
+    void nonmember(T par1);
+
+    template <typename T>
+    struct A {
+      void member(T par2);
+      static void static_member(T par3);
+    };
+
+    void overload(int anInt);
+    void overload(double aDouble);
 
     template <typename T>
     struct S {
-      void bar(T par) {
-        foo($param[[par]]);
+      void bar(A<T> a, T t) {
+        nonmember($par1[[t]]);
+        a.member($par2[[t]]);
+        // FIXME: This one does not work yet.
+        A<T>::static_member($par3[[t]]);
+        // We don't want to arbitrarily pick between
+        // "anInt" or "aDouble", so just show no hint.
+        overload(T{});
       }
     };
-  )cpp");
+  )cpp",
+                       ExpectedHint{"par1: ", "par1"},
+                       ExpectedHint{"par2: ", "par2"});
 }
 
 TEST(ParameterHints, VariadicFunction) {
   assertParameterHints(R"cpp(
     template <typename... T>
     void foo(int fixed, T... variadic);
 
     void bar() {
assertParameterHints(R"cpp(
     }
   )cpp",
                        ExpectedHint{"timeoutMillis: ", "timeoutMillis"},
                        ExpectedHint{"timeout_millis: ", "timeout_millis"});
 }
 
 } // namespace
 } // namespace clangd
-} // namespace clang
+} // namespace clang
 No newline at end of file