[clangd] Fix function-arg-placeholder suppression with macros.

[sammccall]

Summary

While here, unhide function-arg-placeholders flag. It's reasonable to want and
maybe we should consider making it default.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

468–469

while here, i suppose we should perform drop arguments and parentheses magic for macros as well?

511

again while here, maybe introduce constructors into this condition. (Even better, what about a bool isFunctionLikeCompletion(const CompletionItem&) that we can use both here and above?)

[sammccall]

Address review comments.
While here, condense some long comments that repeated the code and seemed confusing.

clang-tools-extra/clangd/CodeComplete.cpp

511

Done, though I didn't want to extract the variable too far away as there are some wrinkles (macros may not be function-like, template args...) that we can get away without resolving here.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

511

thanks! yes that makes sense.

clang-tools-extra/clangd/CodeComplete.cpp

private:
   std::string summarizeSnippet() const {
     if (IsUsingDeclaration)
       return "";
     auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
     if (!Snippet)
       // All bundles are function calls.
       // FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
       // we need to complete 'forward<$1>($0)'.
       return "($0)";
-    // Suppress function argument snippets cursor is followed by left
-    // parenthesis (and potentially arguments) or if there are potentially
-    // template arguments. There are cases where it would be wrong (e.g. next
-    // '<' token is a comparison rather than template argument list start) but
-    // it is less common and suppressing snippet provides better UX.
-    if (Completion.Kind == CompletionItemKind::Function ||
+
+    bool MayHaveArgList = Completion.Kind == CompletionItemKind::Function ||
                           Completion.Kind == CompletionItemKind::Method ||
-        Completion.Kind == CompletionItemKind::Constructor) {
-      // If there is a potential template argument list, drop snippet and just
-      // complete symbol name. Ideally, this could generate an edit that would
-      // paste function arguments after template argument list but it would be
-      // complicated. Example:
-      //
-      // fu^<int> -> function<int>
+                          Completion.Kind == CompletionItemKind::Constructor ||
+                          Completion.Kind == CompletionItemKind::Text /*Macro*/;
+    // If likely arg list already exists, don't add new parens & placeholders.
+    //   Snippet: function(int x, int y)
+    //   func^(1,2) -> function(1, 2)
+    //             NOT function(int x, int y)(1, 2)
+    if (MayHaveArgList) {
+      // Check for a template argument list in the code.
+      //   Snippet: function<class T>(int x)
+      //   fu^<int>(1) -> function<int>(1)
       if (NextTokenKind == tok::less && Snippet->front() == '<')
         return "";
-      // Potentially followed by argument list.
+      // Potentially followed by regular argument list.
       if (NextTokenKind == tok::l_paren) {
-        // If snippet contains template arguments we will emit them and drop
-        // function arguments. Example:
-        //
-        // fu^(42) -> function<int>(42);
+        //   Snippet: function<class T>(int x)
+        //   fu^(1,2) -> function<class T>(1, 2)
         if (Snippet->front() == '<') {
-          // Find matching '>'. Snippet->find('>') will not work in cases like
-          // template <typename T=std::vector<int>>. Hence, iterate through
-          // the snippet until the angle bracket balance reaches zero.
+          // Find matching '>', handling nested brackets.
           int Balance = 0;
           size_t I = 0;
           do {
             if (Snippet->at(I) == '>')
               --Balance;
             else if (Snippet->at(I) == '<')
               ++Balance;
             ++I;
           } while (Balance > 0);
           return Snippet->substr(0, I);
         }
         return "";
       }
     }
     if (EnableFunctionArgSnippets)
       return *Snippet;
 
     // Replace argument snippets with a simplified pattern.
     if (Snippet->empty())
       return "";
-    if (Completion.Kind == CompletionItemKind::Function ||
-        Completion.Kind == CompletionItemKind::Method) {
+    if (MayHaveArgList) {
       // Functions snippets can be of 2 types:
       // - containing only function arguments, e.g.
       //   foo(${1:int p1}, ${2:int p2});
       //   We transform this pattern to '($0)' or '()'.
       // - template arguments and function arguments, e.g.
       //   foo<${1:class}>(${2:int p1}).
       //   We transform this pattern to '<$1>()$0' or '<$0>()'.
 
       bool EmptyArgs = llvm::StringRef(*Snippet).endswith("()");

clang-tools-extra/clangd/tool/ClangdMain.cpp

 
 opt<bool> EnableFunctionArgSnippets{
     "function-arg-placeholders",
     cat(Features),
     desc("When disabled, completions contain only parentheses for "
          "function calls. When enabled, completions also contain "
          "placeholders for method parameters"),
     init(CodeCompleteOptions().EnableFunctionArgSnippets),
-    Hidden,
 };
 
 opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
     "header-insertion",
     cat(Features),
     desc("Add #include directives when accepting code completions"),
     init(CodeCompleteOptions().InsertIncludes),
     values(

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

TEST(CompletionTest, OverloadBundling) {
 
   std::string Context = R"cpp(
     struct X {
       // Overload with int
       int a(int) __attribute__((deprecated("", "")));
       // Overload with bool
       int a(bool);
       int b(float);
+
+      X(int);
+      X(float);
     };
     int GFuncC(int);
     int GFuncD(int);
   )cpp";
 
   // Member completions are bundled.
   EXPECT_THAT(completions(Context + "int y = X().^", {}, Opts).Completions,
               UnorderedElementsAre(Labeled("a(…)"), Labeled("b(float)")));
 
+  // Constructor completions are bundled.
+  EXPECT_THAT(completions(Context + "X z = X^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("X"), Labeled("X(…)")));
+
   // Non-member completions are bundled, including index+sema.
   Symbol NoArgsGFunc = func("GFuncC");
   EXPECT_THAT(
       completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
       UnorderedElementsAre(Labeled("GFuncC(…)"), Labeled("GFuncD(int)")));
 
   // Differences in header-to-insert suppress bundling.
   std::string DeclFile = URI::create(testPath("foo")).toString();
auto Results = completions(
       using foo_alias = T**;
       void f() { foo_^ })cpp",
         {}, Opts);
     EXPECT_THAT(
         Results.Completions,
         UnorderedElementsAre(AllOf(Named("foo_class"), SnippetSuffix("<$0>")),
                              AllOf(Named("foo_alias"), SnippetSuffix("<$0>"))));
   }
+  {
+    auto Results = completions(
+        R"cpp(
+      #define FOO(x, y) x##f
+      FO^ )cpp",
+        {}, Opts);
+    EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+                                         Named("FOO"), SnippetSuffix("($0)"))));
+  }
 }
 
 TEST(CompletionTest, SuggestOverrides) {
   constexpr const char *const Text(R"cpp(
   class A {
    public:
     virtual void vfunc(bool param);
     virtual void vfunc(bool param, int p);
for (const char *Test : No) {
     EXPECT_FALSE(allowImplicitCompletion(A.code(), A.point())) << Test;
   }
 }
 
 TEST(CompletionTest, FunctionArgsExist) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
   std::string Context = R"cpp(
+    #define MACRO(x)
     int foo(int A);
     int bar();
     struct Object {
       Object(int B) {}
     };
     template <typename T>
     struct Container {
       Container(int Size) {}
EXPECT_THAT(
       Contains(AllOf(Labeled("Container<typename T>(int Size)"),
                      SnippetSuffix("<${1:typename T}>"),
                      Kind(CompletionItemKind::Constructor))));
   EXPECT_THAT(
       completions(Context + "Container c = Cont^<int>()", {}, Opts).Completions,
       Contains(AllOf(Labeled("Container<typename T>(int Size)"),
                      SnippetSuffix(""),
                      Kind(CompletionItemKind::Constructor))));
+  EXPECT_THAT(completions(Context + "MAC^(2)", {}, Opts).Completions,
+              Contains(AllOf(Labeled("MACRO(x)"), SnippetSuffix(""),
+                             Kind(CompletionItemKind::Text))));
 }
 
 TEST(CompletionTest, NoCrashDueToMacroOrdering) {
   EXPECT_THAT(completions(R"cpp(
     #define ECHO(X) X
     #define ECHO2(X) ECHO(X)
     int finish_preamble = EC^HO(2);)cpp")
                   .Completions,