[CodeComplete] Improve FunctionCanBeCall

[zyounan]

Summary

This tries to avoid extra calculations in D137040.

In that patch the extra completion strings were dropped at the client
site, after function parameters became available, to implement the
heuristic. However, we can make the process a bit more terse: In the
context where a call isn't required, we could teach the Sema to emit
the completion string without additional function parameters.

In addition, this patch adds support for function templates to
FunctionCanBeCall as well. In such context, we'd emit extra template
argument placeholders that help avoiding unresolved overloads error.

[zyounan]

Changing the code completion string to RK_Pattern makes the completion clearer, in a way that showing candidates with signatures in the completion items if FunctionCanBeCall while a mere name otherwise.

(Comparing to that we always show signatures even if no parameters are produced when hit the item.)

->

---

P.S. Despite the fact that I changed many associated tests, I still hope people don't rely on the former behavior so that this patch could be marked as NFC. :D

I discovered that patch when trying to figure out why clangd doesn't complete function parameters for members in global scope while it produces parameters when I'm inside a call expression. Regard to the heuristic itself, it's currently a bit inconvenient that I have to switch headers and sources back and forth and do copy-pastes when I'm writing member function definitions outside of a class. I think we could be more lenient here e.g., don't set the flag on if we're in the global scope. What do you guys think? (I'm working on another follow-up patch to address this, although it is not strightforward to tell which scope the building Declarator is in inside Sema. )

[zyounan]

This comment was removed by zyounan.

[sammccall]

I'm not sure switching to RK_Pattern is ideal, but I think there are a couple of (easy?) alternatives that achieve the same.

Regard to the heuristic itself, it's currently a bit inconvenient that I have to switch headers and sources back and forth and do copy-pastes when I'm writing member function definitions outside of a class.

Yeah, it would be nice to have completions that provide the declaration syntax.
There's currently support for adding declarations of method overrides to derived classes: see AddOverrideResults in SemaCodeComplete, this should be kind-of similar.
These *do* use RK_Pattern, and I think it's appropriate there. (Though it does cause some ranking problems...)

clang-tools-extra/clangd/CodeComplete.cpp

441

I guess an alternative to this patch would be to clear S.Signature here too?

clang/lib/Sema/SemaCodeComplete.cpp

1422

the problem (from clangd's perspective) with emitting this as a pattern is we don't know which decl this is, and so we lose:

• ranking information (score popular decls higher)
• semantic information (this is a function and should be marked in LSP as such)

Is it possible to change the behavior of Result::CreateCodeCompletionString() instead, based on Result::FunctionCanBeCall? Then we get to keep RK_Declaration and get the right string

[zyounan]

Don't change the CCR. Handle these in CreateCodeCompletionString. Emit template arguments if possible.

[zyounan]

Format

[zyounan]

Thanks for the insightful suggestions!
(Apologies for my late update. Just had a really busy week.)

As suggested, I left the CCR intact and handled these functions in CodeCompletionResult::createCodeCompletionStringForDecl. I think this preserves the Declaration, right? (While I think we could get the associated Decl if using RK_Pattern, however the current approach looks more terse to me.)

I also noticed that the previous implementation did not consider function templates. For example,

struct S {
  template <typename T>
  void foo(T);

  void bar();
};

&S::bar^ // getting `S::bar`
&S::foo^ // getting `S::foo(T)`!

This brings a discrepancy, which I have also fixed in this patch. I hope this doesn't cause too much inconvenience for the review :)

[zyounan]

clang/lib/Sema/SemaCodeComplete.cpp

1407

The current heuristic results in the following:

struct Base {
  void foo(int);
};
struct Derived : Base {
  void foo(float);
};

int main() {
  Derived d;
  d.Base::^    // FunctionCanBeCall for `foo` is false.
}

In situations where it is intended to invoke base member functions hidden by subclasses, we shouldn't remove parentheses, IMO.

[zyounan]

clang/lib/Sema/SemaCodeComplete.cpp

1407

D156605 to address this.

[nridge]

I'm not sure how I feel about dropping the parameters from the signature in the CanBeCall = false case.

I can see arguments in both directions:

• On the one hand, dropping the parameters makes the text that is inserted more consistent with the text that is shown for the proposal.
• On the other hand, imagine you're typing in a CanBeCall = false context (e.g. &ClassName::Prefix^), and there are several matching method names that begin with Prefix that you are trying to choose from. Seeing their signatures might be a useful piece of additional context to help you choose the right one (e.g. maybe you're looking for one with a particular parameter type).

On the whole, I think I lean towards the second point being more important, and therefore towards keeping the signatures.

[zyounan]

In D155370#4545763, @nridge wrote:

(e.g. maybe you're looking for one with a particular parameter type).

I understand the second point that it'd be nice to offer the user a chance to see the parameters in order to help decide if the function is appropriate -- although in the context where CanBeCall=false, parameters don't disambiguate against the overloads, so we'd end up with the same function name after selecting the candidate. (An explicit cast may be required to perform overload resolution if necessary, but that should occur before the completion point &ClassName::Prefix^.)

OTOH, we don't present parameters for overloads in the candidates (using the clangd without this patch):

(At least for VSCode, I'm not sure if others behave the same.)

So, I don't think it is that important to retain the Signature. Any thoughts?

[nridge]

In D155370#4550042, @zyounan wrote:

in the context where CanBeCall=false, parameters don't disambiguate against the overloads, so we'd end up with the same function name after selecting the candidate

I was thinking of functions with different names (with a common prefix) and different signatures, where the signature could be a useful piece of information in addition to the name to help you choose the right one.

For example, consider:

struct Thread {
  void sleep_until(time_point);
  void sleep_for(time_duration);
};

&Thread::sleep^

Maybe the distinction between the functions is less obvious from their names (until vs. for), but more obvious from the type of argument they take.

In D155370#4550042, @zyounan wrote:

An explicit cast may be required to perform overload resolution if necessary, but that should occur before the completion point &ClassName::Prefix^.

(That would be an interesting future enhancement to consider for the overload set case.)

OTOH, we don't present parameters for overloads in the candidates (using the clangd without this patch):

(At least for VSCode, I'm not sure if others behave the same.)

Note, we have a flag that affects this, --completion-style=detailed.

[zyounan]

In D155370#4552967, @nridge wrote:

I was thinking of functions with different names (with a common prefix) and different signatures, where the signature could be a useful piece of information in addition to the name to help you choose the right one.

Ah, that makes sense! Honestly, I've never thought the signature could assist such a scenario.

Note, we have a flag that affects this, --completion-style=detailed.

Thanks. Just learned this flag.

Apart from the Signature, I still have two improvements for CanBeCall. I think it's better to close this review and merge these two changes into one patch.

[zyounan]

Closing this in favor of D156605.

clang-tools-extra/clangd/CodeComplete.cpp

struct CodeCompletionBuilder {
 
   void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
     assert(bool(C.SemaResult) == bool(SemaCCS));
     Bundled.emplace_back();
     BundledEntry &S = Bundled.back();
     if (C.SemaResult) {
       getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix, C.SemaResult->Kind,
                    C.SemaResult->CursorKind, &Completion.RequiredQualifier);
-      if (!C.SemaResult->FunctionCanBeCall)
-        S.SnippetSuffix.clear();
       S.ReturnType = getReturnType(*SemaCCS);
     } else if (C.IndexResult) {
       S.Signature = std::string(C.IndexResult->Signature);
       S.SnippetSuffix = std::string(C.IndexResult->CompletionSnippetSuffix);
       S.ReturnType = std::string(C.IndexResult->ReturnType);
     }
     if (!Completion.Documentation) {
       auto SetDoc = [&](llvm::StringRef Doc) {

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

 TEST(CompletionTest, HeuristicsForMemberFunctionCompletion) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
 
   Annotations Code(R"cpp(
       struct Foo {
         static int staticMethod();
         int method() const;
+        template <typename T, int U>
+        void generic(T);
+        template <typename T, int U = 3>
+        static T staticGeneric();
         Foo() {
-          this->$keepSnippet^
-          $keepSnippet^
-          Foo::$keepSnippet^
+          this->$canBeCall^
+          $canBeCall^
+          Foo::$canBeCall^
         }
       };
 
       struct Derived : Foo {
         Derived() {
-          Foo::$keepSnippet^
+          Foo::$canBeCall^
         }
       };
 
       struct OtherClass {
         OtherClass() {
           Foo f;
-          f.$keepSnippet^
-          &Foo::$noSnippet^
+          f.$canBeCall^
+          &Foo::$canNotBeCall^
         }
       };
 
       int main() {
         Foo f;
-        f.$keepSnippet^
-        &Foo::$noSnippet^
+        f.$canBeCall^
+        &Foo::$canNotBeCall^
       }
       )cpp");
   auto TU = TestTU::withCode(Code.code());
 
-  for (const auto &P : Code.points("noSnippet")) {
+  for (const auto &P : Code.points("canNotBeCall")) {
     auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
     EXPECT_THAT(Results.Completions,
                 Contains(AllOf(named("method"), snippetSuffix(""))));
+    EXPECT_THAT(
+        Results.Completions,
+        Contains(AllOf(named("generic"), signature("<typename T, int U>"),
+                       snippetSuffix("<${1:typename T}, ${2:int U}>"))));
   }
 
-  for (const auto &P : Code.points("keepSnippet")) {
+  for (const auto &P : Code.points("canBeCall")) {
     auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
     EXPECT_THAT(Results.Completions,
                 Contains(AllOf(named("method"), snippetSuffix("()"))));
+    EXPECT_THAT(Results.Completions,
+                Contains(AllOf(
+                    named("generic"), signature("<typename T, int U>(T)"),
+                    snippetSuffix("<${1:typename T}, ${2:int U}>(${3:T})"))));
   }
 
   // static method will always keep the snippet
   for (const auto &P : Code.points()) {
     auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
     EXPECT_THAT(Results.Completions,
                 Contains(AllOf(named("staticMethod"), snippetSuffix("()"))));
+    EXPECT_THAT(
+        Results.Completions,
+        Contains(AllOf(named("staticGeneric"), signature("<typename T>()"),
+                       snippetSuffix("<${1:typename T}>()"))));
   }
 }
 
 TEST(CompletionTest, NoSnippetsInUsings) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
   auto Results = completions(
       R"cpp(

clang/lib/Sema/SemaCodeComplete.cpp

if (const auto *Method = dyn_cast<CXXMethodDecl>(R.Declaration))
         }
         OverloadSet.Add(Method, Results.size());
       }
 
   // 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.
   if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
-    const auto *Method = dyn_cast<CXXMethodDecl>(R.getDeclaration());
+    const NamedDecl *ND = R.getDeclaration();
+    if (const auto *FuncTmpl = dyn_cast<FunctionTemplateDecl>(ND)) {
+      ND = FuncTmpl->getTemplatedDecl();
+    }
+    const auto *Method = dyn_cast<CXXMethodDecl>(ND);
     if (Method && !Method->isStatic()) {
       // Find the class scope that we're currently in.
       // We could e.g. be inside a lambda, so walk up the DeclContext until we
       // find a CXXMethodDecl.
       const auto *CurrentClassScope = [&]() -> const CXXRecordDecl * {
         for (DeclContext *Ctx = SemaRef.CurContext; Ctx;
              Ctx = Ctx->getParent()) {
           const auto *CtxMethod = llvm::dyn_cast<CXXMethodDecl>(Ctx);
           if (CtxMethod && !CtxMethod->getParent()->isLambda()) {
             return CtxMethod->getParent();
           }
         }
         return nullptr;
       }();
 
       R.FunctionCanBeCall =
           CurrentClassScope &&
           (CurrentClassScope == Method->getParent() ||
            CurrentClassScope->isDerivedFrom(Method->getParent()));
     }
   }
 
   // Insert this result into the set of results.
   Results.push_back(R);
 
   if (!AsNestedNameSpecifier)
     MaybeAddConstructorResults(R);
 }
 
 void ResultBuilder::AddResult(Result R) {
   assert(R.Kind != Result::RK_Declaration &&
          "Declaration results need more context");
   Results.push_back(R);
 }
 
 /// Enter into a new scope.
 void ResultBuilder::EnterNewScope() { ShadowMaps.emplace_back(); }
 
 /// Exit from the current scope.
CodeCompletionString *CodeCompletionResult::createCodeCompletionStringForDecl(
   for (const auto *I : ND->specific_attrs<AnnotateAttr>())
     Result.AddAnnotation(Result.getAllocator().CopyString(I->getAnnotation()));
 
   auto AddFunctionTypeAndResult = [&](const FunctionDecl *Function) {
     AddResultTypeChunk(Ctx, Policy, Function, CCContext.getBaseType(), Result);
     AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
                                    Ctx, Policy);
     AddTypedNameChunk(Ctx, Policy, ND, Result);
+    // We don't emit parameters (and qualifiers) for context where it can't be a
+    // call.
+    if (!FunctionCanBeCall)
+      return;
     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);
for (LastDeducibleArgument = Deduced.size(); LastDeducibleArgument > 0;
               cast<TemplateTemplateParmDecl>(Param)->hasDefaultArgument();
         }
 
         if (!HasDefaultArg)
           break;
       }
     }
 
+    if (!FunctionCanBeCall) {
+      // If this isn't a call, emit all the template arguments
+      // to disambiguate the (potential) overloads.
+      Result.AddChunk(CodeCompletionString::CK_LeftAngle);
+      AddTemplateParameterChunks(Ctx, Policy, FunTmpl, Result);
+      Result.AddChunk(CodeCompletionString::CK_RightAngle);
+      return Result.TakeString();
+    }
+
     if (LastDeducibleArgument) {
       // Some of the function template arguments cannot be deduced from a
       // function call, so we introduce an explicit template argument list
       // containing all of the arguments up to the first deducible argument.
       Result.AddChunk(CodeCompletionString::CK_LeftAngle);
       AddTemplateParameterChunks(Ctx, Policy, FunTmpl, Result,
                                  LastDeducibleArgument);
       Result.AddChunk(CodeCompletionString::CK_RightAngle);

clang/test/CodeCompletion/member-access.cpp

// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:159:11 %s -o - | FileCheck -check-prefix=CHECK-CC6 %s
   static void staticFn(T &obj);
 
   struct Nested { };
 };
 
 template<typename T>
 void dependentColonColonCompletion() {
   Template<T>::staticFn();
-// CHECK-CC7: function : [#void#]function()
+// CHECK-CC7: function : [#void#]function
 // CHECK-CC7: Nested : Nested
 // CHECK-CC7: o1 : [#BaseTemplate<int>#]o1
 // CHECK-CC7: o2 : [#BaseTemplate<T>#]o2
 // CHECK-CC7: staticFn : [#void#]staticFn(<#T &obj#>)
 // CHECK-CC7: Template : Template
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:173:16 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s
   typename Template<T>::Nested m;
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:181:25 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s

clang/test/Index/complete-qualified.cpp

 
 void foo()
 {
   Foo::
 
 // RUN: c-index-test -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: FieldDecl:{ResultType C<Foo, class Bar>}{TypedText c} (35)
 // CHECK-CC1: ClassDecl:{TypedText Foo} (35)
-// CHECK-CC1: CXXMethod:{ResultType Foo &}{TypedText operator=}{LeftParen (}{Placeholder const Foo &}{RightParen )}
-// CHECK-CC1: CXXDestructor:{ResultType void}{TypedText ~Foo}{LeftParen (}{RightParen )} (80)
+// CHECK-CC1: CXXMethod:{ResultType Foo &}{TypedText operator=}
+// CHECK-CC1: CXXDestructor:{ResultType void}{TypedText ~Foo} (80)

clang/unittests/Sema/CodeCompleteTest.cpp

SaveCompletedFunctions(std::vector<CompletedFunctionDecl> &CompletedFuncDecls)
         CCTUInfo(std::make_shared<GlobalCodeCompletionAllocator>()) {}
 
   void ProcessCodeCompleteResults(Sema &S, CodeCompletionContext Context,
                                   CodeCompletionResult *Results,
                                   unsigned NumResults) override {
     for (unsigned I = 0; I < NumResults; ++I) {
       auto R = Results[I];
       if (R.Kind == CodeCompletionResult::RK_Declaration) {
-        if (const auto *FD = llvm::dyn_cast<FunctionDecl>(R.getDeclaration())) {
+        auto *ND = R.getDeclaration();
+        if (auto *Template = llvm::dyn_cast<FunctionTemplateDecl>(ND))
+          ND = Template->getTemplatedDecl();
+        if (const auto *FD = llvm::dyn_cast<FunctionDecl>(ND)) {
           CompletedFunctionDecl D;
           D.Name = FD->getNameAsString();
           D.CanBeCall = R.FunctionCanBeCall;
           D.IsStatic = FD->isStatic();
           CompletedFuncDecls.emplace_back(std::move(D));
         }
       }
     }
CollectCompletedFunctions(StringRef Code, std::size_t Point) {
   return Result;
 }
 
 TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
   llvm::Annotations Code(R"cpp(
     struct Foo {
       static int staticMethod();
       int method() const;
+      template <typename T, int U>
+      void generic(T);
+      template <typename T, int U = 3>
+      static T staticGeneric();
       Foo() {
         this->$canBeCall^
         $canBeCall^
         Foo::$canBeCall^
       }
     };
 
     struct Derived : Foo {
int main() {
       &Foo::$cannotBeCall^
     }
     )cpp");
 
   for (const auto &P : Code.points("canBeCall")) {
     auto Results = CollectCompletedFunctions(Code.code(), P);
     EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
                                         canBeCall(true))));
+    EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+                                        canBeCall(true))));
   }
 
   for (const auto &P : Code.points("cannotBeCall")) {
     auto Results = CollectCompletedFunctions(Code.code(), P);
     EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
                                         canBeCall(false))));
+    EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+                                        canBeCall(false))));
   }
 
   // static method can always be a call
   for (const auto &P : Code.points()) {
     auto Results = CollectCompletedFunctions(Code.code(), P);
     EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true),
                                         canBeCall(true))));
+    EXPECT_THAT(Results, Contains(AllOf(named("staticGeneric"), isStatic(true),
+                                        canBeCall(true))));
   }
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSForValidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
      namespace ns1 {}
      namespace ns2 {}
      namespace ns3 {}