[clang][clangd] Improve signature help for variadic functions.

[adamcz]

Summary

This covers both C-style variadic functions and template variadic w/
parameter packs.

Previously we would return no signatures when working with template
variadic functions once activeParameter reached the position of the
parameter pack (except when it was the only param, then we'd still
show it when no arguments were given). With this commit, we now show
signathure help correctly.

Additionally, this commit fixes the activeParameter value in LSP output
of clangd in the presence of variadic functions (both kinds). LSP does
not allow the activeParamter to be higher than the number of parameters
in the active signature. With "..." or parameter pack being just one
argument, for all but first argument passed to "..." we'd report
incorrect activeParameter value. Clients such as VSCode would then treat
it as 0, as suggested in the spec) and highlight the wrong parameter.

In the future, we should add support for per-signature activeParamter
value, which exists in LSP since 3.16.0. This is not part of this
commit.

[sammccall]

Thanks, this looks great, just want to check if there are more callsites we can hit while here.

clang/lib/Sema/SemaOverload.cpp

6460

Hmm, there are something like 6 callers of TooManyArguments.
They all check isVariadic, and probably should all be adjusted in this way (except in once case where PartialOverloading is constant false).
This includes one almost-identical caller in this file, for AddMethodCandidate...

It's tempting to say that we should just pass some more args into TooManyArguments and have it do this logic internally. Maybe pass the FunctionType and the FunctionDecl* in, with the latter nullable?
At least this would keep this nice comment in one place.

[adamcz]

addressed review comment

[adamcz]

Sorry for delay, I got distracted with other stuff. I addressed your comment, partially, and also added more tests and fixed one more issue (see the FunctionType test, it would've failed before).

clang/lib/Sema/SemaOverload.cpp

6460

TooManyArguments is called in 5 places:

• Here
• Below, in the method version, which is identical to this case
• In ProduceCallSignatureHelp, where function can be variadic, but it cannot be a template, so no change needed and we don't even have FunctionDecl (just FunctionType)
• in checkDirectCallValidity, where PartialOverloading is unconditionally false, none of this matters and we shouldn't be checking isTemplateVariadic()
• in DeduceTemplateArguments, where we do check isTemplateVariadic() already, but we shouldn't be looking at template instantiation pattern.

additionally there is mergeCandidatesWithResult, where we don't call TooManyArguments, but need this logic too. If we did call TooManyArguments, we'd have to pass PartialOverloading = false, but still check for template variadic stuff.

We only need the pattern logic in 2 out of 5 places and, considering how specific to code completion/sig help it is, and how weird it is too, I'm leaning towards having this be a separate function.

I renamed the helper to something more appropriate and added a call to it in the method case. Let me know if this is better, or if you think I definitely should put this in TooManyArguments() (I'm leaning towards this solution, but don't have a strong opinion on this myself).

[sammccall]

Thanks, this looks nice!

Since the code is in Sema, we'd should (also) test this in clang/test/CodeCompletion/ if possible.

clang-tools-extra/clangd/CodeComplete.cpp

928

I'd consider pulling out the logic into a helper named something like paramIndexForArg(int, OverloadCandidate)

just because this function is ridiculously long and this piece of logic encapsulates nicely

929

hmm, now that I think about it, should this conversion from arg-index to param-index be happening here, or done already by sema and the param index passed to ProcessOverloadCandidates?

(The docs say "current arg" but I'm struggling to see how that can really be useful)

945

hmm, if foo() is a non-variadic 0-arg function, we set activeParameter to -1.
Before this patch it's zero which is still weird, but less weird.

[adamcz]

addressed review comments, added new test

[adamcz]

Added a slightly awkward clang (not clangd) test. The output is suboptimial, because it's not clangd, but it shows things work as they should.

clang-tools-extra/clangd/CodeComplete.cpp

929

Since ProcessOverloadCandidates is called with multiple candidates CurrentParam wouldn't make sense - which candidate would that be referring to? What if the candidates are:

void fun1(int x, ...);
void fun2(int x, int y, ...);

and CurrentArg is 3, that means for the first signature activeParameter = 1, while for the second one activeParameter = 2.

Right now we do not return this information, but we should (I left a FIXME and intend to follow up, LSP already has the per-signature field).

We could consider moving the CurrentArg out of this function signature and into the OverloadCandidate, so it's per candidate in Sema too. Is that what you're suggesting? I think the current state of things is fine, but if you think I should remove CurrentArg and replace it with a field in OverloadCandidate, let me know.

945

Right, that's unintended. Fixed.

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

929

Whoops, of course you're right, it's more complicated.

In principle, this seems like it does belong on OverloadCandidate. Doing this as a field seems like a yak-shave, maybe it makes sense to make paramIndexForArg a method on OverloadCandidate though. (And still require CurrentArg as input). Up to you, this is fine as it is.

clang-tools-extra/clangd/CodeComplete.cpp

void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
     SigHelp.signatures.reserve(NumCandidates);
     ScoredSignatures.reserve(NumCandidates);
     // FIXME(rwols): How can we determine the "active overload candidate"?
     // Right now the overloaded candidates seem to be provided in a "best fit"
     // order, so I'm not too worried about this.
     SigHelp.activeSignature = 0;
     assert(CurrentArg <= (unsigned)std::numeric_limits<int>::max() &&
            "too many arguments");
+
     SigHelp.activeParameter = static_cast<int>(CurrentArg);
+
     for (unsigned I = 0; I < NumCandidates; ++I) {
       OverloadCandidate Candidate = Candidates[I];
       // We want to avoid showing instantiated signatures, because they may be
       // long in some cases (e.g. when 'T' is substituted with 'std::string', we
       // would get 'std::basic_string<char>').
       if (auto *Func = Candidate.getFunction()) {
         if (auto *Pattern = Func->getTemplateInstantiationPattern())
           Candidate = OverloadCandidate(Pattern);
       }
+      if (static_cast<int>(I) == SigHelp.activeSignature &&
+          Candidate.getFunction()) {
+        // The activeParameter in LSP relates to the activeSignature. There is
+        // another, per-signature field, but we currently do not use it and not
+        // all clients might support it.
+        // FIXME: Add support for per-signature activeParameter field.
+        //
+        // This only matters for variadic functions/templates, where CurrentArg
+        // might be higher than the number of parameters. When that happens, we
+        // assume the last parameter is variadic and assume all further args are
+        // part of it.
+        int NumParams = Candidate.getFunction()->getNumParams();
+        if (Candidate.getFunction()->isVariadic())
+          ++NumParams;
+        SigHelp.activeParameter =
+            std::min(SigHelp.activeParameter, NumParams - 1);
+      }
 
       const auto *CCS = Candidate.CreateSignatureString(
           CurrentArg, S, *Allocator, CCTUInfo, true);
       assert(CCS && "Expected the CodeCompletionString to be non-null");
       ScoredSignatures.push_back(processOverloadCandidate(
           Candidate, *CCS,
           Candidate.getFunction()
               ? getDeclComment(S.getASTContext(), *Candidate.getFunction())
               : ""));
     }
 
     // Sema does not load the docs from the preamble, so we need to fetch extra
     // docs from the index instead.
     llvm::DenseMap<SymbolID, std::string> FetchedDocs;

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

const auto Results = signatures(R"cpp(
       };
     )cpp");
     EXPECT_THAT(Results.signatures,
                 UnorderedElementsAre(Sig("A([[int]])"), Sig("A([[A &&]])"),
                                      Sig("A([[const A &]])")));
   }
 }
 
+TEST(SignatureHelpTest, Variadic) {
+  const std::string Header = R"cpp(
+    void fun(int x, ...) {}
+    void test() {)cpp";
+  const std::string ExpectedSig = "fun([[int x]], [[...]]) -> void";
+
+  {
+    const auto Result = signatures(Header + "fun(^);}");
+    EXPECT_EQ(0, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+  {
+    const auto Result = signatures(Header + "fun(1, ^);}");
+    EXPECT_EQ(1, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+  {
+    const auto Result = signatures(Header + "fun(1, 2, ^);}");
+    EXPECT_EQ(1, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+}
+
+TEST(SignatureHelpTest, VariadicTemplate) {
+  const std::string Header = R"cpp(
+    template<typename T, typename ...Args>
+    void fun(T t, Args ...args) {}
+    void test() {)cpp";
+  const std::string ExpectedSig = "fun([[T t]], [[Args args...]]) -> void";
+
+  {
+    const auto Result = signatures(Header + "fun(^);}");
+    EXPECT_EQ(0, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+  {
+    const auto Result = signatures(Header + "fun(1, ^);}");
+    EXPECT_EQ(1, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+  {
+    const auto Result = signatures(Header + "fun(1, 2, ^);}");
+    EXPECT_EQ(1, Result.activeParameter);
+    EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+  }
+}
+
 TEST(CompletionTest, IncludedCompletionKinds) {
   Annotations Test(R"cpp(#include "^)cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.AdditionalFiles["sub/bar.h"] = "";
   TU.ExtraArgs.push_back("-I" + testPath("sub"));
 
   auto Results = completions(TU, Test.point());
   EXPECT_THAT(Results.Completions,

clang/include/clang/Sema/Overload.h

ConstructorInfo Info = {DeclAccessPair::make(ND, D->getAccess()), nullptr,
                             nullptr};
     Info.ConstructorTmpl = dyn_cast<FunctionTemplateDecl>(D);
     if (Info.ConstructorTmpl)
       D = Info.ConstructorTmpl->getTemplatedDecl();
     Info.Constructor = dyn_cast<CXXConstructorDecl>(D);
     return Info;
   }
 
+  // Returns true if signature help is relevant despite number of arguments
+  // exceeding parameters. Specifically, it returns true when:
+  // * Function is variadic
+  // * Function is template variadic
+  // * Function is an instantiation of template variadic function
+  // The last case may seem strange. The idea is that if we added one more
+  // argument, we'd end up with a function similar to Function. Since, in the
+  // context of signature help and/or code completion, we do not know what the
+  // type of the next argument (that the user is typing) will be, this is as
+  // good candidate as we can get, despite the fact that it takes one less
+  // parameter.
+  bool isVariadicOrInstantiatedFromVariadicTemplate(FunctionDecl *Function);
+
 } // namespace clang
 
 #endif // LLVM_CLANG_SEMA_OVERLOAD_H

clang/lib/Sema/SemaCodeComplete.cpp

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) {
       if (Candidate.Function->isDeleted())
         continue;
-      if (!Candidate.Function->isVariadic() &&
+      if (!isVariadicOrInstantiatedFromVariadicTemplate(Candidate.Function) &&
           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;

clang/lib/Sema/SemaOverload.cpp

if (Constructor) {
     }
   }
 
   unsigned NumParams = Proto->getNumParams();
 
   // (C++ 13.3.2p2): A candidate function having fewer than m
   // parameters is viable only if it has an ellipsis in its parameter
   // list (8.3.5).
   if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
       !Proto->isVariadic()) {
+    // When PartialOverloading is true (code completion/signature help), we want
+    // to keep a candidate with less params as long as it's either
+    // [template] variadic or was instantiated from variadic template. In those
+    // cases, if we were to add another argument (which the user is currently
+    // doing) the signature would remain valid. In case of variadic templates it
+    // would actually be a different function (with one extra parameter), but
+    // for the purpose of signature help that's good enough.
+    if (!PartialOverloading ||
+        !isVariadicOrInstantiatedFromVariadicTemplate(Function)) {
       Candidate.Viable = false;
       Candidate.FailureKind = ovl_fail_too_many_arguments;
       return;
     }
+  }
 
   // (C++ 13.3.2p2): A candidate function having more than m parameters
   // is viable only if the (m+1)st parameter has a default argument
   // (8.3.6). For the purposes of overload resolution, the
   // parameter list is truncated on the right, so that there are
   // exactly m parameters.
   unsigned MinRequiredArgs = Function->getMinRequiredArguments();
   if (Args.size() < MinRequiredArgs && !PartialOverloading) {
Expr *Sema::FixOverloadedFunctionReference(Expr *E, DeclAccessPair Found,
   llvm_unreachable("Invalid reference to overloaded function");
 }
 
 ExprResult Sema::FixOverloadedFunctionReference(ExprResult E,
                                                 DeclAccessPair Found,
                                                 FunctionDecl *Fn) {
   return FixOverloadedFunctionReference(E.get(), Found, Fn);
 }
+
+bool clang::isVariadicOrInstantiatedFromVariadicTemplate(
+    FunctionDecl *Function) {
+  if (!Function)
+    return false;
+  if (Function->isVariadic())
+    return true;
+  if (const auto *Proto =
+          dyn_cast<FunctionProtoType>(Function->getFunctionType()))
+    if (Proto->isTemplateVariadic())
+      return true;
+  if (auto *Pattern = Function->getTemplateInstantiationPattern())
+    if (const auto *Proto =
+            dyn_cast<FunctionProtoType>(Pattern->getFunctionType()))
+      if (Proto->isTemplateVariadic())
+        return true;
+  return false;
+}