[clangd] Add parantheses while auto-completing functions.

[kadircet]

Summary

Currently we only add parantheses to the functions if snippets are
enabled, which also inserts snippets for parameters into parantheses. Adding a
new option to put only parantheses. Also it moves the cursor within parantheses
or at the end of them by looking at whether completion item has any parameters
or not. Still requires snippets support on the client side.

[ioeric]

clangd/CodeComplete.h

86

IIRC, the goal of this patch is to allow disabling snippet templates for function parameters while still allow appending () or ($0) to function candidates. If that's still the case, I think what we want is probably an option like DisableSnippetTemplateForFunctionArgs?

[kadircet]

• Change option name.

clangd/CodeComplete.h

86

Yeah, that makes totally more sense. Thanks!

[ilya-biryukov]

clangd/CodeComplete.h

86

• Maybe use positive option names to avoid double negation?
• Maybe also use a shorter name? E.g. removing 'template' and 'disable' from the name gives a name that does not look any worse: FunctionArgSnippets.

[ioeric]

clangd/CodeComplete.cpp

1377

There seems to be no guarantee on whether the snippet is function arg template when SnippetSuffix.size() > 2. A heuristic we could use is probably checking that the template starts with ( and ends with )? Alternatively, we could try to avoid generating the proper snippet according to the flag when we still have the function declaration around. Not sure if this is feasible for index results though.

clangd/CodeComplete.h

86

+1 to a better name. I didn't really give much thought about the name and just wanted to get the idea across :)

[kadircet]

• Change option name.

[kadircet]

clangd/CodeComplete.cpp

1377

Actually I was first trying to directly look at function argument count as you mentioned, but unfortunately Symbols coming from index don't have any declarations. Therefore it turns back to looking at Signature in this case, which is same with this one. Apart from that, IIUC, SnippetSuffix contains anything only when completion item is a function otherwise it is empty. So IMO this check should be OK.

[ioeric]

clangd/CodeComplete.cpp

1377

Apart from that, IIUC, SnippetSuffix contains anything only when completion item is a function otherwise it is empty. So IMO this check should be OK.

This seems to be true for the current use cases, but I suspect the assumption can change when we add more snippet-based completion results in the future e.g. a snippet for if-statement if (${0}) {${1}}.

[kadircet]

• Only append to snippets of type function or method.

[ioeric]

clangd/CodeComplete.cpp

1379

nit: add parentheses around (... == ...)

1380

nit: no braces around one liner.

1381

I think we are missing one case here: !EnableFunctionArgSnippets && (<not function or method>)?

I think we could do something like:

if (!EnableFunctionArgSnippets && (<function or metrhod>))
  // Truncate parameter template
else
  // append snippet like before

[kadircet]

• Resolve discussions.

[kadircet]

clangd/CodeComplete.cpp

1381

Thanks!

[ioeric]

lgtm

clangd/CodeComplete.h

struct CodeCompleteOptions {
   /// results.
   /// FIXME(ioeric): we might want a better way to pass the index around inside
   /// clangd.
   const SymbolIndex *Index = nullptr;
 
   /// Include completions that require small corrections, e.g. change '.' to
   /// '->' on member access etc.
   bool IncludeFixIts = false;
+
+  /// Whether to generate snippets for function arguments on code-completion.
+  /// Needs snippets to be enabled as well.
+  bool EnableFunctionArgSnippets = true;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ API.
 // We don't use the LSP structures here (unlike most features) as we want
 // to expose more data to allow for more precise testing and evaluation.
 struct CodeCompletion {
   // The unqualified name of the symbol or other completion item.
   std::string Name;

clangd/CodeComplete.cpp

CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const {
   for (const auto &FixIt : FixIts) {
     if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {
       LSP.textEdit->newText = FixIt.newText + LSP.textEdit->newText;
       LSP.textEdit->range.start = FixIt.range.start;
     } else {
       LSP.additionalTextEdits.push_back(FixIt);
     }
   }
-  if (Opts.EnableSnippets)
+  if (Opts.EnableSnippets && !SnippetSuffix.empty()) {
+    if (Opts.EnableFunctionArgSnippets)
       LSP.textEdit->newText += SnippetSuffix;
+    else if ((Kind == CompletionItemKind::Function ||
+              Kind == CompletionItemKind::Method) &&
+             SnippetSuffix.front() == '(' && SnippetSuffix.back() == ')') {
+      // Check whether function has any parameters or not.
+      LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+    }
+  }
+
   // FIXME(kadircet): Do not even fill insertText after making sure textEdit is
   // compatible with most of the editors.
   LSP.insertText = LSP.textEdit->newText;
   LSP.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
                                              : InsertTextFormat::PlainText;
   if (HeaderInsertion)
     LSP.additionalTextEdits.push_back(*HeaderInsertion);
   return LSP;

unittests/clangd/CodeCompleteTests.cpp

EXPECT_THAT(signatures(R"cpp(
     int main() {
       X<int>().foo<double>(^)
     }
   )cpp")
                   .signatures,
               ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+  CodeCompleteOptions Opts;
+  Opts.EnableFunctionArgSnippets = true;
+  {
+    CodeCompletion C;
+    C.RequiredQualifier = "Foo::";
+    C.Name = "x";
+    C.SnippetSuffix = "()";
+
+    auto R = C.render(Opts);
+    EXPECT_EQ(R.textEdit->newText, "Foo::x");
+    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+  }
+
+  Opts.EnableSnippets = true;
+  Opts.EnableFunctionArgSnippets = false;
+  {
+    CodeCompletion C;
+    C.RequiredQualifier = "Foo::";
+    C.Name = "x";
+    C.SnippetSuffix = "";
+
+    auto R = C.render(Opts);
+    EXPECT_EQ(R.textEdit->newText, "Foo::x");
+    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+    CodeCompletion C;
+    C.RequiredQualifier = "Foo::";
+    C.Name = "x";
+    C.SnippetSuffix = "()";
+    C.Kind = CompletionItemKind::Method;
+
+    auto R = C.render(Opts);
+    EXPECT_EQ(R.textEdit->newText, "Foo::x()");
+    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+    CodeCompletion C;
+    C.RequiredQualifier = "Foo::";
+    C.Name = "x";
+    C.SnippetSuffix = "(${0:bool})";
+    C.Kind = CompletionItemKind::Function;
+
+    auto R = C.render(Opts);
+    EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
+    EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang