[clangd] Drop template argument lists from completions followed by <

[kbobyrev]

Summary

Now, given template <typename T> foo() {} when user types fo^<int>() the
completion snippet will not contain <int>().

Also, when the next token is opening parenthesis (() and completion snippet
contains template argument list, it is still emitted.

This patch complements D81380.

Related issue: https://github.com/clangd/clangd/issues/387

[kbobyrev]

@sammccall Ping, this is a real patch :)

[kadircet]

(sorry for forgetting about this)

clang-tools-extra/clangd/CodeComplete.cpp

456

nit: move this case above and get rid of if(Snippet part of the check?

469

what if we have ( in the template parameter list ? i think we need to literally find the matching > and include all the text in between instead.

475

nit: again might be worth putting the easy-to-reason case above to reduce mental load when reading.

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

3090

umm, where did this fixme come from exactly? don't we emit these as replacements for the fo part of the text anyway ?

[kbobyrev]

Address review comments.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/CodeComplete.cpp

474

RHS should be function<int>(42) right?

480

nit:

int Balance = 0;
size_t I = 0;
do {
  if(Snippet[I] == '>') --Balance;
  else if(Snippet[I] == '<') ++Balance;
  ++I;
} while(Balance > 0);
return Snippet->substr(0, I);

This should handle both the case snippet starts with < and not. reducing the nesting a little and making the flow more uniform (i.e. getting rid of the second return statement).

Up to you though, if you keep the existing version please move definition of Balance into for-init statement, use size_t instead of unsigned and array subscripts instead of at(Index).

496

i think it is better to omit (42) in this example.

497

You've marked the previous nit as done, but this case still seems to be coming after the complicated case, just in case it slipped :D

[kbobyrev]

Address review comments

clang-tools-extra/clangd/CodeComplete.cpp

std::string summarizeReturnType() const {
     if (auto *RT = onlyValue<&BundledEntry::ReturnType>())
       return *RT;
     return "";
   }
 
   std::string summarizeSnippet() const {
     if (IsUsingDeclaration)
       return "";
-    // Suppress function argument snippets if args are already present.
-    if ((Completion.Kind == CompletionItemKind::Function ||
-         Completion.Kind == CompletionItemKind::Method ||
-         Completion.Kind == CompletionItemKind::Constructor) &&
-        NextTokenKind == tok::l_paren)
-      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 ||
+        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>
+      if (NextTokenKind == tok::less && Snippet->front() == '<')
+        return "";
+      // Potentially followed by 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);
+        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.
+          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) {

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

std::string Context = R"cpp(
     struct Object {
       Object(int B) {}
     };
     template <typename T>
     struct Container {
       Container(int Size) {}
     };
   )cpp";
   EXPECT_THAT(completions(Context + "int y = fo^", {}, Opts).Completions,
               UnorderedElementsAre(
                   AllOf(Labeled("foo(int A)"), SnippetSuffix("(${1:int A})"))));
   EXPECT_THAT(
       completions(Context + "int y = fo^(42)", {}, Opts).Completions,
       UnorderedElementsAre(AllOf(Labeled("foo(int A)"), SnippetSuffix(""))));
   // FIXME(kirillbobyrev): No snippet should be produced here.
   EXPECT_THAT(completions(Context + "int y = fo^o(42)", {}, Opts).Completions,
               UnorderedElementsAre(
TEST(CompletionTest, FunctionArgsExist) {
   EXPECT_THAT(completions(Context + "Object o = Obj^()", {}, Opts).Completions,
               Contains(AllOf(Labeled("Object(int B)"), SnippetSuffix(""),
                              Kind(CompletionItemKind::Constructor))));
   EXPECT_THAT(
       completions(Context + "Container c = Cont^", {}, Opts).Completions,
       Contains(AllOf(Labeled("Container<typename T>(int Size)"),
                      SnippetSuffix("<${1:typename T}>(${2:int Size})"),
                      Kind(CompletionItemKind::Constructor))));
-  // FIXME(kirillbobyrev): It would be nice to still produce the template
-  // snippet part: in this case it should be "<${1:typename T}>".
   EXPECT_THAT(
       completions(Context + "Container c = Cont^()", {}, Opts).Completions,
       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))));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang