[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thank you Nathan for your constructive opinions! I've updated this patch again, hopefully this becomes better.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // When completing a non-static member function (and not via
-  // dot/arrow member access) and we're not inside that class' scope,
-  // it can't be a call.
+  // Decide whether or not a non-static member function can be a call.
   if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
----------------
nridge wrote:
> zyounan wrote:
> > sammccall wrote:
> > > This is confusing: the `CCC_Symbol` test is part of the specific heuristics being used (it's the "not via dot/arrow member access" part, right?) but you've moved the comment explaining what it does.
> > > 
> > > Also this function is just getting too long, and we're inlining more complicated control flow here.
> > > Can we extract a function?
> > > 
> > > ```
> > > const auto *Method = ...;
> > > if (Method & !Method->isStatic()) {
> > >   R.FunctionCanBeCall = canMethodBeCalled(...);
> > > }    
> > > ```
> > > it's the "not via dot/arrow member access" part
> > 
> > (Sorry for being unaware of the historical context). But I think `CCC_Symbol` should mean "we're completing a name such as a function or type name" per its comment. The heuristic for dot/arrow member access actually lies on the next line, i.e., if the completing decl is a CXXMethodDecl.
> > 
> > > Can we extract a function?
> > 
> > Sure. 
> The check for `CompletionContext.getKind()` is in fact a part of the heuristic:
> 
>  * for `f.method`, the kind is `CCC_DotMemberAccess`
>  * for `f->method`, the kind is `CCC_ArrowMemberAccess`
>  * for `f.Foo::method` and `f->Foo::method`, the kind is `CCC_Symbol`
> 
> (I realize that's a bit inconsistent. Maybe the `f.Foo::` and `f->Foo::` cases should be using `DotMemberAccess` and `ArrowMemberAccess` as well? Anyways, that's a pre-existing issue.)
> 
> So, the `if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol)` check is what currently makes sure that in the `f.method` and `f->method` cases, we just keep `FunctionCanBeCall = true` without having to check any context or expression type.
> 
> I think it may be clearest to move this entire `if` block into the new function (whose name can be generalized to `canBeCall` or similar), and here just unconditionally set `R.FunctionCanBeCall = canBeCall(CompletionContext, /* other things */)`.
> I think it may be clearest to move this entire if block into the new function

Nice suggestion. I'll take it.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
       // containing all of the arguments up to the first deducible argument.
+      // Or, if this isn't a call, emit all the template arguments
+      // to disambiguate the (potential) overloads.
----------------
zyounan wrote:
> nridge wrote:
> > zyounan wrote:
> > > nridge wrote:
> > > > 1. If this is not a call, we can avoid running the `Sema::MarkDeducedTemplateParameters` operation altogether.
> > > > 
> > > > 2. A future improvement we could consider: if this is not a call, try to detect cases where the template parameters can be deduced from the surrounding context (["Deducing template arguments taking the address of a function template "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe add a FIXME for this?
> > > > avoid running the Sema::MarkDeducedTemplateParameters operation altogether
> > > 
> > > I think doing so could cause too many adjustments to the flow, and I'm afraid that the `Sema::MarkDeducedTemplateParameters` would be required again when we decide to implement point 2.
> > > 
> > > I'm adding a FIXME anyway but leave the first intact. However, I'd be happy to rearrange the logic flow if you insist.
> > I realized there's actually an open question here: if `FunctionCanBeCall == false`, do we want to include **all** the template parameters, or just the non-deducible ones?
> > 
> > Let's consider an example:
> > 
> > ```
> > class Foo {
> >   template <typename T, typename U>
> >   T convertTo(U from);
> > };
> > 
> > void bar() {
> >   Foo::^
> > }
> > ```
> > 
> > Here, `U` can be deduced but `T` cannot.
> > 
> > The current behaviour is to complete `convertTo<T>`. That seems appropriate for a **call**, since `U` will be deduced from the call. But since this is not a call, wouldn't it be  better to complete `convertTo<T, U>`?
> > But since this is not a call, wouldn't it be better to complete convertTo<T, U>?
> 
> (Ugh, this is exactly this patch's initial intention, but how...?)
> 
> Oh, I made a mistake here: I presumed `LastDeducibleArgument` would always be 0 if we're in a non-callable context, but this is wrong! This variable is calculated based on the function parameters, which ought to be equal to the number of template parameters deducible in function parameters.
> 
> ( I used to duplicate this `if` block for the `!FunctionCanBeCall` case but finally, I merged these two erroneously. :-( )
> A future improvement we could consider: if this is not a call, try to detect cases where the template parameters can be deduced from the surrounding context ("Deducing template arguments taking the address of a function template "). Maybe add a FIXME for this?

UPD: I've looked into the deduction implementation and found it not difficult to add such a heuristic: We have `PreferredType` that describes the type in a declaration, e.g., `void (*)(int, int)` and `DeduceTemplateArgumentsByTypeMatch` in Sema to tell if the template parameter type `P` could be deduced from the 'synthesized' type `A` (That is, the declaring type `void (int, int)`, for example). 

I suppose we could apply such a deduction for each candidate and eliminate its template parameters in the result if the deduction succeeds - But let me leave that for the next patch(es) so I don't overload you. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605