[clangd] Do not try to use $0 as a placeholder in completion snippets

[nridge]

Summary

$0 can only be used as a tab stop, not as a placeholder (e.g.
${0:expression} is not valid)

Fixes https://github.com/clangd/clangd/issues/1190

[kadircet]

clang-tools-extra/clangd/CodeCompletionStrings.cpp

197

as you've mentioned in the vscode issue, i think it'd be better to append a $0 when we reach CursorSnippetArg. it's annoying that we require an extra tab, but IMO it's still better than just requiring user to move around a bunch.

also i think it makes sense to wait for a little while until we hear back from vscode folks about a workaround. Despite them saying $0 is not a placeholder, LSP doesn't clearly state that. hence clangd might not be the only language server that's affected from this.

[ilya-biryukov]

My 2 cents here. We should probably try hard to keep the cursor inside braces/parentheses for those patterns.
Having

namespace foo {
  decls
}^ <- cursor here

hurts UX. It would be nice to get the old behavior back somehow.
I still have hopes for the VSCode discussion, maybe they can provide a workaround/willing to change their behavior.
@kadircet also suggested trying something like ${2:decls}$0. Could you check if that works in VSCode?

[ilya-biryukov]

Another alternative that I think should give the best UX is to replace ${0:named} with $0.
The items will look different, but will behave identically to the old behavior before VSCode change, i.e. won't "eat" an extra tab press at the end of completion session.
I feel that's the trade-off we should pick.

What do others think?

[sammccall]

In D128621#3645123, @ilya-biryukov wrote:

Another alternative that I think should give the best UX is to replace ${0:named} with $0.
The items will look different, but will behave identically to the old behavior before VSCode change, i.e. won't "eat" an extra tab press at the end of completion session.
I feel that's the trade-off we should pick.

What do others think?

Put some thoughts in https://github.com/clangd/clangd/issues/1190
TL;DR:

• I think fixing this only in the server for future releases (and not touching vscode-clangd) is OK
• Of the ideas we've heard, I like ${0:named} => $0 best, but can certainly live with the one in this patch
• (I think we could refine behavior further, but let's not block on it)

[nridge]

Revise patch to use $0 (no placeholder text) rather than ${n:placeholder}

[nridge]

In D128621#3645271, @sammccall wrote:

• Of the ideas we've heard, I like ${0:named} => $0 best, but can certainly live with the one in this patch

I like this approach as well. Revised the patch to do this.

[kadircet]

thanks, LG, just some extra precautions.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

194–195

i think we shouldn't even have braces for $0 (just to be safe). so what about:

if (SnippetArg == CursorSnippetArg) {
  *Snippet = "$0";
} else {
  *Snippet = "${" + std::to_string(SnippetArg) + ":";
  appendEscapeSnippet(Chunk.Text, Snippet);
  *Snippet += "}";
}

[sammccall]

Thank you!

clang-tools-extra/clangd/CodeCompletionStrings.cpp

194–195

nit: at this point it might be clearer to unfold the two cases (not longer or shorter)
up to you which is more readable

if (SnippetArg == CursorSnippetArg) {
  // we'd like to make...
  *Snippet += "${0}";
} else {
  *Snippet += "${" + to_string(SnippetArg) + ":";
  appendEscapeSnippet(...);
  *Snippet += "}";
}

[nridge]

Address final review comments

clang-tools-extra/clangd/CodeCompletionStrings.cpp

case CodeCompletionString::CK_Text:
       break;
     case CodeCompletionString::CK_Optional:
       assert(Chunk.Optional);      
       // No need to create placeholders for default arguments in Snippet.
       appendOptionalChunk(*Chunk.Optional, Signature);
       break;
     case CodeCompletionString::CK_Placeholder:
       *Signature += Chunk.Text;
       ++SnippetArg;
-      *Snippet +=
-          "${" +
-          std::to_string(SnippetArg == CursorSnippetArg ? 0 : SnippetArg) + ':';
+      if (SnippetArg == CursorSnippetArg) {
+        // We'd like to make $0 a placeholder too, but vscode does not support
+        // this (https://github.com/microsoft/vscode/issues/152837).
+        *Snippet += "$0";
+      } else {
+        *Snippet += "${" + std::to_string(SnippetArg) + ':';
         appendEscapeSnippet(Chunk.Text, Snippet);
         *Snippet += '}';
+      }
       break;
     case CodeCompletionString::CK_Informative:
       HadInformativeChunks = true;
       // For example, the word "const" for a const method, or the name of
       // the base class for methods that are part of the base class.
       *Signature += Chunk.Text;
       // Don't put the informative chunks in the snippet.
       break;

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

auto Results = completions(
     void while_foo(int a, int b);
     void test() {
       whil^
     })cpp",
       /*IndexSymbols=*/{}, Options);
 
   // Last placeholder in code patterns should be $0 to put the cursor there.
   EXPECT_THAT(Results.Completions,
-              Contains(AllOf(
-                  named("while"),
-                  snippetSuffix(" (${1:condition}) {\n${0:statements}\n}"))));
+              Contains(AllOf(named("while"),
+                             snippetSuffix(" (${1:condition}) {\n$0\n}"))));
   // However, snippets for functions must *not* end with $0.
   EXPECT_THAT(Results.Completions,
               Contains(AllOf(named("while_foo"),
                              snippetSuffix("(${1:int a}, ${2:int b})"))));
 }
 
 TEST(CompletionTest, WorksWithNullType) {
   auto R = completions(R"cpp(

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

auto MakeCCS = [this]() -> const CodeCompletionString & {
     Builder.AddChunk(CodeCompletionString::CK_SemiColon);
     return *Builder.TakeString();
   };
   computeSignature(MakeCCS(), /*CompletingPattern=*/false);
   EXPECT_EQ(Snippet, " ${1:name} = ${2:target};");
 
   // When completing a pattern, the last placeholder holds the cursor position.
   computeSignature(MakeCCS(), /*CompletingPattern=*/true);
-  EXPECT_EQ(Snippet, " ${1:name} = ${0:target};");
+  EXPECT_EQ(Snippet, " ${1:name} = $0;");
 }
 
 TEST_F(CompletionStringTest, IgnoreInformativeQualifier) {
   Builder.AddTypedTextChunk("X");
   Builder.AddInformativeChunk("info ok");
   Builder.AddInformativeChunk("info no no::");
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "info ok");