[clang] Make signature help work with dependent args

[kadircet]

Summary

Fixes https://github.com/clangd/clangd/issues/490

[hokein]

This looks like a nice improvement.

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

1212

These tests are nice.

But we're changing Sema in this patch, I think we'd better to have these in clang, /test/CodeCompletion/call.cpp seems to be the place.

clang/lib/Sema/SemaCodeComplete.cpp

5582

possible.

5584

I'm a bit nervous to show all overloads regardlessly, I think we could do some smarter things like

1. if there are some prefix non-type-dependent arguments, we could use these type information to filter out some unrelated candidates;
2. different overloads may have different number of parameters, this is an important signal, e.g. foo(t, ^t) should not give void foo(int) result;

2 seems quite critical and trivial to implement, maybe we can do it in this patch.

[kadircet]

• Add tests into clang-lit
• Make sure current number of args is less than overloads param count.

[kadircet]

• Fix typo in comment

[kadircet]

clang/lib/Sema/SemaCodeComplete.cpp

5584

if there are some prefix non-type-dependent arguments, we could use these type information to filter out some unrelated candidates;

well actually now that I think about it, this is actually not that hard. possibly even easier than current version. changing the logic surface every candidate using the prefix, and do a post-filtering instead of trying to mock all the results.

[kadircet]

• Change logic to find all signatures without any dependent args and post filter non-viable overloads using the argument counts.

[hokein]

thanks, looks better.

clang/lib/Sema/SemaCodeComplete.cpp

5513–5525

nit: use early return.

if (!Candidate.Function) {
  continue;
}
...

[kadircet]

clang/lib/Sema/SemaCodeComplete.cpp

5513–5525

i wanted to do that, but it changes the semantics. as push_back below executes even when Candidate.Function is nullptr :/

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

TEST(SignatureHelpTest, ActiveArg) {
   )cpp");
   EXPECT_THAT(Results.signatures,
               ElementsAre(Sig("baz([[int a]], [[int b]], [[int c]]) -> int")));
   EXPECT_EQ(0, Results.activeSignature);
   EXPECT_EQ(1, Results.activeParameter);
 }
 
 TEST(SignatureHelpTest, OpeningParen) {
-  llvm::StringLiteral Tests[] = {// Recursive function call.
-                                 R"cpp(
+  llvm::StringLiteral Tests[] = {
+      // Recursive function call.
         int foo(int a, int b, int c);
         int main() {
           foo(foo $p^( foo(10, 10, 10), ^ )));
         })cpp",
-                                 // Functional type cast.
-                                 R"cpp(
         struct Foo {
           Foo(int a, int b, int c);
         };
         int main() {
           Foo $p^( 10, ^ );
         })cpp",
-                                 // New expression.
-                                 R"cpp(
         struct Foo {
           Foo(int a, int b, int c);
         };
         int main() {
           new Foo $p^( 10, ^ );
         })cpp",
-                                 // Macro expansion.
-                                 R"cpp(
         int foo(int a, int b, int c);
         #define FOO foo(
 
         int main() {
           // Macro expansions.
           $p^FOO 10, ^ );
         })cpp",
-                                 // Macro arguments.
-                                 R"cpp(
         int foo(int a, int b, int c);
         int main() {
         #define ID(X) X
           // FIXME: figure out why ID(foo (foo(10), )) doesn't work when preserving
           // the recovery expression.
           ID(foo $p^( 10, ^ ))
-    })cpp"};
+        })cpp",
+      // Dependent args.
+      R"cpp(
+        int foo(int a, int b);
+        template <typename T> void bar(T t) {
+          foo$p^(t, ^t);
+        })cpp",
+      // Dependent args on templated func.
+      R"cpp(
+        template <typename T>
+        int foo(T, T);
+        template <typename T> void bar(T t) {
+          foo$p^(t, ^t);
+        })cpp",
+      // Dependent args on member.
+      R"cpp(
+        struct Foo { int foo(int, int); };
+        template <typename T> void bar(T t) {
+          Foo f;
+          f.foo$p^(t, ^t);
+        })cpp",
+      // Dependent args on templated member.
+      R"cpp(
+        struct Foo { template <typename T> int foo(T, T); };
+        template <typename T> void bar(T t) {
+          Foo f;
+          f.foo$p^(t, ^t);
+        })cpp",
+  };
 
   for (auto Test : Tests) {
     Annotations Code(Test);
     EXPECT_EQ(signatures(Code.code(), Code.point()).argListStart,
               Code.point("p"))
         << "Test source:" << Test;
   }
 }

clang/lib/Sema/SemaCodeComplete.cpp

static bool anyNullArguments(ArrayRef<Expr *> Args) {
 
   return false;
 }
 
 typedef CodeCompleteConsumer::OverloadCandidate ResultCandidate;
 
 static void mergeCandidatesWithResults(
     Sema &SemaRef, SmallVectorImpl<ResultCandidate> &Results,
-    OverloadCandidateSet &CandidateSet, SourceLocation Loc) {
+    OverloadCandidateSet &CandidateSet, SourceLocation Loc, size_t ArgSize) {
   // Sort the overload candidate set by placing the best overloads first.
   llvm::stable_sort(CandidateSet, [&](const OverloadCandidate &X,
                                       const OverloadCandidate &Y) {
     return isBetterOverloadCandidate(SemaRef, X, Y, Loc,
                                      CandidateSet.getKind());
   });
 
   // Add the remaining viable overload candidates as code-completion results.
   for (OverloadCandidate &Candidate : CandidateSet) {
-    if (Candidate.Function && Candidate.Function->isDeleted())
+    if (Candidate.Function) {
+      if (Candidate.Function->isDeleted())
         continue;
+      if (!Candidate.Function->isVariadic() &&
+          Candidate.Function->getNumParams() <= ArgSize &&
+          // Having zero args is annoying, normally we don't surface a function
+          // with 2 params, if you already have 2 params, because you are
+          // inserting the 3rd now. But with zero, it helps the user to figure
+          // out there are no overloads that take any arguments. Hence we are
+          // keeping the overload.
+          ArgSize > 0)
+        continue;
+    }
     if (Candidate.Viable)
       Results.push_back(ResultCandidate(Candidate.Function));
   }
 }
 
 /// Get the type of the Nth parameter from a given set of overload
 /// candidates.
 static QualType getParamType(Sema &SemaRef,

 QualType Sema::ProduceCallSignatureHelp(Scope *S, Expr *Fn,
                                         ArrayRef<Expr *> Args,
                                         SourceLocation OpenParLoc) {
   if (!CodeCompleter)
     return QualType();
 
   // FIXME: Provide support for variadic template functions.
   // Ignore type-dependent call expressions entirely.
-  if (!Fn || Fn->isTypeDependent() || anyNullArguments(Args) ||
-      Expr::hasAnyTypeDependentArguments(Args)) {
+  if (!Fn || Fn->isTypeDependent() || anyNullArguments(Args))
     return QualType();
-  }
+  // In presence of dependent args we surface all possible signatures using the
+  // non-dependent args in the prefix. Afterwards we do a post filtering to make
+  // sure provided candidates satisfy parameter count restrictions.
+  auto ArgsWithoutDependentTypes =
+      Args.take_while([](Expr *Arg) { return !Arg->isTypeDependent(); });
+
+  SmallVector<ResultCandidate, 8> Results;
 
+  Expr *NakedFn = Fn->IgnoreParenCasts();
   // Build an overload candidate set based on the functions we find.
   SourceLocation Loc = Fn->getExprLoc();
   OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal);
 
-  SmallVector<ResultCandidate, 8> Results;
-
-  Expr *NakedFn = Fn->IgnoreParenCasts();
-  if (auto ULE = dyn_cast<UnresolvedLookupExpr>(NakedFn))
-    AddOverloadedCallCandidates(ULE, Args, CandidateSet,
+  if (auto ULE = dyn_cast<UnresolvedLookupExpr>(NakedFn)) {
+    AddOverloadedCallCandidates(ULE, ArgsWithoutDependentTypes, CandidateSet,
                                 /*PartialOverloading=*/true);
-  else if (auto UME = dyn_cast<UnresolvedMemberExpr>(NakedFn)) {
+  } else if (auto UME = dyn_cast<UnresolvedMemberExpr>(NakedFn)) {
     TemplateArgumentListInfo TemplateArgsBuffer, *TemplateArgs = nullptr;
     if (UME->hasExplicitTemplateArgs()) {
       UME->copyTemplateArgumentsInto(TemplateArgsBuffer);
       TemplateArgs = &TemplateArgsBuffer;
     }
 
     // Add the base as first argument (use a nullptr if the base is implicit).
     SmallVector<Expr *, 12> ArgExprs(
         1, UME->isImplicitAccess() ? nullptr : UME->getBase());
-    ArgExprs.append(Args.begin(), Args.end());
+    ArgExprs.append(ArgsWithoutDependentTypes.begin(),
+                    ArgsWithoutDependentTypes.end());
     UnresolvedSet<8> Decls;
     Decls.append(UME->decls_begin(), UME->decls_end());
     const bool FirstArgumentIsBase = !UME->isImplicitAccess() && UME->getBase();
     AddFunctionCandidates(Decls, ArgExprs, CandidateSet, TemplateArgs,
                           /*SuppressUserConversions=*/false,
                           /*PartialOverloading=*/true, FirstArgumentIsBase);
   } else {
     FunctionDecl *FD = nullptr;
     if (auto *MCE = dyn_cast<MemberExpr>(NakedFn))
       FD = dyn_cast<FunctionDecl>(MCE->getMemberDecl());
     else if (auto *DRE = dyn_cast<DeclRefExpr>(NakedFn))
       FD = dyn_cast<FunctionDecl>(DRE->getDecl());
     if (FD) { // We check whether it's a resolved function declaration.
       if (!getLangOpts().CPlusPlus ||
           !FD->getType()->getAs<FunctionProtoType>())
         Results.push_back(ResultCandidate(FD));
       else
         AddOverloadCandidate(FD, DeclAccessPair::make(FD, FD->getAccess()),
-                             Args, CandidateSet,
+                             ArgsWithoutDependentTypes, CandidateSet,
                              /*SuppressUserConversions=*/false,
                              /*PartialOverloading=*/true);
 
     } else if (auto DC = NakedFn->getType()->getAsCXXRecordDecl()) {
       // If expression's type is CXXRecordDecl, it may overload the function
       // call operator, so we check if it does and add them as candidates.
       // A complete type is needed to lookup for member function call operators.
       if (isCompleteType(Loc, NakedFn->getType())) {
         DeclarationName OpName =
             Context.DeclarationNames.getCXXOperatorName(OO_Call);
         LookupResult R(*this, OpName, Loc, LookupOrdinaryName);
         LookupQualifiedName(R, DC);
         R.suppressDiagnostics();
         SmallVector<Expr *, 12> ArgExprs(1, NakedFn);
-        ArgExprs.append(Args.begin(), Args.end());
+        ArgExprs.append(ArgsWithoutDependentTypes.begin(),
+                        ArgsWithoutDependentTypes.end());
         AddFunctionCandidates(R.asUnresolvedSet(), ArgExprs, CandidateSet,
                               /*ExplicitArgs=*/nullptr,
                               /*SuppressUserConversions=*/false,
                               /*PartialOverloading=*/true);
       }
     } else {
       // Lastly we check whether expression's type is function pointer or
       // function.
       QualType T = NakedFn->getType();
       if (!T->getPointeeType().isNull())
         T = T->getPointeeType();
 
       if (auto FP = T->getAs<FunctionProtoType>()) {
-        if (!TooManyArguments(FP->getNumParams(), Args.size(),
+        if (!TooManyArguments(FP->getNumParams(),
+                              ArgsWithoutDependentTypes.size(),
                               /*PartialOverloading=*/true) ||
             FP->isVariadic())
           Results.push_back(ResultCandidate(FP));
       } else if (auto FT = T->getAs<FunctionType>())
         // No prototype and declaration, it may be a K & R style function.
         Results.push_back(ResultCandidate(FT));
     }
   }
-  mergeCandidatesWithResults(*this, Results, CandidateSet, Loc);
+  mergeCandidatesWithResults(*this, Results, CandidateSet, Loc, Args.size());
   QualType ParamType =
       ProduceSignatureHelp(*this, S, Results, Args.size(), OpenParLoc);
   return !CandidateSet.empty() ? ParamType : QualType();
 }
 
 QualType Sema::ProduceConstructorSignatureHelp(Scope *S, QualType Type,
                                                SourceLocation Loc,
                                                ArrayRef<Expr *> Args,
if (auto *FD = dyn_cast<FunctionDecl>(C)) {
           FTD, DeclAccessPair::make(FTD, C->getAccess()),
           /*ExplicitTemplateArgs=*/nullptr, Args, CandidateSet,
           /*SuppressUserConversions=*/false,
           /*PartialOverloading=*/true);
     }
   }
 
   SmallVector<ResultCandidate, 8> Results;
-  mergeCandidatesWithResults(*this, Results, CandidateSet, Loc);
+  mergeCandidatesWithResults(*this, Results, CandidateSet, Loc, Args.size());
   return ProduceSignatureHelp(*this, S, Results, Args.size(), OpenParLoc);
 }
 
 QualType Sema::ProduceCtorInitMemberSignatureHelp(
     Scope *S, Decl *ConstructorDecl, CXXScopeSpec SS, ParsedType TemplateTypeTy,
     ArrayRef<Expr *> ArgExprs, IdentifierInfo *II, SourceLocation OpenParLoc) {
   if (!CodeCompleter)
     return QualType();

clang/test/CodeCompletion/call.cpp

void test() {
   // CHECK-CC2: f(int i, int j, <#int k#>)
   f({}, 0, 0);
   // RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:28:7 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
   // CHECK-CC3: OVERLOAD: [#void#]f()
   // CHECK-CC3-NEXT: OVERLOAD: [#void#]f(<#X#>)
   // CHECK-CC3-NEXT: OVERLOAD: [#void#]f(<#int i#>, int j, int k)
   // CHECK-CC3-NEXT: OVERLOAD: [#void#]f(<#float x#>, float y)
 }
+
+void f(int, int, int, int);
+template <typename T>
+void foo(T t) {
+  f(t, t, t);
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:5 %s -o - | FileCheck -check-prefix=CHECK-CC4 %s
+  // CHECK-CC4: f()
+  // CHECK-CC4-NEXT: f(<#X#>)
+  // CHECK-CC4-NEXT: f(<#int i#>, int j, int k)
+  // CHECK-CC4-NEXT: f(<#float x#>, float y)
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:8 %s -o - | FileCheck -check-prefix=CHECK-CC5 %s
+  // CHECK-CC5-NOT: f()
+  // CHECK-CC5: f(int i, <#int j#>, int k)
+  // CHECK-CC5-NEXT: f(float x, <#float y#>)
+  f(5, t, t);
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:49:11 %s -o - | FileCheck -check-prefix=CHECK-CC6 %s
+  // CHECK-CC6-NOT: f(float x, float y)
+  // CHECK-CC6: f(int, int, <#int#>, int)
+  // CHECK-CC6-NEXT: f(int i, int j, <#int k#>)
+}