Code completion should not ignore default parameters in functions.

[usaxena95]

Summary

Inorder to display the default arguments we must process the
CK_Optional chunks of CodeCompletionString while creating the Signature.

We do not create placeholders for default arguments.

[usaxena95]

Removed unused include of logger.

[sammccall]

clang-tools-extra/clangd/CodeCompletionStrings.cpp

35 ↗

(On Diff #213871)

maybe appendOptionalChunk? if in doubt, better to describe the semantics of the param than its type, since the type appears in the signature

156 ↗

(On Diff #213871)

please drop the extra message from the assert unless there's something additional to say (it just echoes the expression here)

(I don't have a strong opinion on this assert - I'm fine with keeping it but you could also drop it as we're enforcing someone else's clearly documented invariant)

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

943 ↗

(On Diff #213871)

for completeness, could you also add a unit test to CodeCompletionStringsTests.cpp?

[usaxena95]

Added tests in CodeCompletionStringsTests.cpp
Resolved comments.

[usaxena95]

Formatted new test.

clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp

 void appendEscapeSnippet(const llvm::StringRef Text, std::string *Out) {
   for (const auto Character : Text) {
     if (Character == '$' || Character == '}' || Character == '\\')
       Out->push_back('\\');
     Out->push_back(Character);
   }
 }
 
+void appendOptionalChunk(const CodeCompletionString &CCS, std::string *Out) {
+  for (const CodeCompletionString::Chunk &C : CCS) {
+    switch (C.Kind) {
+    case CodeCompletionString::CK_Optional:
+      assert(C.Optional &&
+             "Expected the optional code completion string to be non-null.");
+      appendOptionalChunk(*C.Optional, Out);
+      break;
+    default:
+      *Out += C.Text;
+      break;
+    }
+  }
+}
+
 bool looksLikeDocComment(llvm::StringRef CommentText) {
   // We don't report comments that only contain "special" chars.
   // This avoids reporting various delimiters, like:
   //   =================
   //   -----------------
   //   *****************
   return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
 }
case CodeCompletionString::CK_TypedText:
         }
       }
       break;
     case CodeCompletionString::CK_Text:
       *Signature += Chunk.Text;
       *Snippet += Chunk.Text;
       break;
     case CodeCompletionString::CK_Optional:
+      assert(Chunk.Optional);      
+      // No need to create placeholders for default arguments in Snippet.
+      appendOptionalChunk(*Chunk.Optional, Signature);
       break;
     case CodeCompletionString::CK_Placeholder:
       *Signature += Chunk.Text;
       ++SnippetArg;
       *Snippet +=
           "${" +
           std::to_string(SnippetArg == CursorSnippetArg ? 0 : SnippetArg) + ':';
       appendEscapeSnippet(Chunk.Text, Snippet);

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

#if 0
   // really provide useful results in excluded branches.
   par^
 #endif
     }
 )cpp");
 
   EXPECT_TRUE(Results.Completions.empty());
 }
+
+TEST(CompletionTest, DefaultArgs) {
+  clangd::CodeCompleteOptions Opts;
+  std::string Context = R"cpp(
+    int X(int A = 0);
+    int Y(int A, int B = 0);
+    int Z(int A, int B = 0, int C = 0, int D = 0);
+  )cpp";
+  EXPECT_THAT(completions(Context + "int y = X^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("X(int A = 0)")));
+  EXPECT_THAT(completions(Context + "int y = Y^", {}, Opts).Completions,
+              UnorderedElementsAre(AllOf(Labeled("Y(int A, int B = 0)"),
+                                         SnippetSuffix("(${1:int A})"))));
+  EXPECT_THAT(completions(Context + "int y = Z^", {}, Opts).Completions,
+              UnorderedElementsAre(
+                  AllOf(Labeled("Z(int A, int B = 0, int C = 0, int D = 0)"),
+                        SnippetSuffix("(${1:int A})"))));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
                          std::vector<Symbol> IndexSymbols = {}) {
   std::unique_ptr<SymbolIndex> Index;
   if (!IndexSymbols.empty())
     Index = memIndex(IndexSymbols);
 
   MockFSProvider FS;
   MockCompilationDatabase CDB;

clang-tools-extra/trunk/clangd/unittests/CodeCompletionStringsTests.cpp

TEST_F(CompletionStringTest, Function) {
 
   auto *CCS = Builder.TakeString();
   computeSignature(*CCS);
   EXPECT_EQ(Signature, "(p1, p2)");
   EXPECT_EQ(Snippet, "(${1:p1}, ${2:p2})");
   EXPECT_EQ(formatDocumentation(*CCS, "Foo's comment"), "Foo's comment");
 }
 
+TEST_F(CompletionStringTest, FunctionWithDefaultParams) {
+  // return_type foo(p1, p2 = 0, p3 = 0)
+  Builder.AddChunk(CodeCompletionString::CK_Comma);
+  Builder.AddTypedTextChunk("p3 = 0");
+  auto *DefaultParam2 = Builder.TakeString();
+
+  Builder.AddChunk(CodeCompletionString::CK_Comma);
+  Builder.AddTypedTextChunk("p2 = 0");
+  Builder.AddOptionalChunk(DefaultParam2);
+  auto *DefaultParam1 = Builder.TakeString();
+
+  Builder.AddResultTypeChunk("return_type");
+  Builder.AddTypedTextChunk("Foo");
+  Builder.AddChunk(CodeCompletionString::CK_LeftParen);
+  Builder.AddPlaceholderChunk("p1");
+  Builder.AddOptionalChunk(DefaultParam1);
+  Builder.AddChunk(CodeCompletionString::CK_RightParen);
+
+  auto *CCS = Builder.TakeString();
+  computeSignature(*CCS);
+  EXPECT_EQ(Signature, "(p1, p2 = 0, p3 = 0)");
+  EXPECT_EQ(Snippet, "(${1:p1})");
+}
+
 TEST_F(CompletionStringTest, EscapeSnippet) {
   Builder.AddTypedTextChunk("Foo");
   Builder.AddChunk(CodeCompletionString::CK_LeftParen);
   Builder.AddPlaceholderChunk("$p}1\\");
   Builder.AddChunk(CodeCompletionString::CK_RightParen);
 
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "($p}1\\)");