[clangd] Refine logic on $0 in completion snippets

[zyounan]

Summary

We have a workaround from D128621 that makes $0 no longer being
a placeholder to conform a vscode feature. However, we have to
refine the logic as it may suppress the last parameter placeholder
for constructor of base class because not all patterns of completion
are compound statements.

This fixes clangd/clangd#1479

[zyounan]

clang-tools-extra/clangd/CodeCompletionStrings.cpp

153

I was cringed that if we should refine the logic based on CursorKind: It is from libclang; The meaning is sometimes kind of opaque (to me, I don't know it very clearly TBH) like CXCursor_NotImplemented...

[zyounan]

Ping :)

[nridge]

Thanks for the patch!

In the future we may want to add other cursor kinds for which ShouldPatchPlaceholder0 should be false, but I think this is a good start which addresses the case from the bug.

clang-tools-extra/clangd/CodeComplete.cpp

439–440

Since we are now passing in the entire C.SemaResult, can we remove the CompletingPattern parameter, and compute that boolean inside the function instead?

clang-tools-extra/clangd/CodeCompletionStrings.cpp

124–125

Can we move this check after the declaration of ShouldPatchPlaceholder0, and change the condition to if (ShouldPatchPlaceholder0)?

(And at the usage site, swap the operands to if (ShouldPatchPlaceholder0 && SnippetArg == CursorSnippetArg)?)

This would avoid doing the count_if when not necessary.

153

It does seem like a layering violation that a libSema interface (CodeCompletionResult) uses a libclang type (CXCursorKind), but that's a pre-existing issue.

[zyounan]

Address the comments. Revise parameters of getSignature to avoid passing CCR

clang-tools-extra/clangd/CodeComplete.cpp

439–440

Revise the parameters of getSignature to circumvent CCR now -- Tests for CodeCompletionStrings do not construct any CCR and I don't think it necessary to add that logic.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

124–125

Nice catch! thanks!

153

It does seem like a layering violation that a libSema interface (CodeCompletionResult) uses a libclang type (CXCursorKind), but that's a pre-existing issue.

153

Thanks for clarifying.

[zyounan]

Update doxygen comments

clang-tools-extra/clangd/CodeCompletionStrings.h

44–45

Sorry, I forget to revise the comments :(

[nridge]

Thanks for the update! I have a couple of more minor suggestions, hope you don't mind; I'm trying to make sure the next person to look at this will easily understand what the code is trying to do.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

131

Can we factor this out to a namespace scope function bool shouldPatchPlaceholder0(CodeCompletionResult::ResultKind ResultKind, CXCursorKind CursorKind)?

Then the code here becomes pretty simple:

unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max();
if (shouldPatchPlaceholder0(ResultKind, CursorKind)) {
  CursorSnippetArg = count_if(...);
}

And we can combine some of the comments here into a single comment that explains what we are doing:

// By default, the final cursor position is at the end of the snippet,
// but we have the option to place it somewhere else using $0.
// If the snippet contains a group of statements, we replace the
// last placeholder with $0 to leave the cursor there, e.g.
//    namespace ${1:name} {
//      ${0:decls}
//    }
// We try to identify such cases using the ResultKind and CursorKind.

219

I realized the first part of the condition is now redundant: if ShouldPatchPlaceholder0 == false, then CursorSnippetArg will have value std::numeric_limits<unsigned>::max() and SnippetArg == CursorSnippetArg will never be true.

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

31–33

Suggestion: it may be easier to understand if we change the bool CompletingPattern = false parameter of this function to CodeCompletionResult::ResultKind ResultKind = CodeCompletionResult::ResultKind::RK_Declaration.

(And the call site which passed CompletingPattern=true can now pass ResultKind=RK_Pattern.)

Then we don't need to refer to historical behaviour.

[zyounan]

Address comments

[zyounan]

Updated now. Thank you again for your patient review. :)

clang-tools-extra/clangd/CodeCompletionStrings.cpp

219

Aha, yes. Almost forget that.

[nridge]

Thanks! I have just one final nit, then please feel free to merge.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

60

nit: let's add a short comment to describe the function, such as:

// Determine whether the completion string should be patched
// to replace the last placeholder with $0

[zyounan]

Add description to shouldPatchPlaceholder0

clang-tools-extra/clangd/CodeCompletionStrings.cpp

60

Sure!

clang-tools-extra/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, IsPattern);
+      getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix, C.SemaResult->Kind,
+                   C.SemaResult->CursorKind, &Completion.RequiredQualifier);
       if (!C.SemaResult->FunctionCanBeCall)
         S.SnippetSuffix.clear();
       S.ReturnType = getReturnType(*SemaCCS);
     } else if (C.IndexResult) {
       S.Signature = std::string(C.IndexResult->Signature);
       S.SnippetSuffix = std::string(C.IndexResult->CompletionSnippetSuffix);
       S.ReturnType = std::string(C.IndexResult->ReturnType);
     }

clang-tools-extra/clangd/CodeCompletionStrings.h

 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 = "(${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.
+/// When \p ResultKind is RK_Pattern, the last placeholder will be $0,
+/// indicating the cursor should stay there.
+/// Note that for certain \p CursorKind like \p CXCursor_Constructor, $0 won't
+/// be emitted in order to avoid overlapping normal parameters.
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
                   std::string *Snippet,
-                  std::string *RequiredQualifiers = nullptr,
-                  bool CompletingPattern = false);
+                  CodeCompletionResult::ResultKind ResultKind,
+                  CXCursorKind CursorKind,
+                  std::string *RequiredQualifiers = nullptr);
 
 /// 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/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-c/Index.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/JSON.h"
 #include <limits>
 #include <utility>
 
bool looksLikeDocComment(llvm::StringRef CommentText) {
   // We don't report comments that only contain "special" chars.
   // This avoids reporting various delimiters, like:
   //   =================
   //   -----------------
   //   *****************
   return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
 }
 
+// Determine whether the completion string should be patched
+// to replace the last placeholder with $0.
+bool shouldPatchPlaceholder0(CodeCompletionResult::ResultKind ResultKind,
+                             CXCursorKind CursorKind) {
+  bool CompletingPattern = ResultKind == CodeCompletionResult::RK_Pattern;
+
+  if (!CompletingPattern)
+    return false;
+
+  // If the result kind of CodeCompletionResult(CCR) is `RK_Pattern`, it doesn't
+  // always mean we're completing a chunk of statements.  Constructors defined
+  // in base class, for example, are considered as a type of pattern, with the
+  // cursor type set to CXCursor_Constructor.
+  if (CursorKind == CXCursorKind::CXCursor_Constructor ||
+      CursorKind == CXCursorKind::CXCursor_Destructor)
+    return false;
+
+  return true;
+}
+
 } // namespace
 
 std::string getDocComment(const ASTContext &Ctx,
                           const CodeCompletionResult &Result,
                           bool CommentsFromHeaders) {
   // FIXME: clang's completion also returns documentation for RK_Pattern if they
   // contain a pattern for ObjC properties. Unfortunately, there is no API to
   // get this declaration, so we don't show documentation in that case.
if (!looksLikeDocComment(Doc))
     return "";
   // Clang requires source to be UTF-8, but doesn't enforce this in comments.
   if (!llvm::json::isUTF8(Doc))
     Doc = llvm::json::fixUTF8(Doc);
   return Doc;
 }
 
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
-                  std::string *Snippet, std::string *RequiredQualifiers,
-                  bool CompletingPattern) {
-  // Placeholder with this index will be ${0:…} to mark final cursor position.
+                  std::string *Snippet,
+                  CodeCompletionResult::ResultKind ResultKind,
+                  CXCursorKind CursorKind, std::string *RequiredQualifiers) {
+  // 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}
-    //    }
+
+  // If the snippet contains a group of statements, we replace the
+  // last placeholder with $0 to leave the cursor there, e.g.
+  // We try to identify such cases using the ResultKind and CursorKind.
+  if (shouldPatchPlaceholder0(ResultKind, CursorKind)) {
     CursorSnippetArg =
         llvm::count_if(CCS, [](const CodeCompletionString::Chunk &C) {
           return C.Kind == CodeCompletionString::CK_Placeholder;
         });
   }
   unsigned SnippetArg = 0;
   bool HadObjCArguments = false;
   bool HadInformativeChunks = false;
   for (const auto &Chunk : CCS) {
     // Informative qualifier chunks only clutter completion results, skip
     // them.
     if (isInformativeQualifierChunk(Chunk))
       continue;
 
     switch (Chunk.Kind) {
     case CodeCompletionString::CK_TypedText:
       // The typed-text chunk is the actual name. We don't record this chunk.
       // C++:
       //   In general our string looks like <qualifiers><name><signature>.
       //   So once we see the name, any text we recorded so far should be
       //   reclassified as qualifiers.
       //
       // Objective-C:
       //   Objective-C methods expressions may have multiple typed-text chunks,
       //   so we must treat them carefully. For Objective-C methods, all
       //   typed-text and informative chunks will end in ':' (unless there are
       //   no arguments, in which case we can safely treat them as C++).
       //
       //   Completing a method declaration itself (not a method expression) is
case CodeCompletionString::CK_TypedText:
         }
       }
       break;
     case CodeCompletionString::CK_Text:
       *Signature += Chunk.Text;
       *Snippet += Chunk.Text;
       break;
     case CodeCompletionString::CK_Optional:
-      assert(Chunk.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;
       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 += '}';
       }

clang-tools-extra/clangd/index/SymbolCollector.cpp

bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
   if (auto DeclLoc = getTokenLocation(DefLoc))
     S.CanonicalDeclaration = *DeclLoc;
 
   CodeCompletionResult SymbolCompletion(Name);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionStringForMacro(
       *PP, *CompletionAllocator, *CompletionTUInfo);
   std::string Signature;
   std::string SnippetSuffix;
-  getSignature(*CCS, &Signature, &SnippetSuffix);
+  getSignature(*CCS, &Signature, &SnippetSuffix, SymbolCompletion.Kind,
+               SymbolCompletion.CursorKind);
   S.Signature = Signature;
   S.CompletionSnippetSuffix = SnippetSuffix;
 
   IndexedMacros.insert(Name);
   setIncludeLocation(S, DefLoc);
   Symbols.insert(S);
   return true;
 }
if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
     if (Opts.StoreAllDocumentation)
       S.Documentation = Documentation;
     Symbols.insert(S);
     return Symbols.find(S.ID);
   }
   S.Documentation = Documentation;
   std::string Signature;
   std::string SnippetSuffix;
-  getSignature(*CCS, &Signature, &SnippetSuffix);
+  getSignature(*CCS, &Signature, &SnippetSuffix, SymbolCompletion.Kind,
+               SymbolCompletion.CursorKind);
   S.Signature = Signature;
   S.CompletionSnippetSuffix = SnippetSuffix;
   std::string ReturnType = getReturnType(*CCS);
   S.ReturnType = ReturnType;
 
   std::optional<OpaqueType> TypeStorage;
   if (S.Flags & Symbol::IndexedForCodeCompletion) {
     TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);

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

TEST(CompletionTest, CursorInSnippets) {
   // 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\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})"))));
+
+  Results = completions(R"cpp(
+    struct Base {
+      Base(int a, int b) {}
+    };
+
+    struct Derived : Base {
+      Derived() : Base^
+    };
+  )cpp",
+                        /*IndexSymbols=*/{}, Options);
+  // Constructors from base classes are a kind of pattern that shouldn't end
+  // with $0.
+  EXPECT_THAT(Results.Completions,
+              Contains(AllOf(named("Base"),
+                             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^;
       }

clang-tools-extra/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,
-                        bool CompletingPattern = false) {
+                        CodeCompletionResult::ResultKind ResultKind =
+                            CodeCompletionResult::ResultKind::RK_Declaration) {
     Signature.clear();
     Snippet.clear();
-    getSignature(CCS, &Signature, &Snippet, /*RequiredQualifier=*/nullptr,
-                 CompletingPattern);
+    getSignature(CCS, &Signature, &Snippet, ResultKind,
+                 /*CursorKind=*/CXCursorKind::CXCursor_NotImplemented,
+                 /*RequiredQualifiers=*/nullptr);
   }
 
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
   CodeCompletionBuilder Builder;
   std::string Signature;
   std::string Snippet;
 };
auto MakeCCS = [this]() -> const CodeCompletionString & {
     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);
+  computeSignature(MakeCCS());
   EXPECT_EQ(Snippet, " ${1:name} = ${2:target};");
 
   // When completing a pattern, the last placeholder holds the cursor position.
-  computeSignature(MakeCCS(), /*CompletingPattern=*/true);
+  computeSignature(MakeCCS(),
+                   /*ResultKind=*/CodeCompletionResult::ResultKind::RK_Pattern);
   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());