[clangd] Refine logic on $0 in completion snippets
authorYounan Zhang <zyn7109@gmail.com>
Tue, 14 Mar 2023 05:15:34 +0000 (13:15 +0800)
committerYounan Zhang <zyn7109@gmail.com>
Fri, 17 Mar 2023 09:57:24 +0000 (17:57 +0800)
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

Reviewed By: nridge

Differential Revision: https://reviews.llvm.org/D145319

clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/CodeCompletionStrings.cpp
clang-tools-extra/clangd/CodeCompletionStrings.h
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp

index 98fa285..260c44b 100644 (file)
@@ -436,9 +436,8 @@ struct CodeCompletionBuilder {
     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);
index 21f8345..26f8e0e 100644 (file)
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
+#include "clang-c/Index.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
@@ -56,6 +57,26 @@ bool looksLikeDocComment(llvm::StringRef CommentText) {
   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,
@@ -95,17 +116,20 @@ std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
 }
 
 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.
+  //    namespace ${1:name} {
+  //      ${0:decls}
+  //    }
+  // 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;
@@ -185,7 +209,7 @@ void getSignature(const CodeCompletionString &CCS, std::string *Signature,
       *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;
index 6733d02..566756a 100644 (file)
@@ -42,12 +42,15 @@ std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &D);
 /// 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.
index 24d0104..3179810 100644 (file)
@@ -756,7 +756,8 @@ bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
       *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;
 
@@ -933,7 +934,8 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID 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);
index f57f266..25d6a7b 100644 (file)
@@ -3450,6 +3450,22 @@ TEST(CompletionTest, CursorInSnippets) {
   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) {
index 329a213..faab625 100644 (file)
@@ -24,11 +24,13 @@ public:
 
 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;
@@ -145,11 +147,12 @@ TEST_F(CompletionStringTest, SnippetsInPatterns) {
     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;");
 }