This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kadircet on Aug 24 2018, 6:35 AM.

Details

Summary

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

Event Timeline

kadircet created this revision.Aug 24 2018, 6:35 AM
ilya-biryukov added inline comments.
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?

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 added inline comments.Aug 24 2018, 10:28 AM
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 added inline comments.Aug 27 2018, 5:06 AM
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 updated this revision to Diff 164830.Sep 11 2018, 2:59 AM
kadircet marked 5 inline comments as done.
  • Change flag's name and rebase.

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 added inline comments.Sep 11 2018, 3:26 AM
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 added inline comments.Sep 11 2018, 3:48 AM
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 updated this revision to Diff 164847.Sep 11 2018, 4:54 AM
kadircet marked 4 inline comments as done.
  • Update descriptions and change parameter name.

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?

+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 added inline comments.Sep 11 2018, 6:53 AM
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 added inline comments.Sep 11 2018, 7:34 AM
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.

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 updated this revision to Diff 166076.Sep 19 2018, 1:26 AM
kadircet marked 14 inline comments as done.
  • Delete include fixit option.
  • Rebase
kadircet added inline comments.Sep 19 2018, 1:26 AM
clangd/tool/ClangdMain.cpp
197

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

kadircet retitled this revision from [clangd] Add options to enable/disable fixits and function argument snippets. to [clangd] Add option to enable/disable function argument snippets..Sep 19 2018, 1:28 AM
kadircet edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Sep 19 2018, 2:07 AM
This revision was automatically updated to reflect the committed changes.