Author: Sam McCall Date: 2020-04-13T22:08:15+02:00 New Revision: 596b63ad4019e61030803789a1844a0f1aeb34db
URL: https://github.com/llvm/llvm-project/commit/596b63ad4019e61030803789a1844a0f1aeb34db DIFF: https://github.com/llvm/llvm-project/commit/596b63ad4019e61030803789a1844a0f1aeb34db.diff LOG: [clangd] Rebuild dependent files when a header is saved. Summary: Caveats: - only works when the header is changed in the editor and the editor provides the notification - we revalidate preambles for all open files (stat all their headers) rather than taking advantage of the fact that we know which file changed. This is much simpler! Fixes https://github.com/clangd/clangd/issues/107 Reviewers: kadircet Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D77847 Added: Modified: clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdLSPServer.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/test/initialize-params.test clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index f8e1d1f8bd3f..f58bfaf99648 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -569,7 +569,12 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params, {"version", getClangToolFullVersion("clangd")}}}, {"capabilities", llvm::json::Object{ - {"textDocumentSync", (int)TextDocumentSyncKind::Incremental}, + {"textDocumentSync", + llvm::json::Object{ + {"openClose", true}, + {"change", (int)TextDocumentSyncKind::Incremental}, + {"save", true}, + }}, {"documentFormattingProvider", true}, {"documentRangeFormattingProvider", true}, {"documentOnTypeFormattingProvider", @@ -684,7 +689,16 @@ void ClangdLSPServer::onDocumentDidChange( WantDiags, Params.forceRebuild); } +void ClangdLSPServer::onDocumentDidSave( + const DidSaveTextDocumentParams &Params) { + reparseOpenFilesIfNeeded([](llvm::StringRef) { return true; }); +} + void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { + // We could also reparse all open files here. However: + // - this could be frequent, and revalidating all the preambles isn't free + // - this is useful e.g. when switching git branches, but we're likely to see + // fresh headers but still have the old-branch main-file content Server->onFileEvent(Params); } @@ -1180,7 +1194,8 @@ void ClangdLSPServer::applyConfiguration( } } - reparseOpenedFiles(ModifiedFiles); + reparseOpenFilesIfNeeded( + [&](llvm::StringRef File) { return ModifiedFiles.count(File) != 0; }); } void ClangdLSPServer::publishTheiaSemanticHighlighting( @@ -1358,6 +1373,7 @@ ClangdLSPServer::ClangdLSPServer( MsgHandler->bind("textDocument/didOpen", &ClangdLSPServer::onDocumentDidOpen); MsgHandler->bind("textDocument/didClose", &ClangdLSPServer::onDocumentDidClose); MsgHandler->bind("textDocument/didChange", &ClangdLSPServer::onDocumentDidChange); + MsgHandler->bind("textDocument/didSave", &ClangdLSPServer::onDocumentDidSave); MsgHandler->bind("workspace/didChangeWatchedFiles", &ClangdLSPServer::onFileEvent); MsgHandler->bind("workspace/didChangeConfiguration", &ClangdLSPServer::onChangeConfiguration); MsgHandler->bind("textDocument/symbolInfo", &ClangdLSPServer::onSymbolInfo); @@ -1566,13 +1582,11 @@ void ClangdLSPServer::onFileUpdated(PathRef File, const TUStatus &Status) { notify("textDocument/clangd.fileStatus", Status.render(File)); } -void ClangdLSPServer::reparseOpenedFiles( - const llvm::StringSet<> &ModifiedFiles) { - if (ModifiedFiles.empty()) - return; +void ClangdLSPServer::reparseOpenFilesIfNeeded( + llvm::function_ref<bool(llvm::StringRef File)> Filter) { // Reparse only opened files that were modified. for (const Path &FilePath : DraftMgr.getActiveFiles()) - if (ModifiedFiles.find(FilePath) != ModifiedFiles.end()) + if (Filter(FilePath)) if (auto Draft = DraftMgr.getDraft(FilePath)) // else disappeared in race? Server->addDocument(FilePath, std::move(Draft->Contents), encodeVersion(Draft->Version), diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index 294fe0ef6415..9c35ca6bda3a 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -75,6 +75,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks { void onDocumentDidOpen(const DidOpenTextDocumentParams &); void onDocumentDidChange(const DidChangeTextDocumentParams &); void onDocumentDidClose(const DidCloseTextDocumentParams &); + void onDocumentDidSave(const DidSaveTextDocumentParams &); void onDocumentOnTypeFormatting(const DocumentOnTypeFormattingParams &, Callback<std::vector<TextEdit>>); void onDocumentRangeFormatting(const DocumentRangeFormattingParams &, @@ -131,10 +132,12 @@ class ClangdLSPServer : private ClangdServer::Callbacks { /// produce '->' and '::', respectively. bool shouldRunCompletion(const CompletionParams &Params) const; - /// Forces a reparse of all currently opened files which were modified. As a - /// result, this method may be very expensive. This method is normally called - /// when the compilation database is changed. - void reparseOpenedFiles(const llvm::StringSet<> &ModifiedFiles); + /// Requests a reparse of currently opened files using their latest source. + /// This will typically only rebuild if something other than the source has + /// changed (e.g. the CDB yields diff erent flags, or files included in the + /// preamble have been modified). + void reparseOpenFilesIfNeeded( + llvm::function_ref<bool(llvm::StringRef File)> Filter); void applyConfiguration(const ConfigurationSettings &Settings); /// Sends a "publishSemanticHighlighting" notification to the LSP client. diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index aa7b901da3f0..5756e3b02871 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -452,6 +452,11 @@ bool fromJSON(const llvm::json::Value &Params, DidCloseTextDocumentParams &R) { return O && O.map("textDocument", R.textDocument); } +bool fromJSON(const llvm::json::Value &Params, DidSaveTextDocumentParams &R) { + llvm::json::ObjectMapper O(Params); + return O && O.map("textDocument", R.textDocument); +} + bool fromJSON(const llvm::json::Value &Params, DidChangeTextDocumentParams &R) { llvm::json::ObjectMapper O(Params); if (!O) diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index d43522002cf0..8a0e5b44e057 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -654,6 +654,12 @@ struct DidCloseTextDocumentParams { }; bool fromJSON(const llvm::json::Value &, DidCloseTextDocumentParams &); +struct DidSaveTextDocumentParams { + /// The document that was saved. + TextDocumentIdentifier textDocument; +}; +bool fromJSON(const llvm::json::Value &, DidSaveTextDocumentParams &); + struct TextDocumentContentChangeEvent { /// The range of the document that changed. llvm::Optional<Range> range; diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test index 6c4b847a07ef..39bbc0fe01f5 100644 --- a/clang-tools-extra/clangd/test/initialize-params.test +++ b/clang-tools-extra/clangd/test/initialize-params.test @@ -56,7 +56,11 @@ # CHECK-NEXT: "," # CHECK-NEXT: ] # CHECK-NEXT: }, -# CHECK-NEXT: "textDocumentSync": 2, +# CHECK-NEXT: "textDocumentSync": { +# CHECK-NEXT: "change": 2, +# CHECK-NEXT: "openClose": true, +# CHECK-NEXT: "save": true +# CHECK-NEXT: }, # CHECK-NEXT: "typeHierarchyProvider": true # CHECK-NEXT: "workspaceSymbolProvider": true # CHECK-NEXT: }, diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp index b8029b954377..5f169fefd3bd 100644 --- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp @@ -126,6 +126,27 @@ TEST_F(LSPTest, Diagnostics) { EXPECT_THAT(Client.diagnostics("foo.cpp"), llvm::ValueIs(testing::IsEmpty())); } +TEST_F(LSPTest, DiagnosticsHeaderSaved) { + auto &Client = start(); + FS.Files["foo.h"] = "#define VAR original"; + Client.didOpen("foo.cpp", R"cpp( + #include "foo.h" + int x = VAR; + )cpp"); + EXPECT_THAT(Client.diagnostics("foo.cpp"), + llvm::ValueIs(testing::ElementsAre( + DiagMessage("Use of undeclared identifier 'original'")))); + // Now modify the header from within the "editor". + FS.Files["foo.h"] = "#define VAR changed"; + Client.notify( + "textDocument/didSave", + llvm::json::Object{{"textDocument", Client.documentID("foo.h")}}); + // Foo.cpp should be rebuilt with new diagnostics. + EXPECT_THAT(Client.diagnostics("foo.cpp"), + llvm::ValueIs(testing::ElementsAre( + DiagMessage("Use of undeclared identifier 'changed'")))); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
