[clangd] Place cursor better after completing patterns

[ilya-biryukov]

Summary

By producing the $0 marker in the snippets at the last placeholder.
This produces nicer results in most cases, e.g. for

namespace <#name#> {
  <#decls#>
}

we now produce ${0:decls} instead of ${2:decls} and the final cursor
placement is more convenient.

[hokein]

I'm not sure the new behavior suits all cases, it seems to me that this will change a typical behavior (usually when users invoke function calls).

Imaging a case:

void foo(int a, int b);

void call() {
  fo^
}

• before, the final cursor is at the end of the snippet
• after, the final cursor is at the last parameter of foo.

I think the previous cursor location is more convenient (user can continue typing ;).

clang-tools-extra/clangd/CodeCompletionStrings.h

41 ↗

(On Diff #201227)

IIUC, I think we will still return ${0} with the new behavior, size_type is the only placeholder here.

[ilya-biryukov]

In D62389#1517844, @hokein wrote:

I'm not sure the new behavior suits all cases, it seems to me that this will change a typical behavior (usually when users invoke function calls).

Imaging a case:

void foo(int a, int b);

void call() {
  fo^
}

• before, the final cursor is at the end of the snippet
• after, the final cursor is at the last parameter of foo.

I think the previous cursor location is more convenient (user can continue typing ;).

Agree! The new behavior does not apply to function calls, they are not considered 'patterns' in the completion results.
I.e. in your examples we're still getting foo(${1:int a}, ${2:int b}) and the cursor is placed at the end.

This is definitely worth a comment, I'll try to find a good place for it.

[ilya-biryukov]

clang-tools-extra/clangd/CodeCompletionStrings.h

41 ↗

(On Diff #201227)

The comment was incorrect, we did not return ${0:} placeholders before.
Note that behavior for function call arguments is the same in both the old and the new version.

[hokein]

In D62389#1517907, @ilya-biryukov wrote:

In D62389#1517844, @hokein wrote:

I'm not sure the new behavior suits all cases, it seems to me that this will change a typical behavior (usually when users invoke function calls).

Imaging a case:

void foo(int a, int b);

void call() {
  fo^
}

• before, the final cursor is at the end of the snippet
• after, the final cursor is at the last parameter of foo.

I think the previous cursor location is more convenient (user can continue typing ;).

Agree! The new behavior does not apply to function calls, they are not considered 'patterns' in the completion results.
I.e. in your examples we're still getting foo(${1:int a}, ${2:int b}) and the cursor is placed at the end.

This is definitely worth a comment, I'll try to find a good place for it.

Ah, that makes sense! Thanks for clarifying it.

How about adding one more test in CodeCompleteTests.cpp, we already have a snippet test there, and it seems more straightforward.

[ilya-biryukov]

• Add a code completion test

[ilya-biryukov]

In D62389#1518030, @hokein wrote:

How about adding one more test in CodeCompleteTests.cpp, we already have a snippet test there, and it seems more straightforward.

Done!

[hokein]

LGTM

clang-tools-extra/trunk/clangd/CodeComplete.cpp

std::stable_partition(Completion.Includes.begin(),
                           });
   }
 
   void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
     assert(bool(C.SemaResult) == bool(SemaCCS));
     Bundled.emplace_back();
     BundledEntry &S = Bundled.back();
     if (C.SemaResult) {
+      bool IsPattern = C.SemaResult->Kind == CodeCompletionResult::RK_Pattern;
       getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix,
-                   &Completion.RequiredQualifier);
+                   &Completion.RequiredQualifier, IsPattern);
       S.ReturnType = getReturnType(*SemaCCS);
     } else if (C.IndexResult) {
       S.Signature = C.IndexResult->Signature;
       S.SnippetSuffix = C.IndexResult->CompletionSnippetSuffix;
       S.ReturnType = C.IndexResult->ReturnType;
     }
     if (ExtractDocumentation && Completion.Documentation.empty()) {
       if (C.IndexResult)

clang-tools-extra/trunk/clangd/CodeCompletionStrings.h

std::string getDocComment(const ASTContext &Ctx,
                           bool CommentsFromHeaders);
 
 /// Similar to getDocComment, but returns the comment for a NamedDecl.
 std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &D);
 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector<T>::at(size_type) const, this returns:
 ///   *Signature = "(size_type) const"
-///   *Snippet = "(${0:size_type})"
+///   *Snippet = "(${1:size_type})"
 /// If set, RequiredQualifiers is the text that must be typed before the name.
 /// e.g "Base::" when calling a base class member function that's hidden.
+///
+/// When \p CompletingPattern is true, the last placeholder will be of the form
+/// ${0:…}, indicating the cursor should stay there.
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
                   std::string *Snippet,
-                  std::string *RequiredQualifiers = nullptr);
+                  std::string *RequiredQualifiers = nullptr,
+                  bool CompletingPattern = false);
 
 /// Assembles formatted documentation for a completion result. This includes
 /// documentation comments and other relevant information like annotations.
 ///
 /// \param DocComment is a documentation comment for the original declaration,
 ///        it should be obtained via getDocComment or getParameterDocComment.
 std::string formatDocumentation(const CodeCompletionString &CCS,
                                 llvm::StringRef DocComment);

clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp

 //===--- CodeCompletionStrings.cpp -------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
+#include <limits>
 #include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 bool isInformativeQualifierChunk(CodeCompletionString::Chunk const &Chunk) {
   return Chunk.Kind == CodeCompletionString::CK_Informative &&
std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
   // write them into PCH, because they are racy and slow to load.
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc =
       RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
   return looksLikeDocComment(Doc) ? Doc : "";
 }
 
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
-                  std::string *Snippet, std::string *RequiredQualifiers) {
-  unsigned ArgCount = 0;
+                  std::string *Snippet, std::string *RequiredQualifiers,
+                  bool CompletingPattern) {
+  // Placeholder with this index will be ${0:…} to mark final cursor position.
+  // Usually we do not add $0, so the cursor is placed at end of completed text.
+  unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max();
+  if (CompletingPattern) {
+    // In patterns, it's best to place the cursor at the last placeholder, to
+    // handle cases like
+    //    namespace ${1:name} {
+    //      ${0:decls}
+    //    }
+    CursorSnippetArg =
+        llvm::count_if(CCS, [](const CodeCompletionString::Chunk &C) {
+          return C.Kind == CodeCompletionString::CK_Placeholder;
+        });
+  }
+  unsigned SnippetArg = 0;
   bool HadObjCArguments = false;
   for (const auto &Chunk : CCS) {
     // Informative qualifier chunks only clutter completion results, skip
     // them.
     if (isInformativeQualifierChunk(Chunk))
       continue;
 
     switch (Chunk.Kind) {
for (const auto &Chunk : CCS) {
     case CodeCompletionString::CK_Text:
       *Signature += Chunk.Text;
       *Snippet += Chunk.Text;
       break;
     case CodeCompletionString::CK_Optional:
       break;
     case CodeCompletionString::CK_Placeholder:
       *Signature += Chunk.Text;
-      ++ArgCount;
-      *Snippet += "${" + std::to_string(ArgCount) + ':';
+      ++SnippetArg;
+      *Snippet +=
+          "${" +
+          std::to_string(SnippetArg == CursorSnippetArg ? 0 : SnippetArg) + ':';
       appendEscapeSnippet(Chunk.Text, Snippet);
       *Snippet += '}';
       break;
     case CodeCompletionString::CK_Informative:
       // 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.

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

TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromMiddle) {
   auto C = Results.Completions;
   EXPECT_THAT(C, ElementsAre(Named("fooey:")));
   EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
   EXPECT_THAT(C, ElementsAre(ReturnType("id")));
   EXPECT_THAT(C, ElementsAre(Signature("(unsigned int)")));
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, CursorInSnippets) {
+  clangd::CodeCompleteOptions Options;
+  Options.EnableSnippets = true;
+  auto Results = completions(
+      R"cpp(
+    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}){${0:statements}\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(
     int main() {
       for (auto [loopVar] : y ) { // y has to be unresolved.
         int z = loopV^;
       }
     }
   )cpp");

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

 
 class CompletionStringTest : public ::testing::Test {
 public:
   CompletionStringTest()
       : Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator), Builder(*Allocator, CCTUInfo) {}
 
 protected:
-  void computeSignature(const CodeCompletionString &CCS) {
+  void computeSignature(const CodeCompletionString &CCS,
+                        bool CompletingPattern = false) {
     Signature.clear();
     Snippet.clear();
-    getSignature(CCS, &Signature, &Snippet);
+    getSignature(CCS, &Signature, &Snippet, /*RequiredQualifier=*/nullptr,
+                 CompletingPattern);
   }
 
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
   CodeCompletionBuilder Builder;
   std::string Signature;
   std::string Snippet;
 };
TEST_F(CompletionStringTest, EscapeSnippet) {
   Builder.AddPlaceholderChunk("$p}1\\");
   Builder.AddChunk(CodeCompletionString::CK_RightParen);
 
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "($p}1\\)");
   EXPECT_EQ(Snippet, "(${1:\\$p\\}1\\\\})");
 }
 
+TEST_F(CompletionStringTest, SnippetsInPatterns) {
+  auto MakeCCS = [this]() -> const CodeCompletionString & {
+    CodeCompletionBuilder Builder(*Allocator, CCTUInfo);
+    Builder.AddTypedTextChunk("namespace");
+    Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+    Builder.AddPlaceholderChunk("name");
+    Builder.AddChunk(CodeCompletionString::CK_Equal);
+    Builder.AddPlaceholderChunk("target");
+    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};");
+}
+
 TEST_F(CompletionStringTest, IgnoreInformativeQualifier) {
   Builder.AddTypedTextChunk("X");
   Builder.AddInformativeChunk("info ok");
   Builder.AddInformativeChunk("info no no::");
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "info ok");
   EXPECT_EQ(Snippet, "");
 }