[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

450

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.

456

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

459

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

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

2982

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

477

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

483

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).

499

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

500

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 ||
+    auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
+    // 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 (Snippet && (Completion.Kind == CompletionItemKind::Function ||
                     Completion.Kind == CompletionItemKind::Method ||
-         Completion.Kind == CompletionItemKind::Constructor) &&
-        NextTokenKind == tok::l_paren)
+                    Completion.Kind == CompletionItemKind::Constructor)) {
+      // Potentially followed by argument list.
+      if (NextTokenKind == tok::l_paren) {
+        // If snippet contains template arguments we will emit them and drop
+        // function arguments.
+        if (Snippet->front() == '<')
+          return Snippet->substr(0, Snippet->find('('));
         return "";
-    auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
+      }
+      // If there is a potential template argument list, drop snippet. Ideally,
+      // this could generate an edit that would paste function arguments after
+      // template argument list but it would be complicated.
+      if (NextTokenKind == tok::less && Snippet->front() == '<')
+        return "";
+    }
     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)";
     if (EnableFunctionArgSnippets)
       return *Snippet;
 
     // Replace argument snippets with a simplified pattern.
     if (Snippet->empty())
       return "";
     if (Completion.Kind == CompletionItemKind::Function ||
         Completion.Kind == CompletionItemKind::Method) {
       // 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("()");
       if (Snippet->front() == '<')
         return EmptyArgs ? "<$1>()$0" : "<$1>($0)";
       if (Snippet->front() == '(')
         return EmptyArgs ? "()" : "($0)";
       return *Snippet; // Not an arg snippet?
     }
     // 'CompletionItemKind::Interface' matches template type aliases.
     if (Completion.Kind == CompletionItemKind::Interface ||
         Completion.Kind == CompletionItemKind::Class) {
       if (Snippet->front() != '<')
         return *Snippet; // Not an arg snippet?
 
       // Classes and template using aliases can only have template arguments,
       // e.g. Foo<${1:class}>.
       if (llvm::StringRef(*Snippet).endswith("<>"))
         return "<>"; // can happen with defaulted template arguments.
       return "<$0>";
     }
     return *Snippet;
   }
 
   std::string summarizeSignature() const {
     if (auto *Signature = onlyValue<&BundledEntry::Signature>())
       return *Signature;
     // All bundles are function calls.
     return "(…)";
   }
 

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";
+  // FIXME(kirillbobyrev): Also strip prefix of identifier token before the
+  // cursor from completion item ("fo" in this case).
   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,
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