[clangd] Remove `allCommitCharacters`

[sammccall]

Summary

This was added in 2a095ff6f5028b76, however it never worked with VSCode
due to bugs in vscode-languageclient
(https://github.com/microsoft/vscode-languageserver-node/issues/673).
Now that it does work, we can tell the interactions with text editing, with
snippets, and vscode's select-first-completion behavior are bad.

The spec is vague and clients could do something reasonable with the
current values. However they could clearly do something unreasonable
too, and over time behavior+spec tends to converge on VSCode's behavior.

This addresses https://github.com/clangd/vscode-clangd/pull/358
See also https://github.com/clangd/vscode-clangd/pull/358 which hotfixes
this on the client-side (as we can't apply this change retroactively to
clangd 12-14).

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.cpp

537

it would be better to have a comment explaining why we don't set allCommitCharacters.

[kadircet]

sorry for being late to the party, but reading the spec it sounds like vscode behaviour is actually the correct one (despite not being the optimal UX, at least to our understanding).

so at least for the intermediate state (and possibly ever, if LSP folks don't change the wording or add a capability) we might consider still advertising operators as commit characters (e.g. ., -, , ...) but i think there's an even bigger problem in vscode at the moment around combined effect of this and snippets, maybe we should disable commit characters on items with snippets and have the rest use the "safe" set of defaults. WDYT?

[sammccall]

In D129579#3647834, @kadircet wrote:

sorry for being late to the party, but reading the spec it sounds like vscode behaviour is actually the correct one (despite not being the optimal UX, at least to our understanding).

Agree it's the specified behavior, does the comment suggest otherwise? (I did remember the spec being more ambiguous than that, but it's clear).

so at least for the intermediate state (and possibly ever, if LSP folks don't change the wording or add a capability) we might consider still advertising operators as commit characters (e.g. ., -, , ...)

In VSCode this also yields (IMO) poor UX, because VSCode always considers the top completion to be selected and ready to be committed (vs considering nothing initially selected).
See reports to us in https://github.com/clangd/vscode-clangd/issues/356 as well as 357, and https://github.com/microsoft/vscode/issues/139825 and the various dupes it links to: basically this seems like bad behavior that the VSCode devs have dug their heels in on and makes commitchars unhelpful.

This problem is VSCode-specific so in principle we could allow . for other editors, but that's hard to detect (and IMO complexity vs benefit here is too high).

but i think there's an even bigger problem in vscode at the moment around combined effect of this and snippets

I think as you described above, *that* interaction (where ( inserts a second bracket inside the snippet's brackets) isn't vscode-specific but actually specified.

maybe we should disable commit characters on items with snippets and have the rest use the "safe" set of defaults. WDYT?

Having behavior dynamically depend on whether a result has to be a snippet is useless because the user('s muscle memory) doesn't know whether it's a snippet.

We could have different value for editors that don't support snippets at all, but this introduces extra configurations to reason about - supporting non-overlapping sets of features is hard to explain!

clang-tools-extra/clangd/ClangdLSPServer.cpp

llvm::json::Object ServerCaps{
       {"documentRangeFormattingProvider", true},
       {"documentOnTypeFormattingProvider",
        llvm::json::Object{
            {"firstTriggerCharacter", "\n"},
            {"moreTriggerCharacter", {}},
        }},
       {"completionProvider",
        llvm::json::Object{
-           {"allCommitCharacters",
-            {" ", "\t", "(", ")", "[", "]", "{",  "}", "<",
-             ">", ":",  ";", ",", "+", "-", "/",  "*", "%",
-             "^", "&",  "#", "?", ".", "=", "\"", "'", "|"}},
+           // We don't set `(` etc as allCommitCharacters as they interact
+           // poorly with snippet results.
+           // See https://github.com/clangd/vscode-clangd/issues/357
+           // Hopefully we can use them one day without this side-effect:
+           //     https://github.com/microsoft/vscode/issues/42544
            {"resolveProvider", false},
            // We do extra checks, e.g. that > is part of ->.
            {"triggerCharacters", {".", "<", ">", ":", "\"", "/", "*"}},
        }},
       {"semanticTokensProvider",
        llvm::json::Object{
            {"full", llvm::json::Object{{"delta", true}}},
            {"range", false},

clang-tools-extra/clangd/test/initialize-params.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 # Test initialize request parameters with rootUri
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
 #      CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "capabilities": {
 # CHECK-NEXT:      "astProvider": true,
 # CHECK-NEXT:      "callHierarchyProvider": true,
 # CHECK-NEXT:      "clangdInlayHintsProvider": true,
 # CHECK-NEXT:      "codeActionProvider": true,
 # CHECK-NEXT:      "compilationDatabase": {
 # CHECK-NEXT:        "automaticReload": true
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "completionProvider": {
-# CHECK-NEXT:        "allCommitCharacters": [
-# CHECK-NEXT:          " ",
-# CHECK-NEXT:          "\t",
-# CHECK-NEXT:          "(",
-# CHECK-NEXT:          ")",
-# CHECK-NEXT:          "[",
-# CHECK-NEXT:          "]",
-# CHECK-NEXT:          "{",
-# CHECK-NEXT:          "}",
-# CHECK-NEXT:          "<",
-# CHECK-NEXT:          ">",
-# CHECK-NEXT:          ":",
-# CHECK-NEXT:          ";",
-# CHECK-NEXT:          ",",
-# CHECK-NEXT:          "+",
-# CHECK-NEXT:          "-",
-# CHECK-NEXT:          "/",
-# CHECK-NEXT:          "*",
-# CHECK-NEXT:          "%",
-# CHECK-NEXT:          "^",
-# CHECK-NEXT:          "&",
-# CHECK-NEXT:          "#",
-# CHECK-NEXT:          "?",
-# CHECK-NEXT:          ".",
-# CHECK-NEXT:          "=",
-# CHECK-NEXT:          "\"",
-# CHECK-NEXT:          "'",
-# CHECK-NEXT:          "|"
-# CHECK-NEXT:        ],
 # CHECK-NEXT:        "resolveProvider": false,
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          "<",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":",
 # CHECK-NEXT:          "\"",
 # CHECK-NEXT:          "/",