[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 ↗

(On Diff #202739)

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

664 ↗

(On Diff #202739)

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

669 ↗

(On Diff #202739)

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 ↗

(On Diff #202739)

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

831 ↗

(On Diff #202870)

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 ↗

(On Diff #202870)

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

819 ↗

(On Diff #202870)

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

819 ↗

(On Diff #202870)

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.

831 ↗

(On Diff #202870)

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 ↗

(On Diff #202888)

We need a check for !QT.isNull here

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

831 ↗

(On Diff #202870)

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

831 ↗

(On Diff #202870)

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

831 ↗

(On Diff #202870)

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

831 ↗

(On Diff #202870)

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

831 ↗

(On Diff #202870)

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/trunk/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,
   SourceLocation End = Lexer::getLocForEndOfToken(
       Loc, 0, Ctx.getSourceManager(), Ctx.getLangOpts());
   if (!End.isValid())
     return llvm::None;
   return halfOpenToRange(Ctx.getSourceManager(),
                          CharSourceRange::getCharRange(Loc, End));
 }
 
+static const FunctionDecl *getUnderlyingFunction(const Decl *D) {
+  // Extract lambda from variables.
+  if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) {
+    auto QT = VD->getType();
+    if (!QT.isNull()) {
+      while (!QT->getPointeeType().isNull())
+        QT = QT->getPointeeType();
+
+      if (const auto *CD = QT->getAsCXXRecordDecl())
+        return CD->getLambdaCallOperator();
+    }
+  }
+
+  // Non-lambda functions.
+  return D->getAsFunction();
+}
+
 /// 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("::");
if (const TemplateDecl *TD = D->getDescribedTemplate()) {
     if (const auto FTD = FD->getDescribedTemplate()) {
       HI.TemplateParameters =
           fetchTemplateParameters(FTD->getTemplateParameters(), Policy);
       D = FTD;
     }
   }
 
   // Fill in types and params.
-  if (const FunctionDecl *FD = D->getAsFunction()) {
+  if (const FunctionDecl *FD = getUnderlyingFunction(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 << '(';
+    }
 
     HI.Parameters.emplace();
     for (const ParmVarDecl *PVD : FD->parameters()) {
-      if (HI.Parameters->size())
-        TypeOS << ", ";
       HI.Parameters->emplace_back();
       auto &P = HI.Parameters->back();
       if (!PVD->getType().isNull()) {
         P.Type.emplace();
         llvm::raw_string_ostream OS(*P.Type);
         PVD->getType().print(OS, Policy);
-        PVD->getType().print(TypeOS, Policy);
       } else {
         std::string Param;
         llvm::raw_string_ostream OS(Param);
         PVD->dump(OS);
         OS.flush();
         elog("Got param with null type: {0}", Param);
       }
       if (!PVD->getName().empty())
         P.Name = PVD->getNameAsString();
       if (PVD->hasDefaultArg()) {
         P.Default.emplace();
         llvm::raw_string_ostream Out(*P.Default);
         PVD->getDefaultArg()->printPretty(Out, nullptr, Policy);
       }
     }
-    TypeOS << ')';
+
+    HI.Type.emplace();
+    llvm::raw_string_ostream TypeOS(*HI.Type);
+    // Lambdas
+    if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D))
+      VD->getType().getDesugaredType(D->getASTContext()).print(TypeOS, Policy);
+    // Functions
+    else
+      FD->getType().print(TypeOS, Policy);
     // 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/trunk/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;
const std::function<void(HoverInfo &)> ExpectedBuilder;
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "foo";
          HI.Kind = SymbolKind::Function;
          HI.Documentation = "Best foo ever.";
          HI.Definition = "void foo()";
          HI.ReturnType = "void";
-         HI.Type = "void()";
+         HI.Type = "void ()";
          HI.Parameters.emplace();
        }},
       // Inside namespace
       {R"cpp(
           namespace ns1 { namespace ns2 {
             /// Best foo ever.
             void [[fo^o]]() {}
           }}
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "ns1::ns2::";
          HI.Name = "foo";
          HI.Kind = SymbolKind::Function;
          HI.Documentation = "Best foo ever.";
          HI.Definition = "void foo()";
          HI.ReturnType = "void";
-         HI.Type = "void()";
+         HI.Type = "void ()";
          HI.Parameters.emplace();
        }},
       // Field
       {R"cpp(
           namespace ns1 { namespace ns2 {
             struct Foo {
               int [[b^ar]];
             };
class Foo {})cpp";
          HI.NamespaceScope = "";
          HI.Name = "foo";
          HI.Kind = SymbolKind::Function;
          HI.Definition =
              R"cpp(template <template <typename, bool...> class C, typename = char, int = 0,
           bool Q = false, class... Ts>
 void foo())cpp";
          HI.ReturnType = "void";
-         HI.Type = "void()";
+         HI.Type = "void ()";
          HI.Parameters.emplace();
          HI.TemplateParameters = {
              {std::string("template <typename, bool...> class"),
               std::string("C"), llvm::None},
              {std::string("typename"), llvm::None, std::string("char")},
              {std::string("int"), llvm::None, std::string("0")},
              {std::string("bool"), std::string("Q"), std::string("false")},
              {std::string("class..."), std::string("Ts"), llvm::None},
void foo())cpp";
           }
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "foo";
          HI.Kind = SymbolKind::Function;
          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.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")},
          };
        }},
+      // Pointers to lambdas
+      {R"cpp(
+        void foo() {
+          auto lamb = [](int T, bool B) -> bool { return T && B; };
+          auto *b = &lamb;
+          auto *[[^c]] = &b;
+        }
+        )cpp",
+       [](HoverInfo &HI) {
+         HI.NamespaceScope = "";
+         HI.LocalScope = "foo::";
+         HI.Name = "c";
+         HI.Kind = SymbolKind::Variable;
+         HI.Definition = "auto *c = &b";
+         HI.Type = "class (lambda) **";
+         HI.ReturnType = "bool";
+         HI.Parameters = {
+             {std::string("int"), std::string("T"), llvm::None},
+             {std::string("bool"), std::string("B"), llvm::None},
+         };
+         return HI;
+       }},
+      // Lambda parameter with decltype reference
+      {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 = "decltype(lamb) &";
+         HI.ReturnType = "bool";
+         HI.Parameters = {
+             {std::string("int"), std::string("T"), llvm::None},
+             {std::string("bool"), std::string("B"), llvm::None},
+         };
+         return HI;
+       }},
+      // 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 = "class (lambda)";
+         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 = "class (lambda)";
+         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",