diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -436,9 +436,8 @@ 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); diff --git a/clang-tools-extra/clangd/CodeCompletionStrings.h b/clang-tools-extra/clangd/CodeCompletionStrings.h --- a/clang-tools-extra/clangd/CodeCompletionStrings.h +++ b/clang-tools-extra/clangd/CodeCompletionStrings.h @@ -42,12 +42,15 @@ /// 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. diff --git a/clang-tools-extra/clangd/CodeCompletionStrings.cpp b/clang-tools-extra/clangd/CodeCompletionStrings.cpp --- a/clang-tools-extra/clangd/CodeCompletionStrings.cpp +++ b/clang-tools-extra/clangd/CodeCompletionStrings.cpp @@ -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 @@ 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 @@ } 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::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 @@ *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; diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -756,7 +756,8 @@ *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 @@ 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); diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -3450,6 +3450,22 @@ 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) { diff --git a/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp @@ -24,11 +24,13 @@ 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 Allocator; @@ -145,11 +147,12 @@ 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;"); }