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

[Nathan Ridge via Phabricator via cfe-commits]

nridge requested changes to this revision.
nridge added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1297
+    FunctionCanBeCall =
+        MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase);
+  }
----------------
nit: we can avoid the computation in this block if `FunctionCanBeCall` was already initialized to `true` above


================
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) {
----------------
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 */)`.


================
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:
> > 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>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605