[clang-tools-extra] 55a7931 - [clang][clangd] Improve signature help for variadic functions.
Adam Czachorowski via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 18 07:02:50 PST 2021
Author: Adam Czachorowski
Date: 2021-11-18T15:50:47+01:00
New Revision: 55a79318c60d8a39329195f43bf43b89da9a638e
URL: https://github.com/llvm/llvm-project/commit/55a79318c60d8a39329195f43bf43b89da9a638e
DIFF: https://github.com/llvm/llvm-project/commit/55a79318c60d8a39329195f43bf43b89da9a638e.diff
LOG: [clang][clangd] Improve signature help for variadic functions.
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.
Differential Revision: https://reviews.llvm.org/D111318
Added:
clang/test/CodeCompletion/variadic-template.cpp
Modified:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/Overload.h
clang/lib/Sema/SemaCodeComplete.cpp
clang/lib/Sema/SemaOverload.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index f033b5ec001d8..ac6c3b077d472 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -872,6 +872,28 @@ struct ScoredSignature {
SignatureQualitySignals Quality;
};
+// Returns the index of the parameter matching argument number "Arg.
+// This is usually just "Arg", except for variadic functions/templates, where
+// "Arg" 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 paramIndexForArg(const CodeCompleteConsumer::OverloadCandidate &Candidate,
+ int Arg) {
+ int NumParams = 0;
+ if (const auto *F = Candidate.getFunction()) {
+ NumParams = F->getNumParams();
+ if (F->isVariadic())
+ ++NumParams;
+ } else if (auto *T = Candidate.getFunctionType()) {
+ if (auto *Proto = T->getAs<FunctionProtoType>()) {
+ NumParams = Proto->getNumParams();
+ if (Proto->isVariadic())
+ ++NumParams;
+ }
+ }
+ return std::min(Arg, std::max(NumParams - 1, 0));
+}
+
class SignatureHelpCollector final : public CodeCompleteConsumer {
public:
SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
@@ -902,7 +924,9 @@ class SignatureHelpCollector final : public CodeCompleteConsumer {
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
@@ -912,6 +936,14 @@ class SignatureHelpCollector final : public CodeCompleteConsumer {
if (auto *Pattern = Func->getTemplateInstantiationPattern())
Candidate = OverloadCandidate(Pattern);
}
+ if (static_cast<int>(I) == SigHelp.activeSignature) {
+ // 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.
+ SigHelp.activeParameter =
+ paramIndexForArg(Candidate, SigHelp.activeParameter);
+ }
const auto *CCS = Candidate.CreateSignatureString(
CurrentArg, S, *Allocator, CCTUInfo, true);
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index 81dfdcb371fb4..5612a0b17fbba 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2622,6 +2622,104 @@ TEST(SignatureHelpTest, ConstructorInitializeFields) {
}
}
+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(SignatureHelpTest, VariadicMethod) {
+ const std::string Header = R"cpp(
+ class C {
+ template<typename T, typename ...Args>
+ void fun(T t, Args ...args) {}
+ };
+ void test() {C c; )cpp";
+ const std::string ExpectedSig = "fun([[T t]], [[Args args...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "c.fun(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "c.fun(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "c.fun(1, 2, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+}
+
+TEST(SignatureHelpTest, VariadicType) {
+ const std::string Header = R"cpp(
+ void fun(int x, ...) {}
+ auto get_fun() { return fun; }
+ void test() {
+ )cpp";
+ const std::string ExpectedSig = "([[int]], [[...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "get_fun()(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "get_fun()(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "get_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());
diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index 7898a58c27960..88405a63b735d 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -1204,6 +1204,20 @@ class Sema;
return Info;
}
+ // Returns false if signature help is relevant despite number of arguments
+ // exceeding parameters. Specifically, it returns false when
+ // PartialOverloading is true and one of the following:
+ // * 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 shouldEnforceArgLimit(bool PartialOverloading, FunctionDecl *Function);
+
} // namespace clang
#endif // LLVM_CLANG_SEMA_OVERLOAD_H
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index 26e982686cfbb..083a67db7a91d 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/clang/lib/Sema/SemaCodeComplete.cpp
@@ -5818,7 +5818,8 @@ static void mergeCandidatesWithResults(
if (Candidate.Function) {
if (Candidate.Function->isDeleted())
continue;
- if (!Candidate.Function->isVariadic() &&
+ if (shouldEnforceArgLimit(/*PartialOverloading=*/true,
+ 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
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index cce5d901ba903..42b1340f9a65d 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6456,7 +6456,8 @@ void Sema::AddOverloadCandidate(
// parameters is viable only if it has an ellipsis in its parameter
// list (8.3.5).
if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
- !Proto->isVariadic()) {
+ !Proto->isVariadic() &&
+ shouldEnforceArgLimit(PartialOverloading, Function)) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_too_many_arguments;
return;
@@ -6946,7 +6947,8 @@ Sema::AddMethodCandidate(CXXMethodDecl *Method, DeclAccessPair FoundDecl,
// parameters is viable only if it has an ellipsis in its parameter
// list (8.3.5).
if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
- !Proto->isVariadic()) {
+ !Proto->isVariadic() &&
+ shouldEnforceArgLimit(PartialOverloading, Method)) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_too_many_arguments;
return;
@@ -15242,3 +15244,21 @@ ExprResult Sema::FixOverloadedFunctionReference(ExprResult E,
FunctionDecl *Fn) {
return FixOverloadedFunctionReference(E.get(), Found, Fn);
}
+
+bool clang::shouldEnforceArgLimit(bool PartialOverloading,
+ FunctionDecl *Function) {
+ if (!PartialOverloading || !Function)
+ return true;
+ if (Function->isVariadic())
+ return false;
+ if (const auto *Proto =
+ dyn_cast<FunctionProtoType>(Function->getFunctionType()))
+ if (Proto->isTemplateVariadic())
+ return false;
+ if (auto *Pattern = Function->getTemplateInstantiationPattern())
+ if (const auto *Proto =
+ dyn_cast<FunctionProtoType>(Pattern->getFunctionType()))
+ if (Proto->isTemplateVariadic())
+ return false;
+ return true;
+}
diff --git a/clang/test/CodeCompletion/variadic-template.cpp b/clang/test/CodeCompletion/variadic-template.cpp
new file mode 100644
index 0000000000000..099ef3a6961cf
--- /dev/null
+++ b/clang/test/CodeCompletion/variadic-template.cpp
@@ -0,0 +1,18 @@
+template <typename T, typename... Args>
+void fun(T x, Args... args) {}
+
+void f() {
+ fun(1, 2, 3, 4);
+ // The results are quite awkward here, but it's the best we can do for now.
+ // Tools, including clangd, can unexpand "args" when showing this to the user.
+ // The important thing is that we provide OVERLOAD signature in all those cases.
+ //
+ // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:5:7 %s -o - | FileCheck --check-prefix=CHECK-1 %s
+ // CHECK-1: OVERLOAD: [#void#]fun(<#T x#>, Args args...)
+ // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:5:10 %s -o - | FileCheck --check-prefix=CHECK-2 %s
+ // CHECK-2: OVERLOAD: [#void#]fun(int x)
+ // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:5:13 %s -o - | FileCheck --check-prefix=CHECK-3 %s
+ // CHECK-3: OVERLOAD: [#void#]fun(int x, int args)
+ // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:5:16 %s -o - | FileCheck --check-prefix=CHECK-4 %s
+ // CHECK-4: OVERLOAD: [#void#]fun(int x, int args, int args)
+}
More information about the cfe-commits
mailing list