From 5178f924c486c79854c06fd617bdac74c8c7bdfe Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 22 Feb 2018 14:00:39 +0000 Subject: [PATCH] [clangd] DidChangeConfiguration Notification Summary: Implementation of DidChangeConfiguration notification handling in clangd. This currently only supports changing one setting: the path of the compilation database to be used for the current project. In other words, it is no longer necessary to restart clangd with a different command line argument in order to change the compilation database. Reviewers: malaperle, krasimir, bkramer, ilya-biryukov Subscribers: jkorous-apple, ioeric, simark, klimek, ilya-biryukov, arphaman, rwols, cfe-commits Differential Revision: https://reviews.llvm.org/D39571 Signed-off-by: Simon Marchi Signed-off-by: William Enright llvm-svn: 325784 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 12 +++ clang-tools-extra/clangd/ClangdLSPServer.h | 1 + clang-tools-extra/clangd/ClangdServer.cpp | 5 + clang-tools-extra/clangd/ClangdServer.h | 5 + clang-tools-extra/clangd/DraftStore.cpp | 11 +++ clang-tools-extra/clangd/DraftStore.h | 6 ++ .../clangd/GlobalCompilationDatabase.cpp | 6 ++ .../clangd/GlobalCompilationDatabase.h | 3 + clang-tools-extra/clangd/Protocol.cpp | 10 ++ clang-tools-extra/clangd/Protocol.h | 14 +++ clang-tools-extra/clangd/ProtocolHandlers.cpp | 2 + clang-tools-extra/clangd/ProtocolHandlers.h | 1 + clang-tools-extra/unittests/clangd/ClangdTests.cpp | 104 +++++++++++++++++++++ 13 files changed, 180 insertions(+) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 7ebd40d..587aee7 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -370,6 +370,18 @@ void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) { }); } +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( + DidChangeConfigurationParams &Params) { + ClangdConfigurationParamsChange &Settings = Params.settings; + + // Compilation database change. + if (Settings.compilationDatabasePath.hasValue()) { + CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue()); + Server.reparseOpenedFiles(); + } +} + ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, bool StorePreamblesInMemory, const clangd::CodeCompleteOptions &CCOpts, diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index d3a6c8d..c3c0f80 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -76,6 +76,7 @@ private: void onCommand(ExecuteCommandParams &Params) override; void onRename(RenameParams &Parames) override; void onHover(TextDocumentPositionParams &Params) override; + void onChangeConfiguration(DidChangeConfigurationParams &Params) override; std::vector getFixIts(StringRef File, const clangd::Diagnostic &D); diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 4e04059..e9003d0 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -554,6 +554,11 @@ void ClangdServer::scheduleReparseAndDiags( WantDiags, std::move(Callback)); } +void ClangdServer::reparseOpenedFiles() { + for (const Path &FilePath : DraftMgr.getActiveFiles()) + forceReparse(FilePath); +} + void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { // FIXME: Do nothing for now. This will be used for indexing and potentially // invalidating other caches. diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 1be3dbb..cfda66e 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -163,6 +163,11 @@ public: /// and AST and rebuild them from scratch. void forceReparse(PathRef File); + /// Calls forceReparse() on all currently opened files. + /// As a result, this method may be very expensive. + /// This method is normally called when the compilation database is changed. + void reparseOpenedFiles(); + /// Run code completion for \p File at \p Pos. /// Request is processed asynchronously. /// diff --git a/clang-tools-extra/clangd/DraftStore.cpp b/clang-tools-extra/clangd/DraftStore.cpp index f5ceff2..4bda859 100644 --- a/clang-tools-extra/clangd/DraftStore.cpp +++ b/clang-tools-extra/clangd/DraftStore.cpp @@ -21,6 +21,17 @@ VersionedDraft DraftStore::getDraft(PathRef File) const { return It->second; } +std::vector DraftStore::getActiveFiles() const { + std::lock_guard Lock(Mutex); + std::vector ResultVector; + + for (auto DraftIt = Drafts.begin(); DraftIt != Drafts.end(); DraftIt++) + if (DraftIt->second.Draft) + ResultVector.push_back(DraftIt->getKey()); + + return ResultVector; +} + DocVersion DraftStore::getVersion(PathRef File) const { std::lock_guard Lock(Mutex); diff --git a/clang-tools-extra/clangd/DraftStore.h b/clang-tools-extra/clangd/DraftStore.h index 2dd7184..4757768 100644 --- a/clang-tools-extra/clangd/DraftStore.h +++ b/clang-tools-extra/clangd/DraftStore.h @@ -40,6 +40,12 @@ public: /// \return version and contents of the stored document. /// For untracked files, a (0, None) pair is returned. VersionedDraft getDraft(PathRef File) const; + + /// \return List of names of active drafts in this store. Drafts that were + /// removed (which still have an entry in the Drafts map) are not returned by + /// this function. + std::vector getActiveFiles() const; + /// \return version of the tracked document. /// For untracked files, 0 is returned. DocVersion getVersion(PathRef File) const; diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp index 6d92428..c1ad729 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -51,6 +51,12 @@ DirectoryBasedGlobalCompilationDatabase::getFallbackCommand( return C; } +void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) { + std::lock_guard Lock(Mutex); + CompileCommandsDir = P; + CompilationDatabases.clear(); +} + void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile( PathRef File, std::vector ExtraFlags) { std::lock_guard Lock(Mutex); diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h index cb5ad66..b763789 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h @@ -61,6 +61,9 @@ public: /// Uses the default fallback command, adding any extra flags. tooling::CompileCommand getFallbackCommand(PathRef File) const override; + /// Set the compile commands directory to \p P. + void setCompileCommandsDir(Path P); + /// Sets the extra flags that should be added to a file. void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags); diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index 57f342b1..8992599 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -497,5 +497,15 @@ json::Expr toJSON(const DocumentHighlight &DH) { }; } +bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) { + json::ObjectMapper O(Params); + return O && O.map("settings", CCP.settings); +} + +bool fromJSON(const json::Expr &Params, ClangdConfigurationParamsChange &CCPC) { + json::ObjectMapper O(Params); + return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 2c1b278..40ccfaf 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -324,6 +324,20 @@ struct DidChangeWatchedFilesParams { }; bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &); +/// Clangd extension to manage a workspace/didChangeConfiguration notification +/// since the data received is described as 'any' type in LSP. +struct ClangdConfigurationParamsChange { + llvm::Optional compilationDatabasePath; +}; +bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &); + +struct DidChangeConfigurationParams { + // We use this predefined struct because it is easier to use + // than the protocol specified type of 'any'. + ClangdConfigurationParamsChange settings; +}; +bool fromJSON(const json::Expr &, DidChangeConfigurationParams &); + struct FormattingOptions { /// Size of a tab in spaces. int tabSize = 0; diff --git a/clang-tools-extra/clangd/ProtocolHandlers.cpp b/clang-tools-extra/clangd/ProtocolHandlers.cpp index 2f65744..32cfd2f 100644 --- a/clang-tools-extra/clangd/ProtocolHandlers.cpp +++ b/clang-tools-extra/clangd/ProtocolHandlers.cpp @@ -72,4 +72,6 @@ void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, Register("workspace/executeCommand", &ProtocolCallbacks::onCommand); Register("textDocument/documentHighlight", &ProtocolCallbacks::onDocumentHighlight); + Register("workspace/didChangeConfiguration", + &ProtocolCallbacks::onChangeConfiguration); } diff --git a/clang-tools-extra/clangd/ProtocolHandlers.h b/clang-tools-extra/clangd/ProtocolHandlers.h index d06f293..7599ece 100644 --- a/clang-tools-extra/clangd/ProtocolHandlers.h +++ b/clang-tools-extra/clangd/ProtocolHandlers.h @@ -52,6 +52,7 @@ public: virtual void onRename(RenameParams &Parames) = 0; virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0; virtual void onHover(TextDocumentPositionParams &Params) = 0; + virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0; }; void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out, diff --git a/clang-tools-extra/unittests/clangd/ClangdTests.cpp b/clang-tools-extra/unittests/clangd/ClangdTests.cpp index 3f402d7..ac750ea 100644 --- a/clang-tools-extra/unittests/clangd/ClangdTests.cpp +++ b/clang-tools-extra/unittests/clangd/ClangdTests.cpp @@ -7,6 +7,7 @@ // //===----------------------------------------------------------------------===// +#include "Annotations.h" #include "ClangdLSPServer.h" #include "ClangdServer.h" #include "Matchers.h" @@ -77,6 +78,43 @@ private: VFSTag LastVFSTag = VFSTag(); }; +/// For each file, record whether the last published diagnostics contained at +/// least one error. +class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer { +public: + void + onDiagnosticsReady(PathRef File, + Tagged> Diagnostics) override { + bool HadError = diagsContainErrors(Diagnostics.Value); + + std::lock_guard Lock(Mutex); + LastDiagsHadError[File] = HadError; + } + + /// Exposes all files consumed by onDiagnosticsReady in an unspecified order. + /// For each file, a bool value indicates whether the last diagnostics + /// contained an error. + std::vector> filesWithDiags() const { + std::vector> Result; + std::lock_guard Lock(Mutex); + + for (const auto &it : LastDiagsHadError) { + Result.emplace_back(it.first(), it.second); + } + + return Result; + } + + void clear() { + std::lock_guard Lock(Mutex); + LastDiagsHadError.clear(); + } + +private: + mutable std::mutex Mutex; + llvm::StringMap LastDiagsHadError; +}; + /// Replaces all patterns of the form 0x123abc with spaces std::string replacePtrsInDump(std::string const &Dump) { llvm::Regex RE("0x[0-9a-fA-F]+"); @@ -413,6 +451,72 @@ int main() { return 0; } EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); } +// Test ClangdServer.reparseOpenedFiles. +TEST_F(ClangdVFSTest, ReparseOpenedFiles) { + Annotations FooSource(R"cpp( +#ifdef MACRO +$one[[static void bob() {}]] +#else +$two[[static void bob() {}]] +#endif + +int main () { bo^b (); return 0; } +)cpp"); + + Annotations BarSource(R"cpp( +#ifdef MACRO +this is an error +#endif +)cpp"); + + Annotations BazSource(R"cpp( +int hello; +)cpp"); + + MockFSProvider FS; + MockCompilationDatabase CDB; + MultipleErrorCheckingDiagConsumer DiagConsumer; + ClangdServer Server(CDB, DiagConsumer, FS, + /*AsyncThreadsCount=*/0, + /*StorePreamblesInMemory=*/true); + + auto FooCpp = testPath("foo.cpp"); + auto BarCpp = testPath("bar.cpp"); + auto BazCpp = testPath("baz.cpp"); + + FS.Files[FooCpp] = ""; + FS.Files[BarCpp] = ""; + FS.Files[BazCpp] = ""; + + CDB.ExtraClangFlags = {"-DMACRO=1"}; + Server.addDocument(FooCpp, FooSource.code()); + Server.addDocument(BarCpp, BarSource.code()); + Server.addDocument(BazCpp, BazSource.code()); + + EXPECT_THAT(DiagConsumer.filesWithDiags(), + UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), + Pair(BazCpp, false))); + + auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(Locations)); + EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp}, + FooSource.range("one")})); + + // Undefine MACRO, close baz.cpp. + CDB.ExtraClangFlags.clear(); + DiagConsumer.clear(); + Server.removeDocument(BazCpp); + Server.reparseOpenedFiles(); + + EXPECT_THAT(DiagConsumer.filesWithDiags(), + UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false))); + + Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(Locations)); + EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp}, + FooSource.range("two")})); +} + TEST_F(ClangdVFSTest, MemoryUsage) { MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; -- 2.7.4