Index: clangd/CodeComplete.h =================================================================== --- clangd/CodeComplete.h +++ clangd/CodeComplete.h @@ -60,6 +60,13 @@ /// If more results are available, we set CompletionList.isIncomplete. size_t Limit = 0; + /// A visual indicator to prepend to the completion label to indicate whether + /// completion result would trigger an #include insertion or not. + struct IncludeInsertionIndicator { + std::string Insert = "•"; + std::string NoInsert = " "; + } IncludeIndicator; + // Populated internally by clangd, do not set. /// If `Index` is set, it is used to augment the code completion /// results. Index: clangd/CodeComplete.cpp =================================================================== --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -240,10 +240,11 @@ CompletionItem build(StringRef FileName, const CompletionItemScores &Scores, const CodeCompleteOptions &Opts, CodeCompletionString *SemaCCS, - const IncludeInserter *Includes, + const IncludeInserter &Includes, llvm::StringRef SemaDocComment) const { assert(bool(SemaResult) == bool(SemaCCS)); CompletionItem I; + 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, @@ -273,7 +274,7 @@ if (I.detail.empty()) I.detail = D->CompletionDetail; if (auto Inserted = headerToInsertIfNotPresent()) { - auto Edit = [&]() -> Expected> { + auto IncludePath = [&]() -> Expected { auto ResolvedDeclaring = toHeaderFile( IndexResult->CanonicalDeclaration.FileURI, FileName); if (!ResolvedDeclaring) @@ -281,9 +282,13 @@ auto ResolvedInserted = toHeaderFile(*Inserted, FileName); if (!ResolvedInserted) return ResolvedInserted.takeError(); - return Includes->insert(*ResolvedDeclaring, *ResolvedInserted); + if (!Includes.shouldInsertInclude(*ResolvedDeclaring, + *ResolvedInserted)) + return ""; + return Includes.calculateIncludePath(*ResolvedDeclaring, + *ResolvedInserted); }(); - if (!Edit) { + if (!IncludePath) { std::string ErrMsg = ("Failed to generate include insertion edits for adding header " "(FileURI=\"" + @@ -291,13 +296,22 @@ "\", IncludeHeader=\"" + D->IncludeHeader + "\") into " + FileName) .str(); - log(ErrMsg + ":" + llvm::toString(Edit.takeError())); - } else if (*Edit) { - I.additionalTextEdits = {std::move(**Edit)}; + log(ErrMsg + ":" + llvm::toString(IncludePath.takeError())); + } else if (!IncludePath->empty()) { + // FIXME: consider what to show when there is no #include insertion, + // and for sema results, for consistency. + if (auto Edit = Includes.insert(*IncludePath)) { + I.detail += ("\n" + StringRef(*IncludePath).trim('"')).str(); + I.additionalTextEdits = {std::move(*Edit)}; + InsertingInclude = true; + } } } } } + I.label = (InsertingInclude ? Opts.IncludeIndicator.Insert + : Opts.IncludeIndicator.NoInsert) + + I.label; I.scoreInfo = Scores; I.sortText = sortText(Scores.finalScore, Name); I.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet @@ -318,7 +332,13 @@ llvm::StringRef Name = Bundle.front().Name; First.insertText = Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str(); - First.label = (Name + "(…)").str(); + // Keep the visual indicator of the original label. + bool InsertingInclude = + StringRef(First.label).startswith(Opts.IncludeIndicator.Insert); + First.label = (Twine(InsertingInclude ? Opts.IncludeIndicator.Insert + : Opts.IncludeIndicator.NoInsert) + + Name + "(…)") + .str(); First.detail = llvm::formatv("[{0} overloads]", Bundle.size()); return First; } @@ -964,7 +984,9 @@ // 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 = (Opts.Index && allowIndex(Recorder->CCContext)) + ? 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. @@ -976,8 +998,6 @@ } SymbolSlab queryIndex() { - if (!Opts.Index || !allowIndex(Recorder->CCContext)) - return SymbolSlab(); trace::Span Tracer("Query index"); SPAN_ATTACH(Tracer, "limit", Opts.Limit); @@ -1127,7 +1147,7 @@ } return CompletionCandidate::build( Bundle, - Bundle.front().build(FileName, Scores, Opts, SemaCCS, Includes.get(), + Bundle.front().build(FileName, Scores, Opts, SemaCCS, *Includes, FrontDocComment), Opts); } Index: clangd/Headers.h =================================================================== --- clangd/Headers.h +++ clangd/Headers.h @@ -50,25 +50,6 @@ collectInclusionsInMainFileCallback(const SourceManager &SM, std::function Callback); -/// Determines the preferred way to #include a file, taking into account the -/// search path. Usually this will prefer a shorter representation like -/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'. -/// -/// \param File is an absolute file path. -/// \param Inclusions Existing inclusions in the main file. -/// \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. -// \return A quoted "path" or . This returns an empty string 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 calculateIncludePath( - PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo, - const std::vector &Inclusions, const HeaderFile &DeclaringHeader, - const HeaderFile &InsertedHeader); - // Calculates insertion edit for including a new header in a file. class IncludeInserter { public: @@ -81,16 +62,36 @@ 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. + /// Checks whether to add an #include of the header into \p File. + /// An #include will not be added 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. + /// + /// \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. \param Inclusions + /// Existing includes in the main file. + bool shouldInsertInclude(const HeaderFile &DeclaringHeader, + const HeaderFile &InsertedHeader) const; + + /// Determines the preferred way to #include a file, taking into account the + /// search path. Usually this will prefer a shorter representation like + /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'. /// /// \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> insert(const HeaderFile &DeclaringHeader, - const HeaderFile &InsertedHeader) const; + /// + /// \return A quoted "path" or to be included. + std::string calculateIncludePath(const HeaderFile &DeclaringHeader, + const HeaderFile &InsertedHeader) const; + + /// Calculates an edit that inserts \p VerbatimHeader into code. If the header + /// is already included, this returns None. + llvm::Optional insert(llvm::StringRef VerbatimHeader) const; private: StringRef FileName; Index: clangd/Headers.cpp =================================================================== --- clangd/Headers.cpp +++ clangd/Headers.cpp @@ -72,13 +72,11 @@ /// FIXME(ioeric): we might not want to insert an absolute include path if the /// path is not shortened. -llvm::Expected calculateIncludePath( - PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo, - const std::vector &Inclusions, const HeaderFile &DeclaringHeader, - const HeaderFile &InsertedHeader) { +bool IncludeInserter::shouldInsertInclude( + const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader) const { assert(DeclaringHeader.valid() && InsertedHeader.valid()); - if (File == DeclaringHeader.File || File == InsertedHeader.File) - return ""; + if (FileName == DeclaringHeader.File || FileName == InsertedHeader.File) + return false; llvm::StringSet<> IncludedHeaders; for (const auto &Inc : Inclusions) { IncludedHeaders.insert(Inc.Written); @@ -88,53 +86,31 @@ auto Included = [&](llvm::StringRef Header) { return IncludedHeaders.find(Header) != IncludedHeaders.end(); }; - if (Included(DeclaringHeader.File) || Included(InsertedHeader.File)) - return ""; - - bool IsSystem = false; + return !Included(DeclaringHeader.File) && !Included(InsertedHeader.File); +} +std::string +IncludeInserter::calculateIncludePath(const HeaderFile &DeclaringHeader, + const HeaderFile &InsertedHeader) const { + assert(DeclaringHeader.valid() && InsertedHeader.valid()); if (InsertedHeader.Verbatim) return InsertedHeader.File; - + bool IsSystem = false; 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; } -Expected> -IncludeInserter::insert(const HeaderFile &DeclaringHeader, - const HeaderFile &InsertedHeader) const { - auto Validate = [](const HeaderFile &Header) { - return Header.valid() - ? llvm::Error::success() - : llvm::make_error( - "Invalid HeaderFile: " + Header.File + - " (verbatim=" + std::to_string(Header.Verbatim) + ").", - llvm::inconvertibleErrorCode()); - }; - if (auto Err = Validate(DeclaringHeader)) - return std::move(Err); - if (auto Err = Validate(InsertedHeader)) - return std::move(Err); - auto Include = - calculateIncludePath(FileName, BuildDir, HeaderSearchInfo, Inclusions, - 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); +Optional IncludeInserter::insert(StringRef VerbatimHeader) const { + Optional Edit = None; + if (auto Insertion = Inserter.insert(VerbatimHeader.trim("\"<>"), + VerbatimHeader.startswith("<"))) + Edit = replacementToEdit(Code, *Insertion); + return Edit; } } // namespace clangd 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: test/clangd/completion.test =================================================================== --- test/clangd/completion.test +++ test/clangd/completion.test @@ -16,7 +16,7 @@ # CHECK-NEXT: "insertText": "a", # CHECK-NEXT: "insertTextFormat": 1, # CHECK-NEXT: "kind": 5, -# CHECK-NEXT: "label": "a", +# CHECK-NEXT: "label": " a", # CHECK-NEXT: "sortText": "{{.*}}a" # CHECK-NEXT: } # CHECK-NEXT: ] @@ -36,7 +36,7 @@ # CHECK-NEXT: "insertText": "b", # CHECK-NEXT: "insertTextFormat": 1, # CHECK-NEXT: "kind": 5, -# CHECK-NEXT: "label": "b", +# CHECK-NEXT: "label": " b", # CHECK-NEXT: "sortText": "{{.*}}b" # CHECK-NEXT: } # CHECK-NEXT: ] Index: test/clangd/protocol.test =================================================================== --- test/clangd/protocol.test +++ test/clangd/protocol.test @@ -38,7 +38,7 @@ # CHECK-NEXT: "insertText": "a", # CHECK-NEXT: "insertTextFormat": 1, # CHECK-NEXT: "kind": 5, -# CHECK-NEXT: "label": "a", +# CHECK-NEXT: "label": " a", # CHECK-NEXT: "sortText": "{{.*}}" # CHECK: ] # CHECK-NEXT: } @@ -67,7 +67,7 @@ # CHECK-NEXT: "insertText": "a", # CHECK-NEXT: "insertTextFormat": 1, # CHECK-NEXT: "kind": 5, -# CHECK-NEXT: "label": "a", +# CHECK-NEXT: "label": " a", # CHECK-NEXT: "sortText": "{{.*}}" # CHECK: ] # CHECK-NEXT: } @@ -96,7 +96,7 @@ # CHECK-NEXT: "insertText": "a", # CHECK-NEXT: "insertTextFormat": 1, # CHECK-NEXT: "kind": 5, -# CHECK-NEXT: "label": "a", +# CHECK-NEXT: "label": " a", # CHECK-NEXT: "sortText": "{{.*}}" # CHECK: ] # CHECK-NEXT: } Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -44,7 +44,19 @@ // GMock helpers for matching completion items. MATCHER_P(Named, Name, "") { return arg.insertText == Name; } -MATCHER_P(Labeled, Label, "") { return arg.label == Label; } +MATCHER_P(Labeled, Label, "") { + std::string Indented; + if (!StringRef(Label).startswith( + CodeCompleteOptions().IncludeIndicator.Insert) && + !StringRef(Label).startswith( + CodeCompleteOptions().IncludeIndicator.NoInsert)) + Indented = + (Twine(CodeCompleteOptions().IncludeIndicator.NoInsert) + Label).str(); + else + Indented = Label; + return arg.label == Indented; +} +MATCHER_P(SigHelpLabeled, Label, "") { return arg.label == Label; } MATCHER_P(Kind, K, "") { return arg.kind == K; } MATCHER_P(Filter, F, "") { return arg.filterText == F; } MATCHER_P(Doc, D, "") { return arg.documentation == D; } @@ -563,7 +575,10 @@ )cpp", {Sym}); EXPECT_THAT(Results.items, - ElementsAre(AllOf(Named("X"), InsertInclude("\"bar.h\"")))); + ElementsAre(AllOf( + Named("X"), + Labeled(CodeCompleteOptions().IncludeIndicator.Insert + "X"), + InsertInclude("\"bar.h\"")))); // Duplicate based on inclusions in preamble. Results = completions(Server, R"cpp( @@ -571,8 +586,8 @@ 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) { @@ -830,7 +845,7 @@ Matcher Sig(std::string Label, std::vector Params) { - return AllOf(Labeled(Label), ParamsAre(Params)); + return AllOf(SigHelpLabeled(Label), ParamsAre(Params)); } TEST(SignatureHelpTest, Overloads) { @@ -1095,12 +1110,16 @@ NoArgsGFunc.Detail = &Detail; EXPECT_THAT( completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items, - UnorderedElementsAre(AllOf(Named("GFuncC"), InsertInclude("")), - Labeled("GFuncC(int)"), Labeled("GFuncD(int)"))); + UnorderedElementsAre( + AllOf( + Named("GFuncC"), + Labeled(CodeCompleteOptions().IncludeIndicator.Insert + "GFuncC"), + InsertInclude("")), + Labeled("GFuncC(int)"), Labeled("GFuncD(int)"))); // Examine a bundled completion in detail. auto A = completions(Context + "int y = X().a^", {}, Opts).items.front(); - EXPECT_EQ(A.label, "a(…)"); + EXPECT_EQ(A.label, " a(…)"); EXPECT_EQ(A.detail, "[2 overloads]"); EXPECT_EQ(A.insertText, "a"); EXPECT_EQ(A.kind, CompletionItemKind::Method); Index: unittests/clangd/HeadersTests.cpp =================================================================== --- unittests/clangd/HeadersTests.cpp +++ unittests/clangd/HeadersTests.cpp @@ -78,10 +78,10 @@ return Inclusions; } - // Calculates the include path, or returns "" on error. + // Calculates the include path, or returns "" on error or header should not be + // inserted. std::string calculate(PathRef Original, PathRef Preferred = "", - const std::vector &Inclusions = {}, - bool ExpectError = false) { + const std::vector &Inclusions = {}) { auto Clang = setupClang(); PreprocessOnlyAction Action; EXPECT_TRUE( @@ -94,24 +94,21 @@ /*Verbatim=*/!llvm::sys::path::is_absolute(Header)}; }; - auto Path = calculateIncludePath( - MainFile, CDB.getCompileCommand(MainFile)->Directory, - Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions, - ToHeaderFile(Original), ToHeaderFile(Preferred)); + IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(), + CDB.getCompileCommand(MainFile)->Directory, + Clang->getPreprocessor().getHeaderSearchInfo()); + for (const auto &Inc : Inclusions) + Inserter.addExisting(Inc); + auto Declaring = ToHeaderFile(Original); + auto Inserted = ToHeaderFile(Preferred); + if (!Inserter.shouldInsertInclude(Declaring, Inserted)) + return ""; + std::string Path = Inserter.calculateIncludePath(Declaring, Inserted); Action.EndSourceFile(); - if (!Path) { - llvm::consumeError(Path.takeError()); - EXPECT_TRUE(ExpectError); - return std::string(); - } else { - EXPECT_FALSE(ExpectError); - } - return std::move(*Path); + return Path; } - Expected> - insert(const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader, - const std::vector &ExistingInclusions = {}) { + Optional insert(StringRef VerbatimHeader) { auto Clang = setupClang(); PreprocessOnlyAction Action; EXPECT_TRUE( @@ -120,10 +117,7 @@ IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(), CDB.getCompileCommand(MainFile)->Directory, Clang->getPreprocessor().getHeaderSearchInfo()); - for (const auto &Inc : ExistingInclusions) - Inserter.addExisting(Inc); - - auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader); + auto Edit = Inserter.insert(VerbatimHeader); Action.EndSourceFile(); return Edit; } @@ -220,31 +214,10 @@ EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), ""); } -HeaderFile literal(StringRef Include) { - HeaderFile H{Include, /*Verbatim=*/true}; - assert(H.valid()); - return H; -} - TEST_F(HeadersTest, PreferInserted) { - auto Edit = insert(literal(""), literal("")); - EXPECT_TRUE(static_cast(Edit)); - EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains("")); -} - -TEST_F(HeadersTest, ExistingInclusion) { - Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""}; - auto Edit = insert(literal(""), literal(""), {Existing}); - EXPECT_TRUE(static_cast(Edit)); - EXPECT_FALSE(static_cast(*Edit)); -} - -TEST_F(HeadersTest, ValidateHeaders) { - HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true}; - assert(!InvalidHeader.valid()); - auto Edit = insert(InvalidHeader, literal("\"c.h\"")); - EXPECT_FALSE(static_cast(Edit)); - llvm::consumeError(Edit.takeError()); + auto Edit = insert(""); + EXPECT_TRUE(Edit.hasValue()); + EXPECT_TRUE(llvm::StringRef(Edit->newText).contains("")); } } // namespace