[clang-tools-extra] 596b63a - [clangd] Rebuild dependent files when a header is saved.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 13 13:14:00 PDT 2020
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
More information about the cfe-commits
mailing list