[PATCH] D70445: [clangd] Show lambda signature for lambda autocompletions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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)



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3335
+    return nullptr;
+  const auto *LambdaType = VD->getType().getTypePtr();
+  if (const auto *SugaredType = dyn_cast<AutoType>(LambdaType))
----------------
kbobyrev wrote:
> sammccall wrote:
> > VD->getType()->getAsCXXRecordDecl() should be all you need.
> > getAs() and friends desugar as needed.
> 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?
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>.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3342
+  const auto *RecordDecl = LambdaType->getAsCXXRecordDecl();
+  // FIXME: Don't ignore generic lambdas.
+  return (RecordDecl && RecordDecl->isLambda() &&
----------------
kbobyrev wrote:
> sammccall wrote:
> > sammccall wrote:
> > > can we fix that instead of leaving a FIXME? should only be a couple of lines I think
> > also FIXME: support other functor types like std::function?
> > 
> > looking at the implementation of getLambdaCallOperatorHelper it looks pretty easy
> 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?
> 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.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3398
+  // parameters.
+  auto AddTemplatedFunctionTypeAndResult = [&](const FunctionTemplateDecl
+                                                   *FunTmpl) {
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70445/new/

https://reviews.llvm.org/D70445