[clangd] Show lambda signature for lambda autocompletions

[kbobyrev]

Summary

The original bug report can be found here.

Given the following code:

c++
void function() {
  auto Lambda = [](int a, double &b) {return 1.f;};
  La^
}

Triggering the completion at ^ would show (lambda) before this patch and would show signature (int a, double &b) const, build a snippet etc with this patch.

[sammccall]

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

2506

testing this in clangd is nice to have, but the canonical test for stuff in sema code completion should be a lit test under clang/test/CodeCompletion IIRC

2508

specializing opts shouldn't be needed for this test

2518

auto&

clang/lib/Sema/SemaCodeComplete.cpp

3318

llvm style is to make the function static instead (we don't follow this particlularly closely in clangd, but we should here)

3319

in view of the functor (std::function etc) case, should this be extractFunctorCallOperator()?

3323

VD->getType()->getAsCXXRecordDecl() should be all you need.
getAs() and friends desugar as needed.

3330

can we fix that instead of leaving a FIXME? should only be a couple of lines I think

3330

also FIXME: support other functor types like std::function?

looking at the implementation of getLambdaCallOperatorHelper it looks pretty easy

3357

can you give this a clearer name?
Like AddFunctionTypeAndResult?

I also think making it void would be clearer, rather than mixing adding/building.

3361

why do you want to avoid adding the qualifier in lambda case?

3380

above the return statement to below - why?

[kbobyrev]

Add initial support for generic lambdas, put more tests into clang/test/CodeCompletion.

[kbobyrev]

clang/lib/Sema/SemaCodeComplete.cpp

3323

Could you please elaborate here? VD->getType() would be a QualType, so I first cast it to the pointer and then desugar it (otherwise I might not be able to call ->getAsCXXRecordDecl()), then I do the getAsCXXRecordDecl() as suggested. Is there any way I could simplify this?

3330

I was thinking about it, but decided against it in the end because of possible incomplete support of the generic lambdas. I have added the basic handling which, given

auto foo(auto x, const auto &y) { return &y; }

would show

foo(auto x, const auto &y) -> auto

in code completion.

This should be about exposing some human-readable dummy template types - showing foo<class A, class B>(A a, const B &b) -> B * instead. This is a bit more work: not only should we come up with the classnames (which is somewhat OK), but also connect the dots, so that the same typename is "propagated" from the piece which is responsible for adding <class A, class B> to the following pieces - the arguments and return type. I don't see any simple heuristic here and probably the correct way of doing that would be assigning a dummy typename to every TemplateTypeParmDecl and then propagating this information to AddTemplateParameterChunks, AddFunctionParameterChunks, AddFunctionTypeQualsToCompletionString, but it's certainly not several lines of code and it might be better to leave this for the next patch. This probably requires tweaking Sema::MarkDeducedTemplateParameters or doing something similar.

This kind of increases the scope of the patch (which is probably not a great thing) and also makes it incomplete in some sense, but that might be OK.

What do you think? Should I do that in a separate patch?

3380

For lambdas, ND should be replaced with CallOperator, hence I think it's easier to repeat this line of code in the above function/lambda handler (inserting the correct delcaration) and then simply proceed with the general case (i.e. use ND for the result type).

Does this make sense? Can I simplify this somehow?

[sammccall]

Behavior looks good, and I wouldn't extend it any further (other than to non-lambda functors someday).

Implementation can be simplified a bit I think - see comment about the templated function codepath.

(Would like to see one more round if that's OK, if I'm wrong about the simplifications I'd like to understand why)

clang/lib/Sema/SemaCodeComplete.cpp

3323

QualType acts as a smart-pointer to type: QT->foo() is the same as T.foo().
(This is unusual and confusing, but you get used to it...)

You don't need to desugar, because getAsCXXRecordDecl() does the desugaring itself. This isn't clearly documented, but getAsCXXRecordDecl is a variant of (and calls) getAs<T>, which says

Look through sugar for an instance of \<specific type>.

3330

This should be about exposing some human-readable dummy template types - showing foo<class A, class B>(A a, const B &b) -> B * instead. This is a bit more work...

Frankly I'm not sure it's a good idea, so I'd suggest leaving related fixmes out.
While yes, such a signature would be accurate and provide more information, there are downsides:

• the shown signature is very different from the declaration and may not be clear to the reader
• it's longer/noisier, and the new information comes at the end
• there'll be a great deal of complexity in the implementation
• it's inconsistent with how clang treats generic lambda type params elsewhere, which is to print them as auto (see TypePrinter::printTemplateTypeParmBefore)

The basic support you've added (foo(auto x, const auto &y) -> auto) is what I'd personally like to see here as a user.

3387

I actually think there's no need to modify the "templated function" path at all - you can just take return the templated FunctionDecl from extractFunctorCallOperator and treat it as a regular function.

The template function stuff is about identifying template parameters that need to be spelled, and adding them to the CCS. With generic lambdas the type parameters are always deduced and may not be specified. And this can be assumed the case for general functors as well, as the syntax Functor f; f<int>(); isn't legal.

[kbobyrev]

Simplified the patch to address the comments.

clang/lib/Sema/SemaCodeComplete.cpp

3387

Good point! I did that because, as I mentioned earlier, my impression was that we would probably want something like foo<class A, class B>(A a, const B &b) -> B * (and probably even more complicated things to print dependent types). But if we don't want to go that direction, this should not be needed, thanks!

[sammccall]

Great, thanks for fixing this!

clang/test/CodeCompletion/lambdas.cpp

66

nit: often preferable to make the examples valid syntax (and set code completion point to interrupt it, to avoid different tests interfering with each other.

(Here the my_ prefix isn't needed - plain completion on an empty line will yield the result you want)

[kbobyrev]

Get rid of incomplete identifiers in test code.

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

TEST(CompletionTest, NoCompletionsForNewNames) {
   Opts.AllScopes = true;
   auto Results = completions(R"cpp(
       void f() { int n^ }
     )cpp",
                              {cls("naber"), cls("nx::naber")}, Opts);
   EXPECT_THAT(Results.Completions, UnorderedElementsAre());
 }
 
+TEST(CompletionTest, Lambda) {
+  clangd::CodeCompleteOptions Opts = {};
+
+  auto Results = completions(R"cpp(
+    void function() {
+      auto Lambda = [](int a, const double &b) {return 1.f;};
+      Lam^
+    }
+  )cpp",
+                             {}, Opts);
+
+  ASSERT_EQ(Results.Completions.size(), 1u);
+  const auto &A = Results.Completions.front();
+  EXPECT_EQ(A.Name, "Lambda");
+  EXPECT_EQ(A.Signature, "(int a, const double &b) const");
+  EXPECT_EQ(A.Kind, CompletionItemKind::Variable);
+  EXPECT_EQ(A.ReturnType, "float");
+  EXPECT_EQ(A.SnippetSuffix, "(${1:int a}, ${2:const double &b})");
+}
+
 TEST(CompletionTest, ObjectiveCMethodNoArguments) {
   auto Results = completions(R"objc(
       @interface Foo
       @property(nonatomic, setter=setXToIgnoreComplete:) int value;
       @end
       Foo *foo = [Foo new]; int y = [foo v^]
     )objc",
                              /*IndexSymbols=*/{},
                              /*Opts=*/{}, "Foo.m");
 
   auto C = Results.Completions;
   EXPECT_THAT(C, ElementsAre(Named("value")));
   EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
   EXPECT_THAT(C, ElementsAre(ReturnType("int")));
   EXPECT_THAT(C, ElementsAre(Signature("")));
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("")));
 }
 
 TEST(CompletionTest, ObjectiveCMethodOneArgument) {
   auto Results = completions(R"objc(
       @interface Foo
       - (int)valueForCharacter:(char)c;
       @end
       Foo *foo = [Foo new]; int y = [foo v^]
     )objc",
                              /*IndexSymbols=*/{},
                              /*Opts=*/{}, "Foo.m");
 
   auto C = Results.Completions;
   EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
   EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
   EXPECT_THAT(C, ElementsAre(ReturnType("int")));
   EXPECT_THAT(C, ElementsAre(Signature("(char)")));
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(char)}")));
 }
 
 TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromBeginning) {
   auto Results = completions(R"objc(
       @interface Foo

clang/lib/Sema/SemaCodeComplete.cpp

CodeCompletionResult::createCodeCompletionStringForOverride(
   NameAndSignature += " override";
 
   Result.AddTextChunk(Result.getAllocator().CopyString(BeforeName));
   Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
   Result.AddTypedTextChunk(Result.getAllocator().CopyString(NameAndSignature));
   return Result.TakeString();
 }
 
+// FIXME: Right now this works well with lambdas. Add support for other functor
+// types like std::function.
+static const NamedDecl *extractFunctorCallOperator(const NamedDecl *ND) {
+  const auto *VD = dyn_cast<VarDecl>(ND);
+  if (!VD)
+    return nullptr;
+  const auto *RecordDecl = VD->getType()->getAsCXXRecordDecl();
+  if (!RecordDecl || !RecordDecl->isLambda())
+    return nullptr;
+  return RecordDecl->getLambdaCallOperator();
+}
+
 CodeCompletionString *CodeCompletionResult::createCodeCompletionStringForDecl(
     Preprocessor &PP, ASTContext &Ctx, CodeCompletionBuilder &Result,
     bool IncludeBriefComments, const CodeCompletionContext &CCContext,
     PrintingPolicy &Policy) {
   const NamedDecl *ND = Declaration;
   Result.addParentContext(ND->getDeclContext());
 
   if (IncludeBriefComments) {
     // Add documentation comment, if it exists.
     if (const RawComment *RC = getCompletionComment(Ctx, Declaration)) {
       Result.addBriefComment(RC->getBriefText(Ctx));
     }
   }
 
   if (StartsNestedNameSpecifier) {
     Result.AddTypedTextChunk(
         Result.getAllocator().CopyString(ND->getNameAsString()));
     Result.AddTextChunk("::");
     return Result.TakeString();
   }
 
   for (const auto *I : ND->specific_attrs<AnnotateAttr>())
     Result.AddAnnotation(Result.getAllocator().CopyString(I->getAnnotation()));
 
-  AddResultTypeChunk(Ctx, Policy, ND, CCContext.getBaseType(), Result);
-
-  if (const auto *Function = dyn_cast<FunctionDecl>(ND)) {
+  auto AddFunctionTypeAndResult = [&](const FunctionDecl *Function) {
+    AddResultTypeChunk(Ctx, Policy, Function, CCContext.getBaseType(), Result);
     AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
                                    Ctx, Policy);
     AddTypedNameChunk(Ctx, Policy, ND, Result);
     Result.AddChunk(CodeCompletionString::CK_LeftParen);
     AddFunctionParameterChunks(PP, Policy, Function, Result);
     Result.AddChunk(CodeCompletionString::CK_RightParen);
     AddFunctionTypeQualsToCompletionString(Result, Function);
+  };
+
+  if (const auto *Function = dyn_cast<FunctionDecl>(ND)) {
+    AddFunctionTypeAndResult(Function);
+    return Result.TakeString();
+  }
+
+  if (const auto *CallOperator =
+          dyn_cast_or_null<FunctionDecl>(extractFunctorCallOperator(ND))) {
+    AddFunctionTypeAndResult(CallOperator);
     return Result.TakeString();
   }
 
+  AddResultTypeChunk(Ctx, Policy, ND, CCContext.getBaseType(), Result);
+
   if (const FunctionTemplateDecl *FunTmpl =
           dyn_cast<FunctionTemplateDecl>(ND)) {
     AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
                                    Ctx, Policy);
     FunctionDecl *Function = FunTmpl->getTemplatedDecl();
     AddTypedNameChunk(Ctx, Policy, Function, Result);
 
     // Figure out which template parameters are deduced (or have default
     // arguments).
     llvm::SmallBitVector Deduced;
     Sema::MarkDeducedTemplateParameters(Ctx, FunTmpl, Deduced);
     unsigned LastDeducibleArgument;
     for (LastDeducibleArgument = Deduced.size(); LastDeducibleArgument > 0;
          --LastDeducibleArgument) {
       if (!Deduced[LastDeducibleArgument - 1]) {
         // C++0x: Figure out if the template argument has a default. If so,
         // the user doesn't need to type this argument.
         // FIXME: We need to abstract template parameters better!
AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
                                    Ctx, Policy);
     Result.AddTypedTextChunk(
         Result.getAllocator().CopyString(Template->getNameAsString()));
     Result.AddChunk(CodeCompletionString::CK_LeftAngle);
     AddTemplateParameterChunks(Ctx, Policy, Template, Result);
     Result.AddChunk(CodeCompletionString::CK_RightAngle);
     return Result.TakeString();
   }
+
   if (const auto *Method = dyn_cast<ObjCMethodDecl>(ND)) {
     Selector Sel = Method->getSelector();
     if (Sel.isUnarySelector()) {
       Result.AddTypedTextChunk(
           Result.getAllocator().CopyString(Sel.getNameForSlot(0)));
       return Result.TakeString();
     }
 

clang/test/CodeCompletion/function-templates.cpp

 namespace std {
   template<typename RandomAccessIterator>
   void sort(RandomAccessIterator first, RandomAccessIterator last);
-  
   template<class X, class Y>
   X* dyn_cast(Y *Val);
 }
 
 class Foo {
 public:
   template<typename T> T &getAs();
 };
 
+template <typename T, typename U, typename V>
+V doSomething(T t, const U &u, V *v) { return V(); }
+
 void f() {
   std::sort(1, 2);
   Foo().getAs<int>();
-  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:15:8 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:18:8 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
   // CHECK-CC1: dyn_cast<<#class X#>>(<#Y *Val#>)
   // CHECK-CC1: sort(<#RandomAccessIterator first#>, <#RandomAccessIterator last#>
-  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:16:9 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:19:9 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
   // CHECK-CC2: getAs<<#typename T#>>()
-)
-  
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:19:22 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+  // CHECK-CC3: [#V#]doSomething(<#T t#>, <#const U &u#>, <#V *v#>)
+}

clang/test/CodeCompletion/lambdas.cpp

 void test5() {
   // Completions are only added when -code-completion-patterns are enabled.
   function<void()> b = {};
   // RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:57:24 %s -o - | FileCheck -check-prefix=CHECK-7 %s
   // CHECK-7: COMPLETION: Pattern : [<#=
   // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:57:24 %s -o - | FileCheck -check-prefix=CHECK-8 %s
   // CHECK-8-NOT: COMPLETION: Pattern : [<#=
 }
+
+void test6() {
+  auto my_lambda = [&](int a, double &b) { return 1.f; };
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:65:58 %s -o - | FileCheck -check-prefix=CHECK-9 %s
+  // CHECK-9: [#float#]my_lambda(<#int a#>, <#double &b#>)[# const#]
+}
+
+void test7() {
+  auto generic_lambda = [&](auto a, const auto &b) { return a + b; };
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:71:70 %s -o - | FileCheck -check-prefix=CHECK-10 %s
+  // CHECK-10: [#auto#]generic_lambda(<#auto a#>, <#const auto &b#>)[# const#]
+}