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

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

Fixes clangd/clangd#922

Differential Revision: https://reviews.llvm.org/D113765

Author: sam-mccall

clang-tools-extra/clangd/CodeComplete.cpp

@@ -466,32 +466,27 @@ struct CodeCompletionBuilder {
       // 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 ||
-        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>
+
+    bool MayHaveArgList = Completion.Kind == CompletionItemKind::Function ||
+                          Completion.Kind == CompletionItemKind::Method ||
+                          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 {
@@ -512,8 +507,7 @@ struct CodeCompletionBuilder {
     // 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});

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

@@ -233,7 +233,6 @@ opt<bool> EnableFunctionArgSnippets{
          "function calls. When enabled, completions also contain "
          "placeholders for method parameters"),
     init(CodeCompleteOptions().EnableFunctionArgSnippets),
-    Hidden,
 };
 
 opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{

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

@@ -1662,6 +1662,9 @@ TEST(CompletionTest, OverloadBundling) {
       // Overload with bool
       int a(bool);
       int b(float);
+
+      X(int);
+      X(float);
     };
     int GFuncC(int);
     int GFuncD(int);
@@ -1671,6 +1674,10 @@ TEST(CompletionTest, OverloadBundling) {
   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(
@@ -2323,6 +2330,15 @@ TEST(CompletionTest, CompletionFunctionArgsDisabled) {
         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) {
@@ -3170,6 +3186,7 @@ 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 {
@@ -3217,6 +3234,9 @@ TEST(CompletionTest, FunctionArgsExist) {
       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) {