simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov,
mgorny, klimek.
simark added a reviewer: malaperle.
This patch adds support for incremental document syncing, as described
in the LSP spec. The protocol specifies ranges in terms of Position (a
line and a character), and our drafts are stored as plain strings. So I
see two things that may not be super efficient for very large files:
- Converting a Position to an offset (the positionToOffset function) requires
searching for end of lines until we reach the desired line.
- When we update a range, we construct a new string, which implies copying the
whole document.
However, for the typical size of a C++ document and the frequency of
update (at which a user types), it may not be an issue. This patch aims
at getting the basic feature in, and we can always improve it later if
we find it's too slow.
Signed-off-by: Simon Marchi <simon.mar...@ericsson.com>
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
unittests/clangd/DraftStoreTests.cpp
Index: unittests/clangd/DraftStoreTests.cpp
===================================================================
--- /dev/null
+++ unittests/clangd/DraftStoreTests.cpp
@@ -0,0 +1,164 @@
+//===-- DraftStoreTests.cpp - Clangd unit tests -----------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Annotations.h"
+#include "DraftStore.h"
+#include "Protocol.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+using namespace llvm;
+
+namespace {
+using testing::ElementsAre;
+using testing::IsEmpty;
+
+struct IncrementalTestStep {
+ StringRef Src;
+ StringRef Contents;
+};
+
+static void
+runIncrementalUpdateTest(llvm::ArrayRef<IncrementalTestStep> Steps) {
+ DraftStore DS;
+ std::string NewContents;
+ Annotations InitialSrc(Steps[0].Src);
+ const char Path[] = "/hello.cpp";
+
+ // Set the initial content.
+ DS.updateDraft(Path, InitialSrc.code(), {}, &NewContents);
+ EXPECT_EQ(NewContents, InitialSrc.code());
+ EXPECT_EQ(*DS.getDraft(Path).Draft, InitialSrc.code());
+
+ // For each step, replace the range marked in \p Src with \p Contents, and
+ // verify that it's equal to the following step's \p Src.
+ for (size_t i = 1; i < Steps.size(); i++) {
+ Annotations SrcBefore(Steps[i - 1].Src);
+ Annotations SrcAfter(Steps[i].Src);
+ StringRef Contents = Steps[i - 1].Contents;
+
+ DS.updateDraft(Path, Contents, SrcBefore.range(), &NewContents);
+ EXPECT_EQ(NewContents, SrcAfter.code());
+ EXPECT_EQ(*DS.getDraft(Path).Draft, SrcAfter.code());
+ }
+}
+
+TEST(DraftStoreIncrementalUpdateTest, Simple) {
+ // clang-format off
+ IncrementalTestStep Steps[] =
+ {
+ // Replace a range
+ {
+R"cpp(static int
+hello[[World]]()
+{})cpp",
+ "Universe"
+ },
+ // Delete a range
+ {
+R"cpp(static int
+hello[[Universe]]()
+{})cpp",
+ ""
+ },
+ // Add a range
+ {
+R"cpp(static int
+hello[[]]()
+{})cpp",
+ "Monde"
+ },
+ {
+R"cpp(static int
+helloMonde()
+{})cpp",
+ ""
+ }
+ };
+ // clang-format on
+
+ runIncrementalUpdateTest(Steps);
+}
+
+TEST(DraftStoreIncrementalUpdateTest, MultiLine) {
+ // clang-format off
+ IncrementalTestStep Steps[] =
+ {
+ // Replace a range
+ {
+R"cpp(static [[int
+helloWorld]]()
+{})cpp",
+R"cpp(char
+welcome)cpp"
+ },
+ // Delete a range
+ {
+R"cpp(static char[[
+welcome]]()
+{})cpp",
+ ""
+ },
+ // Add a range
+ {
+R"cpp(static char[[]]()
+{})cpp",
+ R"cpp(
+cookies)cpp"
+ },
+ {
+R"cpp(static char
+cookies()
+{})cpp",
+ ""
+ }
+ };
+ // clang-format on
+
+ runIncrementalUpdateTest(Steps);
+}
+
+TEST(DraftStoreUpdateTest, Simple) {
+ StringRef Src =
+ R"cpp(static int
+helloWorld()
+{})cpp";
+
+ DraftStore DS;
+ StringRef Path = "/hello.cpp";
+ std::string NewContents;
+
+ // Add a document
+ DS.updateDraft(Path, Src, {}, &NewContents);
+ EXPECT_EQ(NewContents, Src);
+ EXPECT_EQ(*DS.getDraft(Path).Draft, Src);
+ EXPECT_THAT(DS.getActiveFiles(), ElementsAre(Path));
+
+ StringRef Src2 =
+ R"cpp(static char
+thisIsAFunction()
+{})cpp";
+ DS.updateDraft(Path, Src2, {}, &NewContents);
+ EXPECT_EQ(NewContents, Src2);
+ EXPECT_EQ(*DS.getDraft(Path).Draft, Src2);
+ EXPECT_THAT(DS.getActiveFiles(), ElementsAre(Path));
+
+ DS.removeDraft(Path);
+ EXPECT_FALSE(DS.getDraft(Path).Draft.hasValue());
+ EXPECT_THAT(DS.getActiveFiles(), IsEmpty());
+ // Remove a document
+ // Replace the whole content
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===================================================================
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -15,6 +15,7 @@
CodeCompleteTests.cpp
CodeCompletionStringsTests.cpp
ContextTests.cpp
+ DraftStoreTests.cpp
FileIndexTests.cpp
FuzzyMatchTests.cpp
HeadersTests.cpp
Index: test/clangd/initialize-params.test
===================================================================
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -36,7 +36,7 @@
# CHECK-NEXT: ","
# CHECK-NEXT: ]
# CHECK-NEXT: },
-# CHECK-NEXT: "textDocumentSync": 1
+# CHECK-NEXT: "textDocumentSync": 2
# CHECK-NEXT: }
# CHECK-NEXT: }
---
Index: test/clangd/initialize-params-invalid.test
===================================================================
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -36,7 +36,7 @@
# CHECK-NEXT: ","
# CHECK-NEXT: ]
# CHECK-NEXT: },
-# CHECK-NEXT: "textDocumentSync": 1
+# CHECK-NEXT: "textDocumentSync": 2
# CHECK-NEXT: }
# CHECK-NEXT: }
---
Index: clangd/Protocol.h
===================================================================
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -284,7 +284,10 @@
bool fromJSON(const json::Expr &, DidCloseTextDocumentParams &);
struct TextDocumentContentChangeEvent {
- /// The new text of the document.
+ /// The range of the document that changed.
+ llvm::Optional<Range> range;
+
+ /// The new text of the range/document.
std::string text;
};
bool fromJSON(const json::Expr &, TextDocumentContentChangeEvent &);
Index: clangd/Protocol.cpp
===================================================================
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -248,7 +248,7 @@
bool fromJSON(const json::Expr &Params, TextDocumentContentChangeEvent &R) {
json::ObjectMapper O(Params);
- return O && O.map("text", R.text);
+ return O && O.map("text", R.text) && O.map("range", R.range);
}
bool fromJSON(const json::Expr &Params, FormattingOptions &R) {
Index: clangd/DraftStore.h
===================================================================
--- clangd/DraftStore.h
+++ clangd/DraftStore.h
@@ -11,6 +11,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DRAFTSTORE_H
#include "Path.h"
+#include "Protocol.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/StringMap.h"
#include <cstdint>
@@ -50,9 +51,14 @@
/// For untracked files, 0 is returned.
DocVersion getVersion(PathRef File) const;
- /// Replace contents of the draft for \p File with \p Contents.
+ /// Update the contents of the draft for \p File with \p Contents. If
+ /// \p Range is specified, \p Contents replaces the text in this range.
+ /// Otherwise, it represents the complete content of the document.
+ /// \p *NewContents is updated to contain the new complete content
+ /// of the document.
/// \return The new version of the draft for \p File.
- DocVersion updateDraft(PathRef File, StringRef Contents);
+ DocVersion updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> Range, std::string *NewContents);
/// Remove the contents of the draft
/// \return The new version of the draft for \p File.
DocVersion removeDraft(PathRef File);
Index: clangd/DraftStore.cpp
===================================================================
--- clangd/DraftStore.cpp
+++ clangd/DraftStore.cpp
@@ -8,6 +8,8 @@
//===----------------------------------------------------------------------===//
#include "DraftStore.h"
+#include "Logger.h"
+#include "SourceCode.h"
using namespace clang;
using namespace clang::clangd;
@@ -41,12 +43,38 @@
return It->second.Version;
}
-DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents) {
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> range,
+ std::string *NewContents) {
std::lock_guard<std::mutex> Lock(Mutex);
-
auto &Entry = Drafts[File];
+ std::string newContents;
+
+ if (range) {
+ // Can't specify a range if we don't have an existing draft. In that case,
+ // return the version unchanged.
+ if (!Entry.Draft.hasValue()) {
+ log(llvm::Twine(
+ "Trying to do incremental update on draft without content: ") +
+ File);
+ return Entry.Version;
+ }
+
+ size_t startIndex = positionToOffset(*Entry.Draft, range->start);
+ size_t endIndex = positionToOffset(*Entry.Draft, range->end);
+
+ newContents = Entry.Draft->substr(0, startIndex);
+ newContents += Contents;
+ newContents += Entry.Draft->substr(endIndex);
+ } else {
+ newContents = Contents;
+ }
+
DocVersion NewVersion = ++Entry.Version;
- Entry.Draft = Contents;
+ Entry.Draft = newContents;
+
+ *NewContents = std::move(newContents);
+
return NewVersion;
}
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -157,7 +157,8 @@
/// separate thread. When the parsing is complete, DiagConsumer passed in
/// constructor will receive onDiagnosticsReady callback.
void addDocument(PathRef File, StringRef Contents,
- WantDiagnostics WD = WantDiagnostics::Auto);
+ WantDiagnostics WantDiags = WantDiagnostics::Auto,
+ llvm::Optional<Range> Range = {});
/// Remove \p File from list of tracked files, schedule a request to free
/// resources associated with it.
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -117,10 +117,13 @@
}
void ClangdServer::addDocument(PathRef File, StringRef Contents,
- WantDiagnostics WantDiags) {
- DocVersion Version = DraftMgr.updateDraft(File, Contents);
+ WantDiagnostics WantDiags,
+ llvm::Optional<Range> Range) {
+ std::string NewContents;
+ DocVersion Version =
+ DraftMgr.updateDraft(File, Contents, Range, &NewContents);
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
- scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()},
+ scheduleReparseAndDiags(File, VersionedDraft{Version, std::move(NewContents)},
WantDiags, std::move(TaggedFS));
}
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -98,7 +98,7 @@
reply(json::obj{
{{"capabilities",
json::obj{
- {"textDocumentSync", 1},
+ {"textDocumentSync", 2},
{"documentFormattingProvider", true},
{"documentRangeFormattingProvider", true},
{"documentOnTypeFormattingProvider",
@@ -153,9 +153,10 @@
if (Params.wantDiagnostics.hasValue())
WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
: WantDiagnostics::No;
- // We only support full syncing right now.
- Server.addDocument(Params.textDocument.uri.file(),
- Params.contentChanges[0].text, WantDiags);
+
+ const TextDocumentContentChangeEvent &Change = Params.contentChanges[0];
+ Server.addDocument(Params.textDocument.uri.file(), Change.text, WantDiags,
+ Change.range);
}
void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
