[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE335980: [clangd] codeComplete returns more structured
completion items, LSP. NFC. (authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48762?vs=153493=153495#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48762
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Protocol.h
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -165,11 +165,15 @@
// FIXME(ibiryukov): even if Preamble is non-null, we may want to check
// both the old and the new version in case only one of them matches.
-CompletionList Result = clangd::codeComplete(
+CodeCompleteResult Result = clangd::codeComplete(
File, IP->Command, PreambleData ? >Preamble : nullptr,
PreambleData ? PreambleData->Inclusions : std::vector(),
IP->Contents, Pos, FS, PCHs, CodeCompleteOpts);
-CB(std::move(Result));
+CompletionList LSPResult;
+LSPResult.isIncomplete = Result.HasMore;
+for (const auto : Result.Completions)
+ LSPResult.items.push_back(Completion.render(CodeCompleteOpts));
+CB(std::move(LSPResult));
};
WorkScheduler.runWithPreamble("CodeComplete", File,
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -668,20 +668,6 @@
Snippet = 2,
};
-/// Provides details for how a completion item was scored.
-/// This can be used for client-side filtering of completion items as the
-/// user keeps typing.
-/// This is a clangd extension.
-struct CompletionItemScores {
- /// The score that items are ranked by.
- /// This is filterScore * symbolScore.
- float finalScore = 0.f;
- /// How the partial identifier matched filterText. [0-1]
- float filterScore = 0.f;
- /// How the symbol fits, ignoring the partial identifier.
- float symbolScore = 0.f;
-};
-
struct CompletionItem {
/// The label of this completion item. By default also the text that is
/// inserted when selecting this completion.
@@ -702,9 +688,6 @@
/// When `falsy` the label is used.
std::string sortText;
- /// Details about the quality of this completion item. (clangd extension)
- llvm::Optional scoreInfo;
-
/// A string that should be used when filtering a set of completion items.
/// When `falsy` the label is used.
std::string filterText;
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -43,7 +43,7 @@
#include
// We log detailed candidate here if you run with -debug-only=codecomplete.
-#define DEBUG_TYPE "codecomplete"
+#define DEBUG_TYPE "CodeComplete"
namespace clang {
namespace clangd {
@@ -237,138 +237,154 @@
return IndexResult->Detail->IncludeHeader;
}
- // Builds an LSP completion item.
- CompletionItem build(StringRef FileName, const CompletionItemScores ,
- const CodeCompleteOptions ,
- CodeCompletionString *SemaCCS,
- const IncludeInserter ,
- llvm::StringRef SemaDocComment) const {
-assert(bool(SemaResult) == bool(SemaCCS));
-assert(SemaResult || IndexResult);
-
-CompletionItem I;
-bool InsertingInclude = false; // Whether a new #include will be added.
-if (SemaResult) {
- llvm::StringRef Name(SemaCCS->getTypedText());
- std::string Signature, SnippetSuffix, Qualifiers;
- getSignature(*SemaCCS, , , );
- I.label = (Qualifiers + Name + Signature).str();
- I.filterText = Name;
- I.insertText = (Qualifiers + Name).str();
- if (Opts.EnableSnippets)
-I.insertText += SnippetSuffix;
- I.documentation = formatDocumentation(*SemaCCS, SemaDocComment);
- I.detail = getReturnType(*SemaCCS);
- if (SemaResult->Kind == CodeCompletionResult::RK_Declaration)
-if (const auto *D = SemaResult->getDeclaration())
- if (const auto *ND = llvm::dyn_cast(D))
-I.SymbolScope = splitQualifiedName(printQualifiedName(*ND)).first;
- I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->Declaration);
-}
-if (IndexResult) {
- if (I.SymbolScope.empty())
-I.SymbolScope = IndexResult->Scope;
- if (I.kind == CompletionItemKind::Missing)
-I.kind = toCompletionItemKind(IndexResult->SymInfo.Kind);
- // FIXME: reintroduce a way to show the index source for debugging.
- if (I.label.empty())
-I.label = (IndexResult->Name + IndexResult->Signature).str();
- if (I.filterText.empty())
-I.filterText = IndexResult->Name;
-
- // FIXME(ioeric): support inserting/replacing scope qualifiers.
- if (I.insertText.empty()) {
-
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48762 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
sammccall updated this revision to Diff 153493.
sammccall marked 11 inline comments as done.
sammccall added a comment.
Address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48762
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Protocol.h
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -668,20 +668,6 @@
Snippet = 2,
};
-/// Provides details for how a completion item was scored.
-/// This can be used for client-side filtering of completion items as the
-/// user keeps typing.
-/// This is a clangd extension.
-struct CompletionItemScores {
- /// The score that items are ranked by.
- /// This is filterScore * symbolScore.
- float finalScore = 0.f;
- /// How the partial identifier matched filterText. [0-1]
- float filterScore = 0.f;
- /// How the symbol fits, ignoring the partial identifier.
- float symbolScore = 0.f;
-};
-
struct CompletionItem {
/// The label of this completion item. By default also the text that is
/// inserted when selecting this completion.
@@ -702,9 +688,6 @@
/// When `falsy` the label is used.
std::string sortText;
- /// Details about the quality of this completion item. (clangd extension)
- llvm::Optional scoreInfo;
-
/// A string that should be used when filtering a set of completion items.
/// When `falsy` the label is used.
std::string filterText;
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -75,15 +75,79 @@
const SymbolIndex *Index = nullptr;
};
+// Semi-structured representation of a code-complete suggestion for our C++ API.
+// We don't use the LSP structures here (unlike most features) as we want
+// to expose more data to allow for more precise testing and evaluation.
+struct CodeCompletion {
+ // The unqualified name of the symbol or other completion item.
+ std::string Name;
+ // The scope qualifier for the symbol name. e.g. "ns1::ns2::"
+ // Empty for non-symbol completions. Not inserted, but may be displayed.
+ std::string Scope;
+ // Text that must be inserted before the name, and displayed (e.g. base::).
+ std::string RequiredQualifier;
+ // Details to be displayed following the name. Not inserted.
+ std::string Signature;
+ // Text to be inserted following the name, in snippet format.
+ std::string SnippetSuffix;
+ // Type to be displayed for this completion.
+ std::string ReturnType;
+ std::string Documentation;
+ CompletionItemKind Kind = CompletionItemKind::Missing;
+ // This completion item may represent several symbols that can be inserted in
+ // the same way, such as function overloads. In this case BundleSize > 1, and
+ // the following fields are summaries:
+ // - Signature is e.g. "(...)" for functions.
+ // - SnippetSuffix is similarly e.g. "(${0})".
+ // - ReturnType may be empty
+ // - Documentation may be from one symbol, or a combination of several
+ // Other fields should apply equally to all bundled completions.
+ unsigned BundleSize;
+ // The header through which this symbol could be included.
+ // Quoted string as expected by an #include directive, e.g. "".
+ // Empty for non-symbol completions, or when not known.
+ std::string Header;
+ // Present if Header is set and should be inserted to use this item.
+ llvm::Optional HeaderInsertion;
+
+ // Scores are used to rank completion items.
+ struct Scores {
+// The score that items are ranked by.
+float Total = 0.f;
+
+// The finalScore with the fuzzy name match score excluded.
+// When filtering client-side, editors should calculate the new fuzzy score,
+// whose scale is 0-1 (with 1 = prefix match, special case 2 = exact match),
+// and recompute finalScore = fuzzyScore * symbolScore.
+float ExcludingName = 0.f;
+
+// Component scores that contributed to the final score:
+
+// Quality describes how important we think this candidate is,
+// independent of the query.
+// e.g. symbols with lots of incoming references have higher quality.
+float Quality = 0.f;
+// Relevance describes how well this candidate matched the query.
+// e.g. symbols from nearby files have higher relevance.
+float Relevance = 0.f;
+ };
+ Scores Score;
+
+ // Serialize this to an LSP completion item. This is a lossy operation.
+ CompletionItem render(const CodeCompleteOptions &) const;
+};
+struct CodeCompleteResult {
+ std::vector Completions;
+ bool HasMore = false;
+};
+
/// Get code completions at a specified \p Pos in \p FileName.
-CompletionList codeComplete(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-const std::vector ,
-StringRef Contents,
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
sammccall added inline comments.
Comment at: clangd/CodeComplete.cpp:259
+ : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+if (C.SemaResult) {
+ Completion.Name = llvm::StringRef(SemaCCS->getTypedText());
ioeric wrote:
> nit: shall we keep `assert(bool(SemaResult) == bool(SemaCCS));`?
Done (in `add()`, and hoisted the call to `add()` to the top here)
Comment at: clangd/CodeComplete.cpp:322
+}
+if (ExtractDocumentation && Completion.Documentation.empty()) {
+ if (C.IndexResult && C.IndexResult->Detail)
ioeric wrote:
> nit: the documentation for the bundling says `Documentation may contain a
> combined docstring from all comments`. The implementation seems to take the
> documentation from the last comment (which seems fine to me), but they should
> be consistent.
This was meant to be ambiguous ("may") to allow a better implementation later.
But rewrote it to mention the current behavior as a possibility :-)
Comment at: clangd/CodeComplete.cpp:1145
+Scores.ExcludingName = Relevance.NameMatch
+ ? Scores.Total / Relevance.NameMatch
+ : Scores.Quality;
ioeric wrote:
> nit: this seems to assume that `NameMatch` is a factor of the final score,
> which seems to be changeable given the abstraction of
> `evaluateSymbolAndRelevance`. Not sure if there is an alternative to the
> current approach, but this probably deserves a comment.
Yeah, client-side filtering is the great abstraction-breaker :-(
Added the comment, not sure there's much more to be done.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48762
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
ioeric added a comment.
Looks great! Mostly nits.
Comment at: clangd/CodeComplete.cpp:259
+ : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+if (C.SemaResult) {
+ Completion.Name = llvm::StringRef(SemaCCS->getTypedText());
nit: shall we keep `assert(bool(SemaResult) == bool(SemaCCS));`?
Comment at: clangd/CodeComplete.cpp:279
+if (auto Inserted = C.headerToInsertIfNotPresent()) {
+ llvm::errs() << "Inserted = " << *Inserted << "\n";
+ Completion.Header = *Inserted;
remove?
Comment at: clangd/CodeComplete.cpp:280
+ llvm::errs() << "Inserted = " << *Inserted << "\n";
+ Completion.Header = *Inserted;
+ // Turn absolute path into a literal string that can be #included.
`Inserted` can be an absolute path which might have long uninteresting prefix.
It might make sense to use the result from `calculateIncludePath`?
Comment at: clangd/CodeComplete.cpp:309
+
+ void add(const CompletionCandidate , CodeCompletionString *SemaCCS) {
+Bundled.emplace_back();
nit: It's unclear why `add` is a public interface given that the constructor
also adds candidate. Might worth a comment. Also, it seems that the interface
design has been driven by by the bundling logic, so it might also make sense to
explain a bit on the class level.
Comment at: clangd/CodeComplete.cpp:322
+}
+if (ExtractDocumentation && Completion.Documentation.empty()) {
+ if (C.IndexResult && C.IndexResult->Detail)
nit: the documentation for the bundling says `Documentation may contain a
combined docstring from all comments`. The implementation seems to take the
documentation from the last comment (which seems fine to me), but they should
be consistent.
Comment at: clangd/CodeComplete.cpp:346
+
+ template
+ const std::string *onlyValue() const {
nit: this might worth a comment. it's unclear what this does by just looking at
the name.
Comment at: clangd/CodeComplete.cpp:1145
+Scores.ExcludingName = Relevance.NameMatch
+ ? Scores.Total / Relevance.NameMatch
+ : Scores.Quality;
nit: this seems to assume that `NameMatch` is a factor of the final score,
which seems to be changeable given the abstraction of
`evaluateSymbolAndRelevance`. Not sure if there is an alternative to the
current approach, but this probably deserves a comment.
Comment at: clangd/CodeComplete.cpp:1161
- CompletionItem toCompletionItem(const CompletionCandidate::Bundle ,
- const CompletionItemScores ) {
-CodeCompletionString *SemaCCS = nullptr;
-std::string FrontDocComment;
-if (auto *SR = Bundle.front().SemaResult) {
- SemaCCS = Recorder->codeCompletionString(*SR);
- if (Opts.IncludeComments) {
-assert(Recorder->CCSema);
-FrontDocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR,
-/*CommentsFromHeader=*/false);
- }
+ CodeCompletion toCompletionItem(const CompletionCandidate::Bundle ) {
+llvm::Optional Builder;
nit: toCompletionItem might be ambiguous now that this returns `CodeCompletion`.
Comment at: clangd/CodeComplete.h:88
+ // Qualifiers that must be inserted before the name (e.g. base::).
+ std::string Qualifiers;
+ // Details to be displayed following the name. Not inserted.
Give that this field has caused confusion before, would it make sense to make
the name a bit more concrete like `RequiredQualifier` or `InsertedQualifier`?
Comment at: clangd/CodeComplete.h:106
+ unsigned BundleSize;
+ // The header through which this symbol should be included.
+ // Empty for non-symbol completions.
Would this be an absolute path or an include path?
Comment at: clangd/CodeComplete.h:140
+ std::vector Completions;
+ bool More = false;
+};
I found `Result.More` a bit not straightforward. Maybe `HasMore`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48762
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
sammccall added a comment. @jkorous FYI: this may be interesting from an XPC perspective - it'd be easy to serialize the LSP-style struct, or the more detailed one if that's useful. LSP will be more stable of course :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48762 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC.
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
LSP has some presentational fields with limited semantics (e.g. 'detail') and
doesn't provide a good place to return information like namespace.
Some places where more detailed information is useful:
- tools like quality analysis
- alternate frontends that aren't strictly LSP
- code completion unit tests
In this patch, ClangdServer::codeComplete still return LSP CompletionList, but
I plan to switch that soon (should be a no-op for ClangdLSPServer).
Deferring this makes it clear that we don't change behavior (tests stay the
same) and also keeps the API break to a small patch which can be reverted.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48762
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Protocol.h
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -668,20 +668,6 @@
Snippet = 2,
};
-/// Provides details for how a completion item was scored.
-/// This can be used for client-side filtering of completion items as the
-/// user keeps typing.
-/// This is a clangd extension.
-struct CompletionItemScores {
- /// The score that items are ranked by.
- /// This is filterScore * symbolScore.
- float finalScore = 0.f;
- /// How the partial identifier matched filterText. [0-1]
- float filterScore = 0.f;
- /// How the symbol fits, ignoring the partial identifier.
- float symbolScore = 0.f;
-};
-
struct CompletionItem {
/// The label of this completion item. By default also the text that is
/// inserted when selecting this completion.
@@ -702,9 +688,6 @@
/// When `falsy` the label is used.
std::string sortText;
- /// Details about the quality of this completion item. (clangd extension)
- llvm::Optional scoreInfo;
-
/// A string that should be used when filtering a set of completion items.
/// When `falsy` the label is used.
std::string filterText;
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -75,15 +75,78 @@
const SymbolIndex *Index = nullptr;
};
+// Semi-structured representation of a code-complete suggestion for our C++ API.
+// We don't use the LSP structures here (unlike most features) as we want
+// to expose more data to allow for more precise testing and evaluation.
+struct CodeCompletion {
+ // The unqualified name of the symbol or other completion item.
+ std::string Name;
+ // The scope qualifier for the symbol name. e.g. "ns1::ns2::"
+ // Empty for non-symbol completions. Not inserted, but may be displayed.
+ std::string Scope;
+ // Qualifiers that must be inserted before the name (e.g. base::).
+ std::string Qualifiers;
+ // Details to be displayed following the name. Not inserted.
+ std::string Signature;
+ // Text to be inserted following the name, in snippet format.
+ std::string SnippetSuffix;
+ // Type to be displayed for this completion.
+ std::string ReturnType;
+ std::string Documentation;
+ CompletionItemKind Kind = CompletionItemKind::Missing;
+ // This completion item may represent several symbols that can be inserted in
+ // the same way, such as function overloads. In this case BundleSize > 1, and
+ // the following fields are summaries:
+ // - Signature is e.g. "(...)" for functions.
+ // - SnippetSuffix is similarly e.g. "(${0})".
+ // - ReturnType may be empty
+ // - Documentation may contain a combined docstring from all comments.
+ // Other fields should apply equally to all bundled completions.
+ unsigned BundleSize;
+ // The header through which this symbol should be included.
+ // Empty for non-symbol completions.
+ std::string Header;
+ // Present if Header is set and should be inserted to use this item.
+ llvm::Optional HeaderInsertion;
+
+ // Scores are used to rank completion items.
+ struct Scores {
+// The score that items are ranked by.
+float Total = 0.f;
+
+// The finalScore with the fuzzy name match score excluded.
+// When filtering client-side, editors should calculate the new fuzzy score,
+// whose scale is 0-1 (with 1 = prefix match, special case 2 = exact match),
+// and recompute finalScore = fuzzyScore * symbolScore.
+float ExcludingName = 0.f;
+
+// Component scores that contributed to the final score:
+
+// Quality describes how important we think this candidate is,
+// independent of the query.
+// e.g. symbols with lots of incoming references have higher quality.
+float Quality = 0.f;
+// Relevance describes how well this candidate matched the query.
+// e.g. symbols from nearby files have higher relevance.
+float Relevance = 0.f;
+ };
+ Scores Score;
+
+ // Serialize this to an LSP completion item. This is a lossy operation.
+
