Skip to content

Commit 8f3678d

Browse files
author
Eric Liu
committedJun 15, 2018
[clangd] UI for completion items that would trigger include insertion.
Summary: 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). Vim: {F6357587} vscode: {F6357589} {F6357591} Reviewers: sammccall, ilya-biryukov, hokein Reviewed By: sammccall Subscribers: MaskRay, jkorous, cfe-commits Differential Revision: https://reviews.llvm.org/D48163 llvm-svn: 334828
1 parent 98b9849 commit 8f3678d

File tree

9 files changed

+133
-137
lines changed

9 files changed

+133
-137
lines changed
 

‎clang-tools-extra/clangd/CodeComplete.cpp

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -240,10 +240,11 @@ struct CompletionCandidate {
240240
CompletionItem build(StringRef FileName, const CompletionItemScores &Scores,
241241
const CodeCompleteOptions &Opts,
242242
CodeCompletionString *SemaCCS,
243-
const IncludeInserter *Includes,
243+
const IncludeInserter &Includes,
244244
llvm::StringRef SemaDocComment) const {
245245
assert(bool(SemaResult) == bool(SemaCCS));
246246
CompletionItem I;
247+
bool InsertingInclude = false; // Whether a new #include will be added.
247248
if (SemaResult) {
248249
I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->Declaration);
249250
getLabelAndInsertText(*SemaCCS, &I.label, &I.insertText,
@@ -273,31 +274,44 @@ struct CompletionCandidate {
273274
if (I.detail.empty())
274275
I.detail = D->CompletionDetail;
275276
if (auto Inserted = headerToInsertIfNotPresent()) {
276-
auto Edit = [&]() -> Expected<Optional<TextEdit>> {
277+
auto IncludePath = [&]() -> Expected<std::string> {
277278
auto ResolvedDeclaring = toHeaderFile(
278279
IndexResult->CanonicalDeclaration.FileURI, FileName);
279280
if (!ResolvedDeclaring)
280281
return ResolvedDeclaring.takeError();
281282
auto ResolvedInserted = toHeaderFile(*Inserted, FileName);
282283
if (!ResolvedInserted)
283284
return ResolvedInserted.takeError();
284-
return Includes->insert(*ResolvedDeclaring, *ResolvedInserted);
285+
if (!Includes.shouldInsertInclude(*ResolvedDeclaring,
286+
*ResolvedInserted))
287+
return "";
288+
return Includes.calculateIncludePath(*ResolvedDeclaring,
289+
*ResolvedInserted);
285290
}();
286-
if (!Edit) {
291+
if (!IncludePath) {
287292
std::string ErrMsg =
288293
("Failed to generate include insertion edits for adding header "
289294
"(FileURI=\"" +
290295
IndexResult->CanonicalDeclaration.FileURI +
291296
"\", IncludeHeader=\"" + D->IncludeHeader + "\") into " +
292297
FileName)
293298
.str();
294-
log(ErrMsg + ":" + llvm::toString(Edit.takeError()));
295-
} else if (*Edit) {
296-
I.additionalTextEdits = {std::move(**Edit)};
299+
log(ErrMsg + ":" + llvm::toString(IncludePath.takeError()));
300+
} else if (!IncludePath->empty()) {
301+
// FIXME: consider what to show when there is no #include insertion,
302+
// and for sema results, for consistency.
303+
if (auto Edit = Includes.insert(*IncludePath)) {
304+
I.detail += ("\n" + StringRef(*IncludePath).trim('"')).str();
305+
I.additionalTextEdits = {std::move(*Edit)};
306+
InsertingInclude = true;
307+
}
297308
}
298309
}
299310
}
300311
}
312+
I.label = (InsertingInclude ? Opts.IncludeIndicator.Insert
313+
: Opts.IncludeIndicator.NoInsert) +
314+
I.label;
301315
I.scoreInfo = Scores;
302316
I.sortText = sortText(Scores.finalScore, Name);
303317
I.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
@@ -318,7 +332,13 @@ struct CompletionCandidate {
318332
llvm::StringRef Name = Bundle.front().Name;
319333
First.insertText =
320334
Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str();
321-
First.label = (Name + "(…)").str();
335+
// Keep the visual indicator of the original label.
336+
bool InsertingInclude =
337+
StringRef(First.label).startswith(Opts.IncludeIndicator.Insert);
338+
First.label = (Twine(InsertingInclude ? Opts.IncludeIndicator.Insert
339+
: Opts.IncludeIndicator.NoInsert) +
340+
Name + "(…)")
341+
.str();
322342
First.detail = llvm::formatv("[{0} overloads]", Bundle.size());
323343
return First;
324344
}
@@ -964,7 +984,9 @@ class CodeCompleteFlow {
964984
// explicitly request symbols corresponding to Sema results.
965985
// We can use their signals even if the index can't suggest them.
966986
// We must copy index results to preserve them, but there are at most Limit.
967-
auto IndexResults = queryIndex();
987+
auto IndexResults = (Opts.Index && allowIndex(Recorder->CCContext))
988+
? queryIndex()
989+
: SymbolSlab();
968990
// Merge Sema and Index results, score them, and pick the winners.
969991
auto Top = mergeResults(Recorder->Results, IndexResults);
970992
// Convert the results to the desired LSP structs.
@@ -976,8 +998,6 @@ class CodeCompleteFlow {
976998
}
977999

9781000
SymbolSlab queryIndex() {
979-
if (!Opts.Index || !allowIndex(Recorder->CCContext))
980-
return SymbolSlab();
9811001
trace::Span Tracer("Query index");
9821002
SPAN_ATTACH(Tracer, "limit", Opts.Limit);
9831003

@@ -1127,7 +1147,7 @@ class CodeCompleteFlow {
11271147
}
11281148
return CompletionCandidate::build(
11291149
Bundle,
1130-
Bundle.front().build(FileName, Scores, Opts, SemaCCS, Includes.get(),
1150+
Bundle.front().build(FileName, Scores, Opts, SemaCCS, *Includes,
11311151
FrontDocComment),
11321152
Opts);
11331153
}

‎clang-tools-extra/clangd/CodeComplete.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,13 @@ struct CodeCompleteOptions {
6060
/// If more results are available, we set CompletionList.isIncomplete.
6161
size_t Limit = 0;
6262

63+
/// A visual indicator to prepend to the completion label to indicate whether
64+
/// completion result would trigger an #include insertion or not.
65+
struct IncludeInsertionIndicator {
66+
std::string Insert = "•";
67+
std::string NoInsert = " ";
68+
} IncludeIndicator;
69+
6370
// Populated internally by clangd, do not set.
6471
/// If `Index` is set, it is used to augment the code completion
6572
/// results.

‎clang-tools-extra/clangd/Headers.cpp

Lines changed: 17 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,11 @@ collectInclusionsInMainFileCallback(const SourceManager &SM,
7272

7373
/// FIXME(ioeric): we might not want to insert an absolute include path if the
7474
/// path is not shortened.
75-
llvm::Expected<std::string> calculateIncludePath(
76-
PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
77-
const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
78-
const HeaderFile &InsertedHeader) {
75+
bool IncludeInserter::shouldInsertInclude(
76+
const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader) const {
7977
assert(DeclaringHeader.valid() && InsertedHeader.valid());
80-
if (File == DeclaringHeader.File || File == InsertedHeader.File)
81-
return "";
78+
if (FileName == DeclaringHeader.File || FileName == InsertedHeader.File)
79+
return false;
8280
llvm::StringSet<> IncludedHeaders;
8381
for (const auto &Inc : Inclusions) {
8482
IncludedHeaders.insert(Inc.Written);
@@ -88,53 +86,31 @@ llvm::Expected<std::string> calculateIncludePath(
8886
auto Included = [&](llvm::StringRef Header) {
8987
return IncludedHeaders.find(Header) != IncludedHeaders.end();
9088
};
91-
if (Included(DeclaringHeader.File) || Included(InsertedHeader.File))
92-
return "";
93-
94-
bool IsSystem = false;
89+
return !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
90+
}
9591

92+
std::string
93+
IncludeInserter::calculateIncludePath(const HeaderFile &DeclaringHeader,
94+
const HeaderFile &InsertedHeader) const {
95+
assert(DeclaringHeader.valid() && InsertedHeader.valid());
9696
if (InsertedHeader.Verbatim)
9797
return InsertedHeader.File;
98-
98+
bool IsSystem = false;
9999
std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
100100
InsertedHeader.File, BuildDir, &IsSystem);
101101
if (IsSystem)
102102
Suggested = "<" + Suggested + ">";
103103
else
104104
Suggested = "\"" + Suggested + "\"";
105-
106-
log("Suggested #include for " + InsertedHeader.File + " is: " + Suggested);
107105
return Suggested;
108106
}
109107

110-
Expected<Optional<TextEdit>>
111-
IncludeInserter::insert(const HeaderFile &DeclaringHeader,
112-
const HeaderFile &InsertedHeader) const {
113-
auto Validate = [](const HeaderFile &Header) {
114-
return Header.valid()
115-
? llvm::Error::success()
116-
: llvm::make_error<llvm::StringError>(
117-
"Invalid HeaderFile: " + Header.File +
118-
" (verbatim=" + std::to_string(Header.Verbatim) + ").",
119-
llvm::inconvertibleErrorCode());
120-
};
121-
if (auto Err = Validate(DeclaringHeader))
122-
return std::move(Err);
123-
if (auto Err = Validate(InsertedHeader))
124-
return std::move(Err);
125-
auto Include =
126-
calculateIncludePath(FileName, BuildDir, HeaderSearchInfo, Inclusions,
127-
DeclaringHeader, InsertedHeader);
128-
if (!Include)
129-
return Include.takeError();
130-
if (Include->empty())
131-
return llvm::None;
132-
StringRef IncludeRef = *Include;
133-
auto Insertion =
134-
Inserter.insert(IncludeRef.trim("\"<>"), IncludeRef.startswith("<"));
135-
if (!Insertion)
136-
return llvm::None;
137-
return replacementToEdit(Code, *Insertion);
108+
Optional<TextEdit> IncludeInserter::insert(StringRef VerbatimHeader) const {
109+
Optional<TextEdit> Edit = None;
110+
if (auto Insertion = Inserter.insert(VerbatimHeader.trim("\"<>"),
111+
VerbatimHeader.startswith("<")))
112+
Edit = replacementToEdit(Code, *Insertion);
113+
return Edit;
138114
}
139115

140116
} // namespace clangd

‎clang-tools-extra/clangd/Headers.h

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -50,25 +50,6 @@ std::unique_ptr<PPCallbacks>
5050
collectInclusionsInMainFileCallback(const SourceManager &SM,
5151
std::function<void(Inclusion)> Callback);
5252

53-
/// Determines the preferred way to #include a file, taking into account the
54-
/// search path. Usually this will prefer a shorter representation like
55-
/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
56-
///
57-
/// \param File is an absolute file path.
58-
/// \param Inclusions Existing inclusions in the main file.
59-
/// \param DeclaringHeader is the original header corresponding to \p
60-
/// InsertedHeader e.g. the header that declares a symbol.
61-
/// \param InsertedHeader The preferred header to be inserted. This could be the
62-
/// same as DeclaringHeader but must be provided.
63-
// \return A quoted "path" or <path>. This returns an empty string if:
64-
/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
65-
/// in \p Inclusions (including those included via different paths).
66-
/// - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
67-
llvm::Expected<std::string> calculateIncludePath(
68-
PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
69-
const std::vector<Inclusion> &Inclusions, const HeaderFile &DeclaringHeader,
70-
const HeaderFile &InsertedHeader);
71-
7253
// Calculates insertion edit for including a new header in a file.
7354
class IncludeInserter {
7455
public:
@@ -81,16 +62,36 @@ class IncludeInserter {
8162

8263
void addExisting(Inclusion Inc) { Inclusions.push_back(std::move(Inc)); }
8364

84-
/// Returns a TextEdit that inserts a new header; if the header is not
85-
/// inserted e.g. it's an existing header, this returns None. If any header is
86-
/// invalid, this returns error.
65+
/// Checks whether to add an #include of the header into \p File.
66+
/// An #include will not be added if:
67+
/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
68+
/// in \p Inclusions (including those included via different paths).
69+
/// - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
70+
///
71+
/// \param DeclaringHeader is the original header corresponding to \p
72+
/// InsertedHeader e.g. the header that declares a symbol.
73+
/// \param InsertedHeader The preferred header to be inserted. This could be
74+
/// the same as DeclaringHeader but must be provided. \param Inclusions
75+
/// Existing includes in the main file.
76+
bool shouldInsertInclude(const HeaderFile &DeclaringHeader,
77+
const HeaderFile &InsertedHeader) const;
78+
79+
/// Determines the preferred way to #include a file, taking into account the
80+
/// search path. Usually this will prefer a shorter representation like
81+
/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
8782
///
8883
/// \param DeclaringHeader is the original header corresponding to \p
8984
/// InsertedHeader e.g. the header that declares a symbol.
9085
/// \param InsertedHeader The preferred header to be inserted. This could be
9186
/// the same as DeclaringHeader but must be provided.
92-
Expected<Optional<TextEdit>> insert(const HeaderFile &DeclaringHeader,
93-
const HeaderFile &InsertedHeader) const;
87+
///
88+
/// \return A quoted "path" or <path> to be included.
89+
std::string calculateIncludePath(const HeaderFile &DeclaringHeader,
90+
const HeaderFile &InsertedHeader) const;
91+
92+
/// Calculates an edit that inserts \p VerbatimHeader into code. If the header
93+
/// is already included, this returns None.
94+
llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) const;
9495

9596
private:
9697
StringRef FileName;

‎clang-tools-extra/test/clangd/completion-snippets.test

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
# CHECK-NEXT: "insertText": "func_with_args(${1:int a}, ${2:int b})",
3333
# CHECK-NEXT: "insertTextFormat": 2,
3434
# CHECK-NEXT: "kind": 3,
35-
# CHECK-NEXT: "label": "func_with_args(int a, int b)",
35+
# CHECK-NEXT: "label": " func_with_args(int a, int b)",
3636
# CHECK-NEXT: "sortText": "{{.*}}func_with_args"
3737
# CHECK-NEXT: }
3838
# CHECK-NEXT: ]

‎clang-tools-extra/test/clangd/completion.test

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
# CHECK-NEXT: "insertText": "a",
1717
# CHECK-NEXT: "insertTextFormat": 1,
1818
# CHECK-NEXT: "kind": 5,
19-
# CHECK-NEXT: "label": "a",
19+
# CHECK-NEXT: "label": " a",
2020
# CHECK-NEXT: "sortText": "{{.*}}a"
2121
# CHECK-NEXT: }
2222
# CHECK-NEXT: ]
@@ -36,7 +36,7 @@
3636
# CHECK-NEXT: "insertText": "b",
3737
# CHECK-NEXT: "insertTextFormat": 1,
3838
# CHECK-NEXT: "kind": 5,
39-
# CHECK-NEXT: "label": "b",
39+
# CHECK-NEXT: "label": " b",
4040
# CHECK-NEXT: "sortText": "{{.*}}b"
4141
# CHECK-NEXT: }
4242
# CHECK-NEXT: ]

‎clang-tools-extra/test/clangd/protocol.test

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ Content-Length: 146
3838
# CHECK-NEXT: "insertText": "a",
3939
# CHECK-NEXT: "insertTextFormat": 1,
4040
# CHECK-NEXT: "kind": 5,
41-
# CHECK-NEXT: "label": "a",
41+
# CHECK-NEXT: "label": " a",
4242
# CHECK-NEXT: "sortText": "{{.*}}"
4343
# CHECK: ]
4444
# CHECK-NEXT: }
@@ -67,7 +67,7 @@ Content-Length: 146
6767
# CHECK-NEXT: "insertText": "a",
6868
# CHECK-NEXT: "insertTextFormat": 1,
6969
# CHECK-NEXT: "kind": 5,
70-
# CHECK-NEXT: "label": "a",
70+
# CHECK-NEXT: "label": " a",
7171
# CHECK-NEXT: "sortText": "{{.*}}"
7272
# CHECK: ]
7373
# CHECK-NEXT: }
@@ -96,7 +96,7 @@ Content-Length: 146
9696
# CHECK-NEXT: "insertText": "a",
9797
# CHECK-NEXT: "insertTextFormat": 1,
9898
# CHECK-NEXT: "kind": 5,
99-
# CHECK-NEXT: "label": "a",
99+
# CHECK-NEXT: "label": " a",
100100
# CHECK-NEXT: "sortText": "{{.*}}"
101101
# CHECK: ]
102102
# CHECK-NEXT: }

0 commit comments

Comments
 (0)
Please sign in to comment.