Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp @@ -100,7 +100,7 @@ reply(json::obj{ {{"capabilities", json::obj{ - {"textDocumentSync", 1}, + {"textDocumentSync", (int)TextDocumentSyncKind::Incremental}, {"documentFormattingProvider", true}, {"documentRangeFormattingProvider", true}, {"documentOnTypeFormattingProvider", @@ -147,25 +147,30 @@ PathRef File = Params.textDocument.uri.file(); std::string &Contents = Params.textDocument.text; - DraftMgr.updateDraft(File, Contents); + DraftMgr.addDraft(File, Contents); Server.addDocument(File, Contents, WantDiagnostics::Yes); } void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) { - if (Params.contentChanges.size() != 1) - return replyError(ErrorCode::InvalidParams, - "can only apply one change at a time"); auto WantDiags = WantDiagnostics::Auto; if (Params.wantDiagnostics.hasValue()) WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes : WantDiagnostics::No; PathRef File = Params.textDocument.uri.file(); - std::string &Contents = Params.contentChanges[0].text; + llvm::Expected Contents = + DraftMgr.updateDraft(File, Params.contentChanges); + if (!Contents) { + // If this fails, we are most likely going to be not in sync anymore with + // the client. It is better to remove the draft and let further operations + // fail rather than giving wrong results. + DraftMgr.removeDraft(File); + Server.removeDocument(File); + log(llvm::toString(Contents.takeError())); + return; + } - // We only support full syncing right now. - DraftMgr.updateDraft(File, Contents); - Server.addDocument(File, Contents, WantDiags); + Server.addDocument(File, *Contents, WantDiags); } void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) { Index: clang-tools-extra/trunk/clangd/DraftStore.h =================================================================== --- clang-tools-extra/trunk/clangd/DraftStore.h +++ clang-tools-extra/trunk/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 @@ -21,7 +22,8 @@ namespace clangd { /// A thread-safe container for files opened in a workspace, addressed by -/// filenames. The contents are owned by the DraftStore. +/// filenames. The contents are owned by the DraftStore. This class supports +/// both whole and incremental updates of the documents. class DraftStore { public: /// \return Contents of the stored document. @@ -32,7 +34,17 @@ std::vector getActiveFiles() const; /// Replace contents of the draft for \p File with \p Contents. - void updateDraft(PathRef File, StringRef Contents); + void addDraft(PathRef File, StringRef Contents); + + /// Update the contents of the draft for \p File based on \p Changes. + /// If a position in \p Changes is invalid (e.g. out-of-range), the + /// draft is not modified. + /// + /// \return The new version of the draft for \p File, or an error if the + /// changes couldn't be applied. + llvm::Expected + updateDraft(PathRef File, + llvm::ArrayRef Changes); /// Remove the draft from the store. void removeDraft(PathRef File); Index: clang-tools-extra/trunk/clangd/DraftStore.cpp =================================================================== --- clang-tools-extra/trunk/clangd/DraftStore.cpp +++ clang-tools-extra/trunk/clangd/DraftStore.cpp @@ -8,6 +8,8 @@ //===----------------------------------------------------------------------===// #include "DraftStore.h" +#include "SourceCode.h" +#include "llvm/Support/Errc.h" using namespace clang; using namespace clang::clangd; @@ -32,12 +34,72 @@ return ResultVector; } -void DraftStore::updateDraft(PathRef File, StringRef Contents) { +void DraftStore::addDraft(PathRef File, StringRef Contents) { std::lock_guard Lock(Mutex); Drafts[File] = Contents; } +llvm::Expected DraftStore::updateDraft( + PathRef File, llvm::ArrayRef Changes) { + std::lock_guard Lock(Mutex); + + auto EntryIt = Drafts.find(File); + if (EntryIt == Drafts.end()) { + return llvm::make_error( + "Trying to do incremental update on non-added document: " + File, + llvm::errc::invalid_argument); + } + + std::string Contents = EntryIt->second; + + for (const TextDocumentContentChangeEvent &Change : Changes) { + if (!Change.range) { + Contents = Change.text; + continue; + } + + const Position &Start = Change.range->start; + llvm::Expected StartIndex = + positionToOffset(Contents, Start, false); + if (!StartIndex) + return StartIndex.takeError(); + + const Position &End = Change.range->end; + llvm::Expected EndIndex = positionToOffset(Contents, End, false); + if (!EndIndex) + return EndIndex.takeError(); + + if (*EndIndex < *StartIndex) + return llvm::make_error( + llvm::formatv( + "Range's end position ({0}) is before start position ({1})", End, + Start), + llvm::errc::invalid_argument); + + if (Change.rangeLength && + (ssize_t)(*EndIndex - *StartIndex) != *Change.rangeLength) + return llvm::make_error( + llvm::formatv("Change's rangeLength ({0}) doesn't match the " + "computed range length ({1}).", + *Change.rangeLength, *EndIndex - *StartIndex), + llvm::errc::invalid_argument); + + std::string NewContents; + NewContents.reserve(*StartIndex + Change.text.length() + + (Contents.length() - *EndIndex)); + + NewContents = Contents.substr(0, *StartIndex); + NewContents += Change.text; + NewContents += Contents.substr(*EndIndex); + + Contents = std::move(NewContents); + } + + EntryIt->second = Contents; + return Contents; +} + void DraftStore::removeDraft(PathRef File) { std::lock_guard Lock(Mutex); Index: clang-tools-extra/trunk/clangd/Protocol.h =================================================================== --- clang-tools-extra/trunk/clangd/Protocol.h +++ clang-tools-extra/trunk/clangd/Protocol.h @@ -197,6 +197,20 @@ using ShutdownParams = NoParams; using ExitParams = NoParams; +/// Defines how the host (editor) should sync document changes to the language +/// server. +enum class TextDocumentSyncKind { + /// Documents should not be synced at all. + None = 0, + + /// Documents are synced by always sending the full content of the document. + Full = 1, + + /// Documents are synced by sending the full content on open. After that + /// only incremental updates to the document are send. + Incremental = 2, +}; + struct CompletionItemClientCapabilities { /// Client supports snippets as insert text. bool snippetSupport = false; @@ -287,7 +301,13 @@ bool fromJSON(const json::Expr &, DidCloseTextDocumentParams &); struct TextDocumentContentChangeEvent { - /// The new text of the document. + /// The range of the document that changed. + llvm::Optional range; + + /// The length of the range that got replaced. + llvm::Optional rangeLength; + + /// The new text of the range/document. std::string text; }; bool fromJSON(const json::Expr &, TextDocumentContentChangeEvent &); @@ -745,4 +765,14 @@ } // namespace clangd } // namespace clang +namespace llvm { +template <> struct format_provider { + static void format(const clang::clangd::Position &Pos, raw_ostream &OS, + StringRef Style) { + assert(Style.empty() && "style modifiers for this type are not supported"); + OS << Pos; + } +}; +} // namespace llvm + #endif Index: clang-tools-extra/trunk/clangd/Protocol.cpp =================================================================== --- clang-tools-extra/trunk/clangd/Protocol.cpp +++ clang-tools-extra/trunk/clangd/Protocol.cpp @@ -248,7 +248,8 @@ bool fromJSON(const json::Expr &Params, TextDocumentContentChangeEvent &R) { json::ObjectMapper O(Params); - return O && O.map("text", R.text); + return O && O.map("range", R.range) && O.map("rangeLength", R.rangeLength) && + O.map("text", R.text); } bool fromJSON(const json::Expr &Params, FormattingOptions &R) { Index: clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test =================================================================== --- clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clangd/initialize-params.test =================================================================== --- clang-tools-extra/trunk/test/clangd/initialize-params.test +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test =================================================================== --- clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test +++ clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test @@ -0,0 +1,37 @@ +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s +{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +--- +{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"int main() {}\n"}}} +--- +{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":6}}} +# CHECK: "id": 1, +# CHECK-NEXT: "jsonrpc": "2.0", +# CHECK-NEXT: "result": [ +# CHECK-NEXT: { +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 8, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 4, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: }, +# CHECK-NEXT: "uri": "file:///clangd-test/main.cpp" +# CHECK-NEXT: } +# CHECK-NEXT: ] +--- +{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///main.cpp"},"contentChanges":[{"range":{"start":{"line":100,"character":0},"end":{"line":100,"character":0}},"text": "foo"}]}} +--- +{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":6}}} +# CHECK: "error": { +# CHECK-NEXT: "code": -32602, +# CHECK-NEXT: "message": "trying to get AST for non-added document" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 1, +# CHECK-NEXT: "jsonrpc": "2.0" +# CHECK-NEXT:} +--- +{"jsonrpc":"2.0","id":4,"method":"shutdown"} Index: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt +++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt @@ -15,6 +15,7 @@ CodeCompleteTests.cpp CodeCompletionStringsTests.cpp ContextTests.cpp + DraftStoreTests.cpp FileIndexTests.cpp FuzzyMatchTests.cpp HeadersTests.cpp Index: clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp @@ -0,0 +1,350 @@ +//===-- DraftStoreTests.cpp -------------------------------------*- 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 "SourceCode.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace { + +struct IncrementalTestStep { + StringRef Src; + StringRef Contents; +}; + +int rangeLength(StringRef Code, const Range &Rng) { + llvm::Expected Start = positionToOffset(Code, Rng.start); + llvm::Expected End = positionToOffset(Code, Rng.end); + assert(Start); + assert(End); + return *End - *Start; +} + +/// Send the changes one by one to updateDraft, verify the intermediate results. +void stepByStep(llvm::ArrayRef Steps) { + DraftStore DS; + Annotations InitialSrc(Steps.front().Src); + constexpr llvm::StringLiteral Path("/hello.cpp"); + + // Set the initial content. + DS.addDraft(Path, InitialSrc.code()); + + 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; + TextDocumentContentChangeEvent Event{ + SrcBefore.range(), + rangeLength(SrcBefore.code(), SrcBefore.range()), + Contents.str(), + }; + + llvm::Expected Result = DS.updateDraft(Path, {Event}); + ASSERT_TRUE(!!Result); + EXPECT_EQ(*Result, SrcAfter.code()); + EXPECT_EQ(*DS.getDraft(Path), SrcAfter.code()); + } +} + +/// Send all the changes at once to updateDraft, check only the final result. +void allAtOnce(llvm::ArrayRef Steps) { + DraftStore DS; + Annotations InitialSrc(Steps.front().Src); + Annotations FinalSrc(Steps.back().Src); + constexpr llvm::StringLiteral Path("/hello.cpp"); + std::vector Changes; + + for (size_t i = 0; i < Steps.size() - 1; i++) { + Annotations Src(Steps[i].Src); + StringRef Contents = Steps[i].Contents; + + Changes.push_back({ + Src.range(), + rangeLength(Src.code(), Src.range()), + Contents.str(), + }); + } + + // Set the initial content. + DS.addDraft(Path, InitialSrc.code()); + + llvm::Expected Result = DS.updateDraft(Path, Changes); + + ASSERT_TRUE(!!Result) << llvm::toString(Result.takeError()); + EXPECT_EQ(*Result, FinalSrc.code()); + EXPECT_EQ(*DS.getDraft(Path), FinalSrc.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 + + stepByStep(Steps); + allAtOnce(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" + }, + // Replace the whole file + { +R"cpp([[static char +cookies() +{}]])cpp", + R"cpp(#include +)cpp" + }, + // Delete the whole file + { + R"cpp([[#include +]])cpp", + "", + }, + // Add something to an empty file + { + "[[]]", + R"cpp(int main() { +)cpp", + }, + { + R"cpp(int main() { +)cpp", + "" + } + }; + // clang-format on + + stepByStep(Steps); + allAtOnce(Steps); +} + +TEST(DraftStoreIncrementalUpdateTest, WrongRangeLength) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 0; + Change.range->start.character = 0; + Change.range->end.line = 0; + Change.range->end.character = 2; + Change.rangeLength = 10; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ( + llvm::toString(Result.takeError()), + "Change's rangeLength (10) doesn't match the computed range length (2)."); +} + +TEST(DraftStoreIncrementalUpdateTest, EndBeforeStart) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 0; + Change.range->start.character = 5; + Change.range->end.line = 0; + Change.range->end.character = 3; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Range's end position (0:3) is before start position (0:5)"); +} + +TEST(DraftStoreIncrementalUpdateTest, StartCharOutOfRange) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 0; + Change.range->start.character = 100; + Change.range->end.line = 0; + Change.range->end.character = 100; + Change.text = "foo"; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Character value is out of range (100)"); +} + +TEST(DraftStoreIncrementalUpdateTest, EndCharOutOfRange) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 0; + Change.range->start.character = 0; + Change.range->end.line = 0; + Change.range->end.character = 100; + Change.text = "foo"; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Character value is out of range (100)"); +} + +TEST(DraftStoreIncrementalUpdateTest, StartLineOutOfRange) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 100; + Change.range->start.character = 0; + Change.range->end.line = 100; + Change.range->end.character = 0; + Change.text = "foo"; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Line value is out of range (100)"); +} + +TEST(DraftStoreIncrementalUpdateTest, EndLineOutOfRange) { + DraftStore DS; + Path File = "foo.cpp"; + + DS.addDraft(File, "int main() {}\n"); + + TextDocumentContentChangeEvent Change; + Change.range.emplace(); + Change.range->start.line = 0; + Change.range->start.character = 0; + Change.range->end.line = 100; + Change.range->end.character = 0; + Change.text = "foo"; + + llvm::Expected Result = DS.updateDraft(File, {Change}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Line value is out of range (100)"); +} + +/// Check that if a valid change is followed by an invalid change, the original +/// version of the document (prior to all changes) is kept. +TEST(DraftStoreIncrementalUpdateTest, InvalidRangeInASequence) { + DraftStore DS; + Path File = "foo.cpp"; + + StringRef OriginalContents = "int main() {}\n"; + DS.addDraft(File, OriginalContents); + + // The valid change + TextDocumentContentChangeEvent Change1; + Change1.range.emplace(); + Change1.range->start.line = 0; + Change1.range->start.character = 0; + Change1.range->end.line = 0; + Change1.range->end.character = 0; + Change1.text = "Hello "; + + // The invalid change + TextDocumentContentChangeEvent Change2; + Change2.range.emplace(); + Change2.range->start.line = 0; + Change2.range->start.character = 5; + Change2.range->end.line = 0; + Change2.range->end.character = 100; + Change2.text = "something"; + + llvm::Expected Result = DS.updateDraft(File, {Change1, Change2}); + + EXPECT_TRUE(!Result); + EXPECT_EQ(llvm::toString(Result.takeError()), + "Character value is out of range (100)"); + + llvm::Optional Contents = DS.getDraft(File); + EXPECT_TRUE(Contents); + EXPECT_EQ(*Contents, OriginalContents); +} + +} // namespace +} // namespace clangd +} // namespace clang