[clangd] Move function argument snippet disable mechanism from LSP rendering to internal clangd reprensentation.

[kadircet]

Summary

We were handling the EnableFunctionArgSnippets only when we are producing LSP
response. Move that code into CompletionItem generation so that internal clients
can benefit from that as well.

[ilya-biryukov]

LGTM

clangd/CodeComplete.cpp

 // Many fields are the same for all candidates in a bundle (e.g. name), and are
 // computed from the first candidate, in the constructor.
 // Others vary per candidate, so add() must be called for remaining candidates.
 struct CodeCompletionBuilder {
   CodeCompletionBuilder(ASTContext &ASTCtx, const CompletionCandidate &C,
                         CodeCompletionString *SemaCCS,
                         const IncludeInserter &Includes, StringRef FileName,
                         const CodeCompleteOptions &Opts)
-      : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+      : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments),
+        EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets) {
     add(C, SemaCCS);
     if (C.SemaResult) {
       Completion.Origin |= SymbolOrigin::AST;
       Completion.Name = llvm::StringRef(SemaCCS->getTypedText());
       if (Completion.Scope.empty()) {
         if ((C.SemaResult->Kind == CodeCompletionResult::RK_Declaration) ||
             (C.SemaResult->Kind == CodeCompletionResult::RK_Pattern))
           if (const auto *D = C.SemaResult->getDeclaration())
private:
 
   std::string summarizeReturnType() const {
     if (auto *RT = onlyValue<&BundledEntry::ReturnType>())
       return *RT;
     return "";
   }
 
   std::string summarizeSnippet() const {
-    if (auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>())
-      return *Snippet;
+    auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
+    if (!Snippet)
       // All bundles are function calls.
-    return "(${0})";
+      return "($0)";
+    if (!Snippet->empty() && !EnableFunctionArgSnippets &&
+        ((Completion.Kind == CompletionItemKind::Function) ||
+         (Completion.Kind == CompletionItemKind::Method)) &&
+        (Snippet->front() == '(') && (Snippet->back() == ')'))
+      // Check whether function has any parameters or not.
+      return Snippet->size() > 2 ? "($0)" : "()";
+    return *Snippet;
   }
 
   std::string summarizeSignature() const {
     if (auto *Signature = onlyValue<&BundledEntry::Signature>())
       return *Signature;
     // All bundles are function calls.
     return "(…)";
   }
 
   ASTContext &ASTCtx;
   CodeCompletion Completion;
   SmallVector<BundledEntry, 1> Bundled;
   bool ExtractDocumentation;
+  bool EnableFunctionArgSnippets;
 };
 
 // Determine the symbol ID for a Sema code completion result, if possible.
 llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R) {
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
     return clang::clangd::getSymbolID(R.Declaration);
CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const {
   for (const auto &FixIt : FixIts) {
     if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {
       LSP.textEdit->newText = FixIt.newText + LSP.textEdit->newText;
       LSP.textEdit->range.start = FixIt.range.start;
     } else {
       LSP.additionalTextEdits.push_back(FixIt);
     }
   }
-  if (Opts.EnableSnippets && !SnippetSuffix.empty()) {
-    if (!Opts.EnableFunctionArgSnippets &&
-        ((Kind == CompletionItemKind::Function) ||
-         (Kind == CompletionItemKind::Method)) &&
-        (SnippetSuffix.front() == '(') && (SnippetSuffix.back() == ')'))
-      // Check whether function has any parameters or not.
-      LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
-    else
-      LSP.textEdit->newText += SnippetSuffix;
-  }
+  if (Opts.EnableSnippets)
 
   // FIXME(kadircet): Do not even fill insertText after making sure textEdit is
   // compatible with most of the editors.
   LSP.insertText = LSP.textEdit->newText;
   LSP.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
                                              : InsertTextFormat::PlainText;
   if (HeaderInsertion)
     LSP.additionalTextEdits.push_back(*HeaderInsertion);

unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, OverloadBundling) {
   EXPECT_EQ(A.Name, "a");
   EXPECT_EQ(A.Signature, "(…)");
   EXPECT_EQ(A.BundleSize, 2u);
   EXPECT_EQ(A.Kind, CompletionItemKind::Method);
   EXPECT_EQ(A.ReturnType, "int"); // All overloads return int.
   // For now we just return one of the doc strings arbitrarily.
   EXPECT_THAT(A.Documentation, AnyOf(HasSubstr("Overload with int"),
                                      HasSubstr("Overload with bool")));
-  EXPECT_EQ(A.SnippetSuffix, "(${0})");
+  EXPECT_EQ(A.SnippetSuffix, "($0)");
 }
 
 TEST(CompletionTest, DocumentationFromChangedFileCrash) {
   MockFSProvider FS;
   auto FooH = testPath("foo.h");
   auto FooCpp = testPath("foo.cpp");
   FS.Files[FooH] = R"cpp(
     // this is my documentation comment.
EXPECT_THAT(
       signatures(Sig1, {Foo0, Foo1, Foo2}).signatures,
       ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
                   AllOf(Sig("foo(int) -> int", {"int"}),
                         SigDoc("Overriden doc from sema")),
                   AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
                         SigDoc("Doc from sema"))));
 }
 
-TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
-  Opts.EnableFunctionArgSnippets = true;
-  {
-    CodeCompletion C;
-    C.RequiredQualifier = "Foo::";
-    C.Name = "x";
-    C.SnippetSuffix = "()";
-
-    auto R = C.render(Opts);
-    EXPECT_EQ(R.textEdit->newText, "Foo::x");
-    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
-  }
-
   Opts.EnableSnippets = true;
   Opts.EnableFunctionArgSnippets = false;
+  const std::string Header =
+      R"cpp(
+      void xfoo();
+      void xfoo(int x, int y);
+      void xbar();
+      void f() {
+    )cpp";
   {
-    CodeCompletion C;
-    C.RequiredQualifier = "Foo::";
-    C.Name = "x";
-    C.SnippetSuffix = "";
-
-    auto R = C.render(Opts);
-    EXPECT_EQ(R.textEdit->newText, "Foo::x");
-    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+    auto Results = completions(Header + "\nxfo^", {}, Opts);
+    EXPECT_THAT(
+        Results.Completions,
+        UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("()")),
+                             AllOf(Named("xfoo"), SnippetSuffix("($0)"))));
   }
-
   {
-    CodeCompletion C;
-    C.RequiredQualifier = "Foo::";
-    C.Name = "x";
-    C.SnippetSuffix = "()";
-    C.Kind = CompletionItemKind::Method;
-
-    auto R = C.render(Opts);
-    EXPECT_EQ(R.textEdit->newText, "Foo::x()");
-    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+    auto Results = completions(Header + "\nxba^", {}, Opts);
+    EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+                                         Named("xbar"), SnippetSuffix("()"))));
   }
-
   {
-    CodeCompletion C;
-    C.RequiredQualifier = "Foo::";
-    C.Name = "x";
-    C.SnippetSuffix = "(${0:bool})";
-    C.Kind = CompletionItemKind::Function;
-
-    auto R = C.render(Opts);
-    EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
-    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+    Opts.BundleOverloads = true;
+    auto Results = completions(Header + "\nxfo^", {}, Opts);
+    EXPECT_THAT(
+        Results.Completions,
+        UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("($0)"))));
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang