[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
This revision was automatically updated to reflect the committed changes. Closed by commit rL340005: [clangd] Fetch documentation from the Index during signature help (authored by ibiryukov, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50727?vs=160992&id=161189#toc Repository: rL LLVM https://reviews.llvm.org/D50727 Files: clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/clangd/CodeComplete.h clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp === --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp @@ -826,11 +826,19 @@ EXPECT_TRUE(Results.Completions.empty()); } -SignatureHelp signatures(StringRef Text) { +SignatureHelp signatures(StringRef Text, + std::vector IndexSymbols = {}) { + std::unique_ptr Index; + if (!IndexSymbols.empty()) +Index = memIndex(IndexSymbols); + MockFSProvider FS; MockCompilationDatabase CDB; IgnoreDiagnostics DiagConsumer; - ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + ClangdServer::Options Opts = ClangdServer::optsForTest(); + Opts.StaticIndex = Index.get(); + + ClangdServer Server(CDB, FS, DiagConsumer, Opts); auto File = testPath("foo.cpp"); Annotations Test(Text); runAddDocument(Server, File, Test.code()); @@ -845,6 +853,7 @@ return false; return true; } +MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; } Matcher Sig(std::string Label, std::vector Params) { @@ -1594,6 +1603,51 @@ ElementsAre(Sig("foo(T, U) -> void", {"T", "U"}))); } +TEST(SignatureHelpTest, IndexDocumentation) { + Symbol::Details DocDetails; + DocDetails.Documentation = "Doc from the index"; + + Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#"); + Foo0.Detail = &DocDetails; + Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#"); + Foo1.Detail = &DocDetails; + Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#"); + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +int foo(double); + +void test() { + foo(^); +} + )cpp", + {Foo0}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(double) -> int", {"double"}), SigDoc(""; + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +// Overriden doc from sema +int foo(int); +// Doc from sema +int foo(int, int); + +void test() { + foo(^); +} + )cpp", + {Foo0, Foo1, Foo2}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(int) -> int", {"int"}), +SigDoc("Overriden doc from sema")), + AllOf(Sig("foo(int, int) -> int", {"int", "int"}), +SigDoc("Doc from sema"; +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/trunk/clangd/CodeComplete.h === --- clang-tools-extra/trunk/clangd/CodeComplete.h +++ clang-tools-extra/trunk/clangd/CodeComplete.h @@ -172,12 +172,11 @@ CodeCompleteOptions Opts); /// Get signature help at a specified \p Pos in \p FileName. -SignatureHelp signatureHelp(PathRef FileName, -const tooling::CompileCommand &Command, -PrecompiledPreamble const *Preamble, -StringRef Contents, Position Pos, -IntrusiveRefCntPtr VFS, -std::shared_ptr PCHs); +SignatureHelp +signatureHelp(PathRef FileName, const tooling::CompileCommand &Command, + PrecompiledPreamble const *Preamble, StringRef Contents, + Position Pos, IntrusiveRefCntPtr VFS, + std::shared_ptr PCHs, SymbolIndex *Index); // For index-based completion, we only consider: // * symbols in namespaces or translation unit scopes (e.g. no class Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp === --- clang-tools-extra/trunk/clangd/ClangdServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp @@ -177,15 +177,16 @@ auto PCHs = this->PCHs; auto FS = FSProvider.getFileSystem(); - auto Action = [Pos, FS, PCHs](Path File, Callback CB, -llvm::Expected IP) { + auto *Index = this->Index; + auto Action = [Pos, FS, PCHs, Index](Path File, Callback CB, +
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ilya-biryukov wrote: > ioeric wrote: > > hokein wrote: > > > ioeric wrote: > > > > ilya-biryukov wrote: > > > > > ioeric wrote: > > > > > > drive by: I think this should be `vlog` or `dlog`. > > > > > Code completion also logs the number of results from sema, index, > > > > > etc. using the `log()` call. > > > > > The added log message looks similar, so trying to be consistent with > > > > > the rest of the code in this file. > > > > > > > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd > > > > > rather leave it to a separate patch. > > > > I'm not sure which level code completion log messages should be in, but > > > > I think we should follow the guidelines in the logger documentation > > > > instead of the existing uses. > > > > > > > > Quote from Logger.h > > > > ``` > > > > // log() is used for information important to understanding a clangd > > > > session. > > > > // e.g. the names of LSP messages sent are logged at this level. > > > > // This level could be enabled in production builds to allow later > > > > inspection. > > > > > > > > // vlog() is used for details often needed for debugging clangd > > > > sessions. > > > > // This level would typically be enabled for clangd developers. > > > > ``` > > > My recent experience of debugging some weird GotoDef issues tells me that > > > log of index should be on production (since it is a non-trivial part in a > > > clangd session), it would be really helpful to understand what is going > > > on. > > I agree that they are useful for debugging, but they might not be > > interesting to end-users. And I think that's why there is `vlog`. Clangd > > developers could use a different log level with `--log` flag. > I agree that `vlog` is probably a better fit here, but still think we should > change it across `CodeComplete.cpp` in a single commit, rather than partially > apply the guidelines to different parts of the file. > > However, if everyone agrees we should use `vlog` here, happy to use `vlog` > too and also send a patch to update all the rest of the usages. > The situation I'm trying to avoid is: > 1. We commit `vlog` here. > 2. Someone argues that using `log` is actually better and we decide to not > change other usages to `log`. > 3. We end up with inconsistent choices across this file: `vlog` for signature > help and `log` for code completion. > > But if there's an agreement that we should use `vlog` everywhere, more than > happy to change it :-) That sounds good to me. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > hokein wrote: > > ioeric wrote: > > > ilya-biryukov wrote: > > > > ioeric wrote: > > > > > drive by: I think this should be `vlog` or `dlog`. > > > > Code completion also logs the number of results from sema, index, etc. > > > > using the `log()` call. > > > > The added log message looks similar, so trying to be consistent with > > > > the rest of the code in this file. > > > > > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather > > > > leave it to a separate patch. > > > I'm not sure which level code completion log messages should be in, but I > > > think we should follow the guidelines in the logger documentation instead > > > of the existing uses. > > > > > > Quote from Logger.h > > > ``` > > > // log() is used for information important to understanding a clangd > > > session. > > > // e.g. the names of LSP messages sent are logged at this level. > > > // This level could be enabled in production builds to allow later > > > inspection. > > > > > > // vlog() is used for details often needed for debugging clangd sessions. > > > // This level would typically be enabled for clangd developers. > > > ``` > > My recent experience of debugging some weird GotoDef issues tells me that > > log of index should be on production (since it is a non-trivial part in a > > clangd session), it would be really helpful to understand what is going on. > I agree that they are useful for debugging, but they might not be interesting > to end-users. And I think that's why there is `vlog`. Clangd developers could > use a different log level with `--log` flag. I agree that `vlog` is probably a better fit here, but still think we should change it across `CodeComplete.cpp` in a single commit, rather than partially apply the guidelines to different parts of the file. However, if everyone agrees we should use `vlog` here, happy to use `vlog` too and also send a patch to update all the rest of the usages. The situation I'm trying to avoid is: 1. We commit `vlog` here. 2. Someone argues that using `log` is actually better and we decide to not change other usages to `log`. 3. We end up with inconsistent choices across this file: `vlog` for signature help and `log` for code completion. But if there's an agreement that we should use `vlog` everywhere, more than happy to change it :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", hokein wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > ioeric wrote: > > > > drive by: I think this should be `vlog` or `dlog`. > > > Code completion also logs the number of results from sema, index, etc. > > > using the `log()` call. > > > The added log message looks similar, so trying to be consistent with the > > > rest of the code in this file. > > > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather > > > leave it to a separate patch. > > I'm not sure which level code completion log messages should be in, but I > > think we should follow the guidelines in the logger documentation instead > > of the existing uses. > > > > Quote from Logger.h > > ``` > > // log() is used for information important to understanding a clangd > > session. > > // e.g. the names of LSP messages sent are logged at this level. > > // This level could be enabled in production builds to allow later > > inspection. > > > > // vlog() is used for details often needed for debugging clangd sessions. > > // This level would typically be enabled for clangd developers. > > ``` > My recent experience of debugging some weird GotoDef issues tells me that log > of index should be on production (since it is a non-trivial part in a clangd > session), it would be really helpful to understand what is going on. I agree that they are useful for debugging, but they might not be interesting to end-users. And I think that's why there is `vlog`. Clangd developers could use a different log level with `--log` flag. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > drive by: I think this should be `vlog` or `dlog`. > > Code completion also logs the number of results from sema, index, etc. > > using the `log()` call. > > The added log message looks similar, so trying to be consistent with the > > rest of the code in this file. > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather > > leave it to a separate patch. > I'm not sure which level code completion log messages should be in, but I > think we should follow the guidelines in the logger documentation instead of > the existing uses. > > Quote from Logger.h > ``` > // log() is used for information important to understanding a clangd session. > // e.g. the names of LSP messages sent are logged at this level. > // This level could be enabled in production builds to allow later inspection. > > // vlog() is used for details often needed for debugging clangd sessions. > // This level would typically be enabled for clangd developers. > ``` My recent experience of debugging some weird GotoDef issues tells me that log of index should be on production (since it is a non-trivial part in a clangd session), it would be really helpful to understand what is going on. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ilya-biryukov wrote: > ioeric wrote: > > drive by: I think this should be `vlog` or `dlog`. > Code completion also logs the number of results from sema, index, etc. using > the `log()` call. > The added log message looks similar, so trying to be consistent with the rest > of the code in this file. > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave > it to a separate patch. I'm not sure which level code completion log messages should be in, but I think we should follow the guidelines in the logger documentation instead of the existing uses. Quote from Logger.h ``` // log() is used for information important to understanding a clangd session. // e.g. the names of LSP messages sent are logged at this level. // This level could be enabled in production builds to allow later inspection. // vlog() is used for details often needed for debugging clangd sessions. // This level would typically be enabled for clangd developers. ``` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > drive by: I think this should be `vlog` or `dlog`. Code completion also logs the number of results from sema, index, etc. using the `log()` call. The added log message looks similar, so trying to be consistent with the rest of the code in this file. Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave it to a separate patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein added inline comments. Comment at: clangd/index/Index.h:65 public: + static llvm::Optional forDecl(const Decl &D); + ilya-biryukov wrote: > hokein wrote: > > We already have this similar function in clangd/AST.h. > Thanks for pointing this out. > It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern > would probably be a somewhat broken layering, right? E.g. `Index.h` should > not directly depend on anything AST-specific Yes, I think we will not add any AST-specific stuff to `Index.h`, that's why we have `AST.h`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:742 +llvm::DenseMap FetchedDocs; +if (Index) { + LookupRequest IndexRequest; hokein wrote: > nit: do we want to log anything here? It may be useful for debug. Definitely useful. Thanks! Comment at: clangd/index/Index.h:65 public: + static llvm::Optional forDecl(const Decl &D); + hokein wrote: > We already have this similar function in clangd/AST.h. Thanks for pointing this out. It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern would probably be a somewhat broken layering, right? E.g. `Index.h` should not directly depend on anything AST-specific Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", drive by: I think this should be `vlog` or `dlog`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov updated this revision to Diff 160992. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Log on index request, remove FIXME that was addressed - Remove SymbolID::forDecl, use existing helper instead Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 Files: clangd/ClangdServer.cpp clangd/CodeComplete.cpp clangd/CodeComplete.h unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -826,11 +826,19 @@ EXPECT_TRUE(Results.Completions.empty()); } -SignatureHelp signatures(StringRef Text) { +SignatureHelp signatures(StringRef Text, + std::vector IndexSymbols = {}) { + std::unique_ptr Index; + if (!IndexSymbols.empty()) +Index = memIndex(IndexSymbols); + MockFSProvider FS; MockCompilationDatabase CDB; IgnoreDiagnostics DiagConsumer; - ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + ClangdServer::Options Opts = ClangdServer::optsForTest(); + Opts.StaticIndex = Index.get(); + + ClangdServer Server(CDB, FS, DiagConsumer, Opts); auto File = testPath("foo.cpp"); Annotations Test(Text); runAddDocument(Server, File, Test.code()); @@ -845,6 +853,7 @@ return false; return true; } +MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; } Matcher Sig(std::string Label, std::vector Params) { @@ -1590,6 +1599,51 @@ ElementsAre(Sig("foo(T, U) -> void", {"T", "U"}))); } +TEST(SignatureHelpTest, IndexDocumentation) { + Symbol::Details DocDetails; + DocDetails.Documentation = "Doc from the index"; + + Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#"); + Foo0.Detail = &DocDetails; + Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#"); + Foo1.Detail = &DocDetails; + Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#"); + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +int foo(double); + +void test() { + foo(^); +} + )cpp", + {Foo0}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(double) -> int", {"double"}), SigDoc(""; + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +// Overriden doc from sema +int foo(int); +// Doc from sema +int foo(int, int); + +void test() { + foo(^); +} + )cpp", + {Foo0, Foo1, Foo2}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(int) -> int", {"int"}), +SigDoc("Overriden doc from sema")), + AllOf(Sig("foo(int, int) -> int", {"int", "int"}), +SigDoc("Doc from sema"; +} + } // namespace } // namespace clangd } // namespace clang Index: clangd/CodeComplete.h === --- clangd/CodeComplete.h +++ clangd/CodeComplete.h @@ -171,12 +171,11 @@ CodeCompleteOptions Opts); /// Get signature help at a specified \p Pos in \p FileName. -SignatureHelp signatureHelp(PathRef FileName, -const tooling::CompileCommand &Command, -PrecompiledPreamble const *Preamble, -StringRef Contents, Position Pos, -IntrusiveRefCntPtr VFS, -std::shared_ptr PCHs); +SignatureHelp +signatureHelp(PathRef FileName, const tooling::CompileCommand &Command, + PrecompiledPreamble const *Preamble, StringRef Contents, + Position Pos, IntrusiveRefCntPtr VFS, + std::shared_ptr PCHs, SymbolIndex *Index); // For index-based completion, we only consider: // * symbols in namespaces or translation unit scopes (e.g. no class Index: clangd/CodeComplete.cpp === --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -687,18 +687,23 @@ llvm::unique_function ResultsCallback; }; -using ScoredSignature = -std::pair; +struct ScoredSignature { + // When set, requires documentation to be requested from the index with this + // ID. + llvm::Optional IDForDoc; + SignatureInformation Signature; + SignatureQualitySignals Quality; +}; class SignatureHelpCollector final : public CodeCompleteConsumer { - public: SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts, - SignatureHelp &SigHelp) - : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false), + SymbolIndex *Index, SignatureHelp &SigHelp) + : CodeComplete
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein accepted this revision. hokein added a comment. looks good, a few nits. Comment at: clangd/CodeComplete.cpp:742 +llvm::DenseMap FetchedDocs; +if (Index) { + LookupRequest IndexRequest; nit: do we want to log anything here? It may be useful for debug. Comment at: clangd/index/Index.h:65 public: + static llvm::Optional forDecl(const Decl &D); + We already have this similar function in clangd/AST.h. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov updated this revision to Diff 160647. ilya-biryukov added a comment. - run clang-format Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 Files: clangd/ClangdServer.cpp clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clangd/index/Index.h unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -826,11 +826,19 @@ EXPECT_TRUE(Results.Completions.empty()); } -SignatureHelp signatures(StringRef Text) { +SignatureHelp signatures(StringRef Text, + std::vector IndexSymbols = {}) { + std::unique_ptr Index; + if (!IndexSymbols.empty()) +Index = memIndex(IndexSymbols); + MockFSProvider FS; MockCompilationDatabase CDB; IgnoreDiagnostics DiagConsumer; - ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + ClangdServer::Options Opts = ClangdServer::optsForTest(); + Opts.StaticIndex = Index.get(); + + ClangdServer Server(CDB, FS, DiagConsumer, Opts); auto File = testPath("foo.cpp"); Annotations Test(Text); runAddDocument(Server, File, Test.code()); @@ -845,6 +853,7 @@ return false; return true; } +MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; } Matcher Sig(std::string Label, std::vector Params) { @@ -1590,6 +1599,51 @@ ElementsAre(Sig("foo(T, U) -> void", {"T", "U"}))); } +TEST(SignatureHelpTest, IndexDocumentation) { + Symbol::Details DocDetails; + DocDetails.Documentation = "Doc from the index"; + + Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#"); + Foo0.Detail = &DocDetails; + Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#"); + Foo1.Detail = &DocDetails; + Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#"); + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +int foo(double); + +void test() { + foo(^); +} + )cpp", + {Foo0}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(double) -> int", {"double"}), SigDoc(""; + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +// Overriden doc from sema +int foo(int); +// Doc from sema +int foo(int, int); + +void test() { + foo(^); +} + )cpp", + {Foo0, Foo1, Foo2}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(int) -> int", {"int"}), +SigDoc("Overriden doc from sema")), + AllOf(Sig("foo(int, int) -> int", {"int", "int"}), +SigDoc("Doc from sema"; +} + } // namespace } // namespace clangd } // namespace clang Index: clangd/index/Index.h === --- clangd/index/Index.h +++ clangd/index/Index.h @@ -21,6 +21,8 @@ #include namespace clang { +class Decl; + namespace clangd { struct SymbolLocation { @@ -60,6 +62,8 @@ // SymbolID can be used as key in the symbol indexes to lookup the symbol. class SymbolID { public: + static llvm::Optional forDecl(const Decl &D); + SymbolID() = default; explicit SymbolID(llvm::StringRef USR); Index: clangd/index/Index.cpp === --- clangd/index/Index.cpp +++ clangd/index/Index.cpp @@ -8,6 +8,7 @@ //===--===// #include "Index.h" +#include "clang/Index/USRGeneration.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/SHA1.h" #include "llvm/Support/raw_ostream.h" @@ -23,6 +24,13 @@ << L.End.Line << ":" << L.End.Column << ")"; } +llvm::Optional SymbolID::forDecl(const Decl &D) { + llvm::SmallString<128> USR; + if (index::generateUSRForDecl(&D, USR)) +return llvm::None; + return SymbolID(USR); +} + SymbolID::SymbolID(StringRef USR) : HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {} Index: clangd/CodeComplete.h === --- clangd/CodeComplete.h +++ clangd/CodeComplete.h @@ -171,12 +171,11 @@ CodeCompleteOptions Opts); /// Get signature help at a specified \p Pos in \p FileName. -SignatureHelp signatureHelp(PathRef FileName, -const tooling::CompileCommand &Command, -PrecompiledPreamble const *Preamble, -StringRef Contents, Position Pos, -IntrusiveRefCntPtr VFS, -std::shared_ptr PCHs); +SignatureHelp +signatureHelp(PathRef FileName, const tooling::CompileC
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, kadircet. Herald added subscribers: arphaman, mgrang, jkorous, MaskRay. Sema can only be used for documentation in the current file, other doc comments should be fetched from the index. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 Files: clangd/ClangdServer.cpp clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clangd/index/Index.h unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -826,11 +826,19 @@ EXPECT_TRUE(Results.Completions.empty()); } -SignatureHelp signatures(StringRef Text) { +SignatureHelp signatures(StringRef Text, + std::vector IndexSymbols = {}) { + std::unique_ptr Index; + if (!IndexSymbols.empty()) +Index = memIndex(IndexSymbols); + MockFSProvider FS; MockCompilationDatabase CDB; IgnoreDiagnostics DiagConsumer; - ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + ClangdServer::Options Opts = ClangdServer::optsForTest(); + Opts.StaticIndex = Index.get(); + + ClangdServer Server(CDB, FS, DiagConsumer, Opts); auto File = testPath("foo.cpp"); Annotations Test(Text); runAddDocument(Server, File, Test.code()); @@ -845,6 +853,7 @@ return false; return true; } +MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; } Matcher Sig(std::string Label, std::vector Params) { @@ -1590,6 +1599,51 @@ ElementsAre(Sig("foo(T, U) -> void", {"T", "U"}))); } +TEST(SignatureHelpTest, IndexDocumentation) { + Symbol::Details DocDetails; + DocDetails.Documentation = "Doc from the index"; + + Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#"); + Foo0.Detail = &DocDetails; + Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#"); + Foo1.Detail = &DocDetails; + Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#"); + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +int foo(double); + +void test() { + foo(^); +} + )cpp", + {Foo0}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(double) -> int", {"double"}), SigDoc(""; + + EXPECT_THAT( + signatures(R"cpp( +int foo(); +// Overriden doc from sema +int foo(int); +// Doc from sema +int foo(int, int); + +void test() { + foo(^); +} + )cpp", + {Foo0, Foo1, Foo2}) + .signatures, + ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), + AllOf(Sig("foo(int) -> int", {"int"}), +SigDoc("Overriden doc from sema")), + AllOf(Sig("foo(int, int) -> int", {"int", "int"}), +SigDoc("Doc from sema"; +} + } // namespace } // namespace clangd } // namespace clang Index: clangd/index/Index.h === --- clangd/index/Index.h +++ clangd/index/Index.h @@ -21,6 +21,8 @@ #include namespace clang { +class Decl; + namespace clangd { struct SymbolLocation { @@ -60,6 +62,8 @@ // SymbolID can be used as key in the symbol indexes to lookup the symbol. class SymbolID { public: + static llvm::Optional forDecl(const Decl& D); + SymbolID() = default; explicit SymbolID(llvm::StringRef USR); Index: clangd/index/Index.cpp === --- clangd/index/Index.cpp +++ clangd/index/Index.cpp @@ -8,6 +8,7 @@ //===--===// #include "Index.h" +#include "clang/Index/USRGeneration.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/SHA1.h" #include "llvm/Support/raw_ostream.h" @@ -23,8 +24,16 @@ << L.End.Line << ":" << L.End.Column << ")"; } +llvm::Optional SymbolID::forDecl(const Decl &D) { + llvm::SmallString<128> USR; + if (index::generateUSRForDecl(&D, USR)) +return llvm::None; + return SymbolID(USR); +} + SymbolID::SymbolID(StringRef USR) -: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {} +: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) { +} raw_ostream &operator<<(raw_ostream &OS, const SymbolID &ID) { OS << toHex(toStringRef(ID.HashValue)); Index: clangd/CodeComplete.h === --- clangd/CodeComplete.h +++ clangd/CodeComplete.h @@ -176,7 +176,8 @@ PrecompiledPreamble const *Preamble, StringRef Contents, Position Pos, IntrusiveRefCntPtr VFS, -