[clangd] Treat lambdas as functions when preparing hover response

[kadircet]

• Get rid of wrong check

[ilya-biryukov]

Do we care about pointers or references to lambda types?

clang-tools-extra/clangd/XRefs.cpp

662

Variable types can be null (for C++17 deconstruction syntax), use getTypePtrOrNull.

664

NIT: add extra braces to the inner if for more readable code

669

NIT: add extra braces to the inner if for more readable code

[kadircet]

• Address comments
• Handle pointer/reference types to lambdas

[kadircet]

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

[ilya-biryukov]

In D62814#1528736, @kadircet wrote:

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

Those cases are pretty rare, though, so I wasn't even sure they were worth it. They are pretty cheap to handle, though, so LG.

LGTM with a few last second NITs

clang-tools-extra/clangd/XRefs.cpp

664

Uh, I meant the outer if, sorry for the confusion. My idea is that only one level of nesting is allowed to omit braces.

if (condition1) {
  if (condition2) 
    return true;
}

Not a big deal, though, don't want to nit-pick on minor code style issues. Feel free to tailor to your liking

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

810

Could you add another test with even weirder types where we fail to show the signature? To make sure we don't break when reaching the limitations of the chosen approach and document what those limitations are.

Something like:

auto a = [](int a) { return 10; };
auto *b = &a;
auto *c = &b;

We would fail to show the signature here, but it's totally ok to ignore it.

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

657

can this be a separate function rather than a local lambda?

I think the name could be clearer, e.g. getUnderlyingFunction

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

798

I'm slightly nervous about the fact that "lambda" doesn't appear anywhere here.

e.g. maybe the Type should be "<lambda> bool(int, bool)" or so?
Many lambdas are not interchangeable with "plain" functions references.

[kadircet]

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

798

I've added the textual info to type. I don't think it is useful enough to put it as semantic info into the struct.

I believe it is rather a visual cue to the user, which seems pretty available in the "Type" field.

810

added cases, and changed code(a lot simpler now) to generate signatures for those cases as well.

[kadircet]

• Address comments

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.cpp

628

We need a check for !QT.isNull here

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

810

Here's an example when the new approach falls short too:

auto a = [](int) { return 10; }
std::function<void(decltype(a) x)> b;

In general, are we ok with loosing all the information about the type that we drop?
One level of references and pointers seemed ok, dropping more is a bit more cheesy..

At the same time, either case is so rare that we probably don't care.

[ilya-biryukov]

Please add a check for the type of variable being not null before landing. The other comment is not very important

[kadircet]

• Address comments

[kadircet]

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

810

are you talking about hovering over x ? I don't think AST contains information regarding that one.

for a code like this:

auto foo = []() { return 5; };

template <class T>
class Cls {};

Cls<void(decltype(foo) bar)> X;

This is the AST dump for variable X:

`-VarDecl 0x2b0e808 <line:6:1, col:30> col:30 X 'Cls<void (decltype(foo))>':'Cls<void ((lambda at a.cc:1:12))>' callinit
  `-CXXConstructExpr 0x2b12e80 <col:30> 'Cls<void (decltype(foo))>':'Cls<void ((lambda at a.cc:1:12))>' 'void () noexcept'

[ilya-biryukov]

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

810

I'm talking about hovering over b and, as Sam mentioned, there's a good chance you don't have this information in the type and we need to look at TypeLocs instead.

Also agree with Sam, we don't want any complexity for that case. Just wanted to make sure we added a test like this just to make sure we have some idea of what's produced there and it does not crash.

[kadircet]

• Address comments

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

810

I see, but then I don't think this case has anything to do with current patch, right?

It becomes a matter of decomposing a type with sugared components(which I believe should be visited but not in this patch) rather than expanding a lambda to a function like.

[ilya-biryukov]

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

810

Sorry, I missed that we still have the Type field set which will contain all pointers/references.
We don't seem to loose any information in that case, it's really up to the presentation layer to figure it out. LG

clang-tools-extra/clangd/XRefs.cpp

 #include "Protocol.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "index/Merge.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolLocation.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
static llvm::Optional<Range> getTokenRange(SourceLocation Loc,
   return halfOpenToRange(Ctx.getSourceManager(),
                          CharSourceRange::getCharRange(Loc, End));
 }
 
 /// Generate a \p Hover object given the declaration \p D.
 static HoverInfo getHoverContents(const Decl *D) {
   HoverInfo HI;
   const ASTContext &Ctx = D->getASTContext();
 
   HI.NamespaceScope = getNamespaceScope(D);
   if (!HI.NamespaceScope->empty())
     HI.NamespaceScope->append("::");
   HI.LocalScope = getLocalScope(D);
   if (!HI.LocalScope.empty())
     HI.LocalScope.append("::");
 
   PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
static HoverInfo getHoverContents(const Decl *D) {
   } else if (const FunctionDecl *FD = D->getAsFunction()) {
     if (const auto FTD = FD->getDescribedTemplate()) {
       HI.TemplateParameters =
           fetchTemplateParameters(FTD->getTemplateParameters(), Policy);
       D = FTD;
     }
   }
 
+  auto ToFunctionDecl = [](const Decl *D) -> const FunctionDecl * {
+    // Extract lambda from variables.
+    if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) {
+      // It might be declared by a decltype.
+      if (const DecltypeType *DT =
+              llvm::dyn_cast<DecltypeType>(VD->getType().getTypePtr()))
+        if (!DT->getUnderlyingType().isNull())
+          if (const auto *CD = DT->getUnderlyingType()->getAsCXXRecordDecl())
+            return CD->getLambdaCallOperator();
+
+      // Or with an initializer.
+      if (const Expr *E = VD->getInit())
+        if (const LambdaExpr *LE = llvm::dyn_cast<LambdaExpr>(E))
+          return LE->getCallOperator();
+    }
+
+    // Handle classes with call operators.
+    if (const CXXRecordDecl *CRD = llvm::dyn_cast<CXXRecordDecl>(D))
+      return CRD->getLambdaCallOperator();
+
+    // Non-lambda functions.
+    return D->getAsFunction();
+  };
+
   // Fill in types and params.
-  if (const FunctionDecl *FD = D->getAsFunction()) {
+  if (const FunctionDecl *FD = ToFunctionDecl(D)) {
     HI.ReturnType.emplace();
     llvm::raw_string_ostream OS(*HI.ReturnType);
     FD->getReturnType().print(OS, Policy);
 
     HI.Type.emplace();
     llvm::raw_string_ostream TypeOS(*HI.Type);
     FD->getReturnType().print(TypeOS, Policy);
     TypeOS << '(';
for (const ParmVarDecl *PVD : FD->parameters()) {
         P.Default.emplace();
         llvm::raw_string_ostream Out(*P.Default);
         PVD->getDefaultArg()->printPretty(Out, nullptr, Policy);
       }
     }
     TypeOS << ')';
     // FIXME: handle variadics.
   } else if (const auto *VD = dyn_cast<ValueDecl>(D)) {
-    // FIXME: Currently lambdas are also handled as ValueDecls, they should be
-    // more similar to functions.
     HI.Type.emplace();
     llvm::raw_string_ostream OS(*HI.Type);
     VD->getType().print(OS, Policy);
   }
 
   HI.Definition = printDefinition(D);
   return HI;
 }

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

 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/ADT/None.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::Matcher;
void foo())cpp";
          HI.Definition = "Foo<bool, true, false> foo(int, bool T = false)";
          HI.ReturnType = "Foo<bool, true, false>";
          HI.Type = "Foo<bool, true, false>(int, bool)";
          HI.Parameters = {
              {std::string("int"), llvm::None, llvm::None},
              {std::string("bool"), std::string("T"), std::string("false")},
          };
        }},
+      // Lambda parameter with decltype
+      {R"cpp(
+        auto lamb = [](int T, bool B) -> bool { return T && B; };
+        void foo(decltype(lamb) bar) {
+          [[ba^r]](0, false);
+        }
+        )cpp",
+       [](HoverInfo &HI) {
+         HI.NamespaceScope = "";
+         HI.LocalScope = "foo::";
+         HI.Name = "bar";
+         HI.Kind = SymbolKind::Variable;
+         HI.Definition = "decltype(lamb) bar";
+         HI.Type = "bool(int, bool)";
+         HI.ReturnType = "bool";
+         HI.Parameters = {
+             {std::string("int"), std::string("T"), llvm::None},
+             {std::string("bool"), std::string("B"), llvm::None},
+         };
+         return HI;
+       }},
       // Lambda variable
       {R"cpp(
         void foo() {
           int bar = 5;
           auto lamb = [&bar](int T, bool B) -> bool { return T && B && bar; };
           bool res = [[lam^b]](bar, false);
         }
         )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.LocalScope = "foo::";
          HI.Name = "lamb";
          HI.Kind = SymbolKind::Variable;
          HI.Definition = "auto lamb = [&bar](int T, bool B) -> bool {}";
-         HI.Type = std::string("class (lambda)");
+         HI.Type = "bool(int, bool)";
+         HI.ReturnType = "bool";
+         HI.Parameters = {
+             {std::string("int"), std::string("T"), llvm::None},
+             {std::string("bool"), std::string("B"), llvm::None},
+         };
          return HI;
        }},
       // Local variable in lambda
       {R"cpp(
         void foo() {
           auto lamb = []{int [[te^st]];};
         }
         )cpp",
        [](HoverInfo &HI) {