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

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thanks for Sam and Nathan's thorough review! I've updated, and please take another look.
(Apologize for churning on weekends, but I don't have chunk time during the week.)



================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
     case CodeCompletionString::CK_RightParen:
+      if (DropFunctionArguments &&
+          ResultKind == CodeCompletionResult::RK_Declaration)
----------------
sammccall wrote:
> nridge wrote:
> > It looks like we are assuming two things:
> > 
> >  1. Any LeftParen in an RK_Declaration starts a function argument list
> >  2. Everything that comes after the function argument list can be discarded
> > 
> > I think these assumptions are fine to make, but let's be explicit about them in a comment
> Agree, but also I think the code could reflect this more directly:
>  - this should trigger only for CK_LeftParen, not CK_RightParen
> 
> Rather than redirecting output to a different string by reassigning the param, I think it would be a bit more direct to have
> 
> ```
> optional<unsigned> TruncateSnippetAt;
> ...
> case CK_LeftBracket:
>   if (DropFunctionArguments && ... && !TruncateSnippetAt)
>     TruncateSnippetAt = Snippet->size();
> ...
> if (TruncateSnippetAt)
>   Snippet->resize(*TruncateSnippetAt);
> }
> ```
> 
> (though still not totally clear)
> It looks like we are assuming two things

Honestly, I don't quite prefer this assumption. However, we have lost some of the semantics e.g., the structure of a call expression within this function, and unfortunately it's not trivial to pass these out from clang. :-(

> (though still not totally clear)

Yeah. I imagine a clearer way would be separating the calculation for `Signature` and `Snippet`, and we could bail out early for `Snippet`. But I'm afraid that making so many adjustments to `getSignature` for such a small feature is inappropriate.

Assuming the left parenthesis is the beginning of a call might be sufficient for the time being, and let's leave the refactoring to subsequent patches.



================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
     auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
     EXPECT_THAT(Results.Completions,
                 Contains(AllOf(named("method"), snippetSuffix(""))));
----------------
nridge wrote:
> Since you're updating this test anyways, could you add `signature()` matchers for the non-template cases as well please?
Of course!


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
-                 bool InBaseClass);
+                 bool InBaseClass, QualType BaseType);
 
----------------
nridge wrote:
> The word "base" is a bit overloaded in this signature; in the parameter `InBaseClass` it refers to inheritance, but in `BaseType` it refers to the base expression of a `MemberExpr`.
> 
> Maybe we can name the new parameter `BaseExprType` to avoid confusion?
Makes sense to me. Thanks for the correction.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
     Result.ShadowDecl = Using;
     AddResult(Result, CurContext, Hiding);
     return;
----------------
nridge wrote:
> Should we propagate `BaseType` into this recursive call?
Yes. And thanks for spotting this. I added another test case reflecting it.


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


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+        R.FunctionCanBeCall =
+            MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase);
+      }
----------------
nridge wrote:
> Is there any situation where `MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase)` is false?
> 
> I am wondering if we can simplify this to:
> 
> ```
> if (!BaseType.isNull()) {
>   R.FunctionCanBeCall = true;
> }
> ```
I think this could prevent us from completing ill-formed expressions like:

```
struct B {
  void foo();
};

struct D : B {
  void bar();
};

struct S {
  void method();
};

void baz() {
  D().S::^
}
```

Currently, we have the candidate `method`, and completing it with parentheses is definitely wrong. (Although we shouldn't be providing `method` as well.)


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605