ioeric created this revision.
ioeric added reviewers: sammccall, ilya-biryukov, hokein.
Herald added subscribers: cfe-commits, jkorous, MaskRay.
For completion items that would trigger include insertions (i.e. index symbols
that are not #included yet), add a visual indicator "+" before the completion
label. The inserted headers will appear in the completion detail.
Open to suggestions for better visual indicators; "+" was picked because it
seems cleaner than a few other candidates I've tried (*, #, @ ...).
The displayed header would be like a/b/c.h (without quote) or <vector> for
system
headers. I didn't add quotation or "#include" because they can take up limited
space and do not provide additional information after users know what the
headers are. I think a header alone should be obvious for users to infer that
this is an include header..
To align indentation, also prepend ' ' to labels of candidates that would not
trigger include insertions (only for completions where index results are
possible).
(Placeholder: add screenshots)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===================================================================
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -106,7 +106,7 @@
} else {
EXPECT_FALSE(ExpectError);
}
- return std::move(*Path);
+ return Path->second ? std::move(Path->first) : "";
}
Expected<Optional<TextEdit>>
@@ -123,9 +123,13 @@
for (const auto &Inc : ExistingInclusions)
Inserter.addExisting(Inc);
- auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+ auto HeaderAndEdit = Inserter.insert(DeclaringHeader, InsertedHeader);
Action.EndSourceFile();
- return Edit;
+ if (!HeaderAndEdit)
+ return HeaderAndEdit.takeError();
+ if (!HeaderAndEdit->second)
+ return llvm::None;
+ return *(HeaderAndEdit->second);
}
MockFSProvider FS;
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -566,17 +566,17 @@
int main() { ns::^ }
)cpp",
{Sym});
- EXPECT_THAT(Results.items,
- ElementsAre(AllOf(Named("X"), InsertInclude("\"bar.h\""))));
+ EXPECT_THAT(Results.items, ElementsAre(AllOf(Named("X"), Labeled("+X"),
+ InsertInclude("\"bar.h\""))));
// Duplicate based on inclusions in preamble.
Results = completions(Server,
R"cpp(
#include "sub/bar.h" // not shortest, so should only match resolved.
int main() { ns::^ }
)cpp",
{Sym});
- EXPECT_THAT(Results.items,
- ElementsAre(AllOf(Named("X"), Not(HasAdditionalEdits()))));
+ EXPECT_THAT(Results.items, ElementsAre(AllOf(Named("X"), Labeled(" X"),
+ Not(HasAdditionalEdits()))));
}
TEST(CompletionTest, NoIncludeInsertionWhenDeclFoundInFile) {
Index: test/clangd/completion-snippets.test
===================================================================
--- test/clangd/completion-snippets.test
+++ test/clangd/completion-snippets.test
@@ -32,7 +32,7 @@
# CHECK-NEXT: "insertText": "func_with_args(${1:int a}, ${2:int b})",
# CHECK-NEXT: "insertTextFormat": 2,
# CHECK-NEXT: "kind": 3,
-# CHECK-NEXT: "label": "func_with_args(int a, int b)",
+# CHECK-NEXT: "label": " func_with_args(int a, int b)",
# CHECK-NEXT: "sortText": "{{.*}}func_with_args"
# CHECK-NEXT: }
# CHECK-NEXT: ]
Index: clangd/Headers.h
===================================================================
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -60,11 +60,12 @@
/// InsertedHeader e.g. the header that declares a symbol.
/// \param InsertedHeader The preferred header to be inserted. This could be the
/// same as DeclaringHeader but must be provided.
-// \return A quoted "path" or <path>. This returns an empty string if:
+/// \return A quoted "path" or <path> to be included and a boolean that is true
+/// if the include can be added, and false if:
/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
/// in \p Inclusions (including those included via different paths).
/// - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
-llvm::Expected<std::string> calculateIncludePath(
+llvm::Expected<std::pair<std::string, bool>> calculateIncludePath(
PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
const HeaderFile &InsertedHeader);
@@ -81,16 +82,18 @@
void addExisting(Inclusion Inc) { Inclusions.push_back(std::move(Inc)); }
- /// Returns a TextEdit that inserts a new header; if the header is not
- /// inserted e.g. it's an existing header, this returns None. If any header is
- /// invalid, this returns error.
+ /// Returns a <Header, Edit> pair where Header is verbatim
+ /// (e.g. <vector>) and Edit, if not None, inserts the entire include
+ /// directive. Edit is None is the header is not suitable to include e.g.
+ /// it's already included. If any header is invalid, this returns error.
///
/// \param DeclaringHeader is the original header corresponding to \p
/// InsertedHeader e.g. the header that declares a symbol.
/// \param InsertedHeader The preferred header to be inserted. This could be
/// the same as DeclaringHeader but must be provided.
- Expected<Optional<TextEdit>> insert(const HeaderFile &DeclaringHeader,
- const HeaderFile &InsertedHeader) const;
+ Expected<std::pair<std::string, Optional<TextEdit>>>
+ insert(const HeaderFile &DeclaringHeader,
+ const HeaderFile &InsertedHeader) const;
private:
StringRef FileName;
Index: clangd/Headers.cpp
===================================================================
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -72,13 +72,13 @@
/// FIXME(ioeric): we might not want to insert an absolute include path if the
/// path is not shortened.
-llvm::Expected<std::string> calculateIncludePath(
+llvm::Expected<std::pair<std::string, bool>> calculateIncludePath(
PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
const HeaderFile &InsertedHeader) {
assert(DeclaringHeader.valid() && InsertedHeader.valid());
- if (File == DeclaringHeader.File || File == InsertedHeader.File)
- return "";
+ bool AddInclude =
+ File != DeclaringHeader.File && File != InsertedHeader.File;
llvm::StringSet<> IncludedHeaders;
for (const auto &Inc : Inclusions) {
IncludedHeaders.insert(Inc.Written);
@@ -88,26 +88,24 @@
auto Included = [&](llvm::StringRef Header) {
return IncludedHeaders.find(Header) != IncludedHeaders.end();
};
- if (Included(DeclaringHeader.File) || Included(InsertedHeader.File))
- return "";
+ AddInclude &=
+ !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
bool IsSystem = false;
if (InsertedHeader.Verbatim)
- return InsertedHeader.File;
+ return std::make_pair(InsertedHeader.File, AddInclude);
std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
InsertedHeader.File, BuildDir, &IsSystem);
if (IsSystem)
Suggested = "<" + Suggested + ">";
else
Suggested = "\"" + Suggested + "\"";
-
- log("Suggested #include for " + InsertedHeader.File + " is: " + Suggested);
- return Suggested;
+ return std::make_pair(Suggested, AddInclude);
}
-Expected<Optional<TextEdit>>
+Expected<std::pair<std::string, Optional<TextEdit>>>
IncludeInserter::insert(const HeaderFile &DeclaringHeader,
const HeaderFile &InsertedHeader) const {
auto Validate = [](const HeaderFile &Header) {
@@ -127,14 +125,13 @@
DeclaringHeader, InsertedHeader);
if (!Include)
return Include.takeError();
- if (Include->empty())
- return llvm::None;
- StringRef IncludeRef = *Include;
- auto Insertion =
- Inserter.insert(IncludeRef.trim("\"<>"), IncludeRef.startswith("<"));
- if (!Insertion)
- return llvm::None;
- return replacementToEdit(Code, *Insertion);
+ StringRef IncludeRef = Include->first;
+ Optional<TextEdit> Edit = None;
+ if (Include->second)
+ if (auto Insertion = Inserter.insert(IncludeRef.trim("\"<>"),
+ IncludeRef.startswith("<")))
+ Edit = replacementToEdit(Code, *Insertion);
+ return std::make_pair(IncludeRef, std::move(Edit));
}
} // namespace clangd
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -183,16 +183,22 @@
// We may have a result from Sema, from the index, or both.
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ // Whether or not this candidate is in a completion where index is allowed.
+ // This can affect how items are built (e.g. indent label to align with visual
+ // indicator in index results).
+ bool AllowIndexCompletion = false;
// Builds an LSP completion item.
CompletionItem build(StringRef FileName, const CompletionItemScores &Scores,
const CodeCompleteOptions &Opts,
CodeCompletionString *SemaCCS,
const IncludeInserter *Includes,
llvm::StringRef SemaDocComment) const {
assert(bool(SemaResult) == bool(SemaCCS));
+ assert((AllowIndexCompletion || !IndexResult));
CompletionItem I;
bool ShouldInsertInclude = true;
+ bool InsertingInclude = false; // Whether a new #include will be added.
if (SemaResult) {
I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->Declaration);
getLabelAndInsertText(*SemaCCS, &I.label, &I.insertText,
@@ -236,7 +242,8 @@
if (I.detail.empty())
I.detail = D->CompletionDetail;
if (ShouldInsertInclude && Includes && !D->IncludeHeader.empty()) {
- auto Edit = [&]() -> Expected<Optional<TextEdit>> {
+ auto HeaderAndEdit =
+ [&]() -> Expected<std::pair<std::string, Optional<TextEdit>>> {
auto ResolvedDeclaring = toHeaderFile(
IndexResult->CanonicalDeclaration.FileURI, FileName);
if (!ResolvedDeclaring)
@@ -246,21 +253,29 @@
return ResolvedInserted.takeError();
return Includes->insert(*ResolvedDeclaring, *ResolvedInserted);
}();
- if (!Edit) {
+ if (!HeaderAndEdit) {
std::string ErrMsg =
("Failed to generate include insertion edits for adding header "
"(FileURI=\"" +
IndexResult->CanonicalDeclaration.FileURI +
"\", IncludeHeader=\"" + D->IncludeHeader + "\") into " +
FileName)
.str();
- log(ErrMsg + ":" + llvm::toString(Edit.takeError()));
- } else if (*Edit) {
- I.additionalTextEdits = {std::move(**Edit)};
+ log(ErrMsg + ":" + llvm::toString(HeaderAndEdit.takeError()));
+ } else {
+ InsertingInclude = static_cast<bool>(HeaderAndEdit->second);
+ if (InsertingInclude) {
+ I.detail += ((I.detail.empty() ? "" : "\n") +
+ StringRef(HeaderAndEdit->first).trim('"'))
+ .str();
+ I.additionalTextEdits = {std::move(*(HeaderAndEdit->second))};
+ }
}
}
}
}
+ if (AllowIndexCompletion)
+ I.label = (InsertingInclude ? "+" : " ") + I.label;
I.scoreInfo = Scores;
I.sortText = sortText(Scores.finalScore, Name);
I.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
@@ -848,6 +863,7 @@
bool Incomplete = false; // Would more be available with a higher limit?
llvm::Optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
std::unique_ptr<IncludeInserter> Includes; // Initialized once compiler runs.
+ bool AllowIndexCompletion = false; // Initialized once Sema runs.
public:
// A CodeCompleteFlow object is only useful for calling run() exactly once.
@@ -897,12 +913,13 @@
CompletionList runWithSema() {
Filter = FuzzyMatcher(
Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
+ AllowIndexCompletion = Opts.Index && allowIndex(Recorder->CCContext);
// Sema provides the needed context to query the index.
// FIXME: in addition to querying for extra/overlapping symbols, we should
// explicitly request symbols corresponding to Sema results.
// We can use their signals even if the index can't suggest them.
// We must copy index results to preserve them, but there are at most Limit.
- auto IndexResults = queryIndex();
+ auto IndexResults = AllowIndexCompletion ? queryIndex() : SymbolSlab();
// Merge Sema and Index results, score them, and pick the winners.
auto Top = mergeResults(Recorder->Results, IndexResults);
// Convert the results to the desired LSP structs.
@@ -914,8 +931,6 @@
}
SymbolSlab queryIndex() {
- if (!Opts.Index || !allowIndex(Recorder->CCContext))
- return SymbolSlab();
trace::Span Tracer("Query index");
SPAN_ATTACH(Tracer, "limit", Opts.Limit);
@@ -986,6 +1001,7 @@
const CodeCompletionResult *SemaResult,
const Symbol *IndexResult) {
CompletionCandidate C;
+ C.AllowIndexCompletion = AllowIndexCompletion;
C.SemaResult = SemaResult;
C.IndexResult = IndexResult;
C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
