[clangd] Add option to enable/disable function argument snippets.

[kadircet]

Summary

Currently LSP clients cannot directly change EnableFunctionArgSnippets parameter.
This patch is to provide them with a way to enable/disable that functionality.

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

197

I wonder if we should make the IncludeFixIts option hidden?
It currently only works for our YCM integration, VSCode and other clients break.

198

This is a bit too generic name for the binary. Maybe -completion-fixits?

202

Maybe specify the value here explicitly?
The defaults for users of the clangd binary (what the option is for) is not necessarily the best default for the ClangdServer clients (what the default in the struct is for).
More importantly, it's useful to be more straightforward about the defaults we have for the options.

205

I wonder if we can infer this setting from the -completion-style' (=bundled` implies enable-function-arg-snippets == false)
@sammccall, WDYT?

[sammccall]

The description says "LSP clients", i.e. editors.
For editors (e.g. whether editor can handle snippets), LSP capability negotiation rather than flags is the right thing. I suspect this is true for including completion fixes.
For users (e.g. I like behavior X), flags are the right mechanism. But we don't need to expose every option, just the important ones.

clangd/tool/ClangdMain.cpp

197

why would a user want to turn this on or off?

198

clang calls changes attached to diagnostics "fix-it hints", clangd calls them "fixes".

This isn't either of those, maybe it should have a different name?
Or if clang is going to consider them as another type of fixit, we should probably call them fixes.
Maybe -completions-with-fixes, which hints that if the flag is off you're going to lose these completions entirely?

202

Not sure I agree here, this is consistent with the other options above. It's the other ClangdServer clients that are the weirdos, they should have to be explicit :-)

The defaults are clear when running with -help, which is how users will see them.

205

They're not inextricably linked, the main connection is "these options both need good signaturehelp support to be really useful".
But we might want to link them just to cut down on number of options.

I don't have a strong opinion, I don't use -completion-style=bundled. Can we find a few people who do and ask if they like this setting?

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

197

Ideally, we want to have it always on.
However, all editors interpret the results we return in different ways. This is a temporary option until we can define how text edits are handled by LSP.
We filed the bugs, will dig them up on Monday.

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

202

The defaults are clear when running with -help, which is how users will see them.

Sure, but I would still argue reading the code is simpler without extra indirection and defaults of the binary are not necessarily the defaults we use in the APIs.

But also feel free to keep the code as is, don't think it's terribly important.

205

I would (obviously) want these two options to be packaged together whenever I use =bundled.
But I also use VSCode, which has signatureHelp.

I think @ioeric wanted to try out the =bundled completion style. @ioeric, WDYT?

[kadircet]

• Change flag's name and rebase.

[ilya-biryukov]

LG from my side, @sammccall is not a big fan of options so please wait for his approval too

clangd/tool/ClangdMain.cpp

200

s/needs/need

206

Maybe mention code completion in the comment?
Something like:

When enabled, completion snippets insert full function signatures.
When disabled, completion snippets insert only parentheses for the call.

207

s/parantheses/parentheses

[sammccall]

clangd/tool/ClangdMain.cpp

197

Do we have any more details here? I'm still skeptical that exposing this to end users will help at all, this seems likely that it should be a capability if we do need it.

205

This seems reasonable to have as a preference, I'm also fine combining it with bundled.

If you keep it, naming nits:

• drop "enable" prefix (our other bool flags don't have this)
• snippets -> placeholders (snippets is both jargon and technically incorrect - we still emit snippets like foo({$0}).

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

197

No updates on the issue. Here it is:
https://github.com/Microsoft/language-server-protocol/issues/543

Not sure capability is the right thing there, the problem is that additionalTextEdits are underspecified and implemented differently in every client. What we need is a fix in the protocol and fixes in all the clients.

Sadly, this only works in YCM-based completer for now (of all we tested)

[kadircet]

• Update descriptions and change parameter name.

[sammccall]

I think I'm still where I was a few weeks ago - option to drop args makes sense, completions with fixes isn't something users should care about.

clangd/tool/ClangdMain.cpp

197

Sure, sounds like protocol fix is the long-term answer. I don't think adding user-facing options are better than nothing. If YCM does the right thing and we want to disable it for everyone not on YCM, we can add a textEditsAreAppliedInOrder capability to the YCM completer and treat that as an opt-in. It's not clear what the advantage of a user-facing flag over an editor-facing capability is for this purpose.

Mostly given LSP is unclear here it seems this feature isn't ready for prime-time.
Could we fix it on our side by coalescing multiple edits into a single one?

[ilya-biryukov]

+1 to adding an option to drop arguments from snippets and removing the option for the fixes.

clangd/tool/ClangdMain.cpp

197

I agree, the feature is not very useful if it breaks everywhere. Removing the option and exploring other ways to do it LG.

Could we fix it on our side by coalescing multiple edits into a single one?

We tried to combine additionalTextEdits into the main textEdit, that's what works in YCM.
However, it did not help in other editors, they misinterpret a main textedit (each in a different way) if it affects anything before the start of the completion identifier, which is exactly the case for the only fix we have at the time, that is . to ->.

[kadircet]

clangd/tool/ClangdMain.cpp

197

+1 to @ilya-biryukov . We might need to wait for a while until all editors supports additionalTextEdits in a sane way.

202

Keeping it as it is.

205

ping on this discussion to @ioeric , but I think linking seems like a good idea. Because it wouldn't be very useful if one already selects a specific overload from completion list.

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

205

I don't think YCM completes the snippets currently, so Eric won't notice the change for bundled vs normal completions.

Anyhow, keeping it as a separate option is a safe bet.

And some personal experience: have been using bundled completions for a few weeks now and will probably switch this one on immediately when it lands.

[ilya-biryukov]

Maybe commit only an option to enable function arg snippets for now (found myself wanting this option today :-))? The fixes would also be nice, but since they never work...

[kadircet]

• Delete include fixit option.
• Rebase

[kadircet]

clangd/tool/ClangdMain.cpp

197

Deleting this option and waiting until editors out there start to handle additionalTextEdits in a similar way.

[ilya-biryukov]

LGTM

clangd/tool/ClangdMain.cpp

static llvm::cl::opt<CompileArgsFrom> CompileArgsFrom(
     llvm::cl::values(clEnumValN(LSPCompileArgs, "lsp",
                                 "All compile commands come from LSP and "
                                 "'compile_commands.json' files are ignored"),
                      clEnumValN(FilesystemCompileArgs, "filesystem",
                                 "All compile commands come from the "
                                 "'compile_commands.json' files")),
     llvm::cl::init(FilesystemCompileArgs), llvm::cl::Hidden);
 
+static llvm::cl::opt<bool> IncludeFixIts(
+    "include-fixits",
+    llvm::cl::desc(
+        "Enables suggestion of completion items that needs additional changes. "
+        "Like changing an arrow(->) member access to dot(.) member access."),
+    llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+
+static llvm::cl::opt<bool> EnableFunctionArgSnippets(
+    "enable-function-arg-snippets",
+    llvm::cl::desc("Gives snippets for function arguments, when disabled only "
+                   "gives parantheses for the call."),
+    llvm::cl::init(clangd::CodeCompleteOptions().EnableFunctionArgSnippets));
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
     OS << clang::getClangToolFullVersion("clangd") << "\n";
   });
   llvm::cl::ParseCommandLineOptions(
       argc, argv,
       "clangd is a language server that provides IDE-like features to editors. "
int main(int argc, char *argv[]) {
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   if (!HeaderInsertionDecorators) {
     CCOpts.IncludeIndicator.Insert.clear();
     CCOpts.IncludeIndicator.NoInsert.clear();
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
+  CCOpts.IncludeFixIts = IncludeFixIts;
+  CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
       Out, CCOpts, CompileCommandsDirPath,
       /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
   return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
 }