From: Sam McCall Date: Tue, 3 Mar 2020 14:57:39 +0000 (+0100) Subject: [clangd] Propagate versions into DraftStore, assigning where missing. NFC X-Git-Tag: 2020.06-alpha~123^2~301 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=caf5a4d57fe0ac0ca8c8d45fd31e5dbbc6bb6bec;p=platform%2Fupstream%2Fllvm.git [clangd] Propagate versions into DraftStore, assigning where missing. NFC This prepares for propagating versions through the server so diagnostics etc can be versioned. --- diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 9d93b8592fdc..44288eb7274d 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -117,7 +117,7 @@ llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) { // If the file is open in user's editor, make sure the version we // saw and current version are compatible as this is the text that // will be replaced by editors. - if (!It.second.canApplyTo(*Draft)) { + if (!It.second.canApplyTo(Draft->Contents)) { ++InvalidFileCount; LastInvalidFile = It.first(); } @@ -630,7 +630,7 @@ void ClangdLSPServer::onDocumentDidOpen( const std::string &Contents = Params.textDocument.text; - DraftMgr.addDraft(File, Contents); + DraftMgr.addDraft(File, Params.textDocument.version, Contents); Server->addDocument(File, Contents, WantDiagnostics::Yes); } @@ -642,19 +642,19 @@ void ClangdLSPServer::onDocumentDidChange( : WantDiagnostics::No; PathRef File = Params.textDocument.uri.file(); - llvm::Expected Contents = - DraftMgr.updateDraft(File, Params.contentChanges); - if (!Contents) { + llvm::Expected Draft = DraftMgr.updateDraft( + File, Params.textDocument.version, Params.contentChanges); + if (!Draft) { // 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); - elog("Failed to update {0}: {1}", File, Contents.takeError()); + elog("Failed to update {0}: {1}", File, Draft.takeError()); return; } - Server->addDocument(File, *Contents, WantDiags, Params.forceRebuild); + Server->addDocument(File, Draft->Contents, WantDiags, Params.forceRebuild); } void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { @@ -773,8 +773,7 @@ void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params, void ClangdLSPServer::onRename(const RenameParams &Params, Callback Reply) { Path File = std::string(Params.textDocument.uri.file()); - llvm::Optional Code = DraftMgr.getDraft(File); - if (!Code) + if (!DraftMgr.getDraft(File)) return Reply(llvm::make_error( "onRename called for non-added file", ErrorCode::InvalidParams)); Server->rename( @@ -829,7 +828,7 @@ void ClangdLSPServer::onDocumentOnTypeFormatting( "onDocumentOnTypeFormatting called for non-added file", ErrorCode::InvalidParams)); - Reply(Server->formatOnType(*Code, File, Params.position, Params.ch)); + Reply(Server->formatOnType(Code->Contents, File, Params.position, Params.ch)); } void ClangdLSPServer::onDocumentRangeFormatting( @@ -842,9 +841,10 @@ void ClangdLSPServer::onDocumentRangeFormatting( "onDocumentRangeFormatting called for non-added file", ErrorCode::InvalidParams)); - auto ReplacementsOrError = Server->formatRange(*Code, File, Params.range); + auto ReplacementsOrError = + Server->formatRange(Code->Contents, File, Params.range); if (ReplacementsOrError) - Reply(replacementsToEdits(*Code, ReplacementsOrError.get())); + Reply(replacementsToEdits(Code->Contents, ReplacementsOrError.get())); else Reply(ReplacementsOrError.takeError()); } @@ -859,9 +859,9 @@ void ClangdLSPServer::onDocumentFormatting( "onDocumentFormatting called for non-added file", ErrorCode::InvalidParams)); - auto ReplacementsOrError = Server->formatFile(*Code, File); + auto ReplacementsOrError = Server->formatFile(Code->Contents, File); if (ReplacementsOrError) - Reply(replacementsToEdits(*Code, ReplacementsOrError.get())); + Reply(replacementsToEdits(Code->Contents, ReplacementsOrError.get())); else Reply(ReplacementsOrError.takeError()); } @@ -1328,7 +1328,7 @@ bool ClangdLSPServer::shouldRunCompletion( // Running the lexer here would be more robust (e.g. we can detect comments // and avoid triggering completion there), but we choose to err on the side // of simplicity here. - auto Offset = positionToOffset(*Code, Params.position, + auto Offset = positionToOffset(Code->Contents, Params.position, /*AllowColumnsBeyondLineLength=*/false); if (!Offset) { vlog("could not convert position '{0}' to offset for file '{1}'", @@ -1339,9 +1339,9 @@ bool ClangdLSPServer::shouldRunCompletion( return false; if (Trigger == ">") - return (*Code)[*Offset - 2] == '-'; // trigger only on '->'. + return Code->Contents[*Offset - 2] == '-'; // trigger only on '->'. if (Trigger == ":") - return (*Code)[*Offset - 2] == ':'; // trigger only on '::'. + return Code->Contents[*Offset - 2] == ':'; // trigger only on '::'. assert(false && "unhandled trigger character"); return true; } @@ -1475,7 +1475,7 @@ void ClangdLSPServer::reparseOpenedFiles( // Reparse only opened files that were modified. for (const Path &FilePath : DraftMgr.getActiveFiles()) if (ModifiedFiles.find(FilePath) != ModifiedFiles.end()) - Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath), + Server->addDocument(FilePath, DraftMgr.getDraft(FilePath)->Contents, WantDiagnostics::Auto); } diff --git a/clang-tools-extra/clangd/DraftStore.cpp b/clang-tools-extra/clangd/DraftStore.cpp index d4a5d0c73b80..03867dcd286e 100644 --- a/clang-tools-extra/clangd/DraftStore.cpp +++ b/clang-tools-extra/clangd/DraftStore.cpp @@ -7,13 +7,14 @@ //===----------------------------------------------------------------------===// #include "DraftStore.h" +#include "Logger.h" #include "SourceCode.h" #include "llvm/Support/Errc.h" namespace clang { namespace clangd { -llvm::Optional DraftStore::getDraft(PathRef File) const { +llvm::Optional DraftStore::getDraft(PathRef File) const { std::lock_guard Lock(Mutex); auto It = Drafts.find(File); @@ -33,14 +34,32 @@ std::vector DraftStore::getActiveFiles() const { return ResultVector; } -void DraftStore::addDraft(PathRef File, llvm::StringRef Contents) { +static void updateVersion(DraftStore::Draft &D, + llvm::Optional Version) { + if (Version) { + // We treat versions as opaque, but the protocol says they increase. + if (*Version <= D.Version) + log("File version went from {0} to {1}", D.Version, Version); + D.Version = *Version; + } else { + // Note that if D was newly-created, this will bump D.Version from -1 to 0. + ++D.Version; + } +} + +int64_t DraftStore::addDraft(PathRef File, llvm::Optional Version, + llvm::StringRef Contents) { std::lock_guard Lock(Mutex); - Drafts[File] = std::string(Contents); + Draft &D = Drafts[File]; + updateVersion(D, Version); + D.Contents = Contents.str(); + return D.Version; } -llvm::Expected DraftStore::updateDraft( - PathRef File, llvm::ArrayRef Changes) { +llvm::Expected DraftStore::updateDraft( + PathRef File, llvm::Optional Version, + llvm::ArrayRef Changes) { std::lock_guard Lock(Mutex); auto EntryIt = Drafts.find(File); @@ -49,8 +68,8 @@ llvm::Expected DraftStore::updateDraft( "Trying to do incremental update on non-added document: " + File, llvm::errc::invalid_argument); } - - std::string Contents = EntryIt->second; + Draft &D = EntryIt->second; + std::string Contents = EntryIt->second.Contents; for (const TextDocumentContentChangeEvent &Change : Changes) { if (!Change.range) { @@ -104,8 +123,9 @@ llvm::Expected DraftStore::updateDraft( Contents = std::move(NewContents); } - EntryIt->second = Contents; - return Contents; + updateVersion(D, Version); + D.Contents = std::move(Contents); + return D; } void DraftStore::removeDraft(PathRef File) { diff --git a/clang-tools-extra/clangd/DraftStore.h b/clang-tools-extra/clangd/DraftStore.h index 1578ce9fad6b..babc679ed763 100644 --- a/clang-tools-extra/clangd/DraftStore.h +++ b/clang-tools-extra/clangd/DraftStore.h @@ -23,26 +23,37 @@ namespace clangd { /// A thread-safe container for files opened in a workspace, addressed by /// filenames. The contents are owned by the DraftStore. This class supports /// both whole and incremental updates of the documents. +/// Each time a draft is updated, it is assigned a version number. This can be +/// specified by the caller or incremented from the previous version. class DraftStore { public: + struct Draft { + std::string Contents; + int64_t Version = -1; + }; + /// \return Contents of the stored document. /// For untracked files, a llvm::None is returned. - llvm::Optional getDraft(PathRef File) const; + llvm::Optional getDraft(PathRef File) const; /// \return List of names of the drafts in this store. std::vector getActiveFiles() const; /// Replace contents of the draft for \p File with \p Contents. - void addDraft(PathRef File, StringRef Contents); + /// If no version is specified, one will be automatically assigned. + /// Returns the version. + int64_t addDraft(PathRef File, llvm::Optional Version, + 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. + /// If no version is specified, one will be automatically assigned. /// /// \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::Expected + updateDraft(PathRef File, llvm::Optional Version, llvm::ArrayRef Changes); /// Remove the draft from the store. @@ -50,7 +61,7 @@ public: private: mutable std::mutex Mutex; - llvm::StringMap Drafts; + llvm::StringMap Drafts; }; } // namespace clangd diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index 5a867c52c1ed..ba96f495c148 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -90,6 +90,20 @@ bool fromJSON(const llvm::json::Value &Params, TextDocumentIdentifier &R) { return O && O.map("uri", R.uri); } +llvm::json::Value toJSON(const VersionedTextDocumentIdentifier &R) { + auto Result = toJSON(static_cast(R)); + if (R.version) + Result.getAsObject()->try_emplace("version", R.version); + return Result; +} + +bool fromJSON(const llvm::json::Value &Params, + VersionedTextDocumentIdentifier &R) { + llvm::json::ObjectMapper O(Params); + return fromJSON(Params, static_cast(R)) && O && + O.map("version", R.version); +} + bool fromJSON(const llvm::json::Value &Params, Position &R) { llvm::json::ObjectMapper O(Params); return O && O.map("line", R.line) && O.map("character", R.character); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 596c7e9004e7..5d3914da699f 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -124,6 +124,20 @@ struct TextDocumentIdentifier { llvm::json::Value toJSON(const TextDocumentIdentifier &); bool fromJSON(const llvm::json::Value &, TextDocumentIdentifier &); +struct VersionedTextDocumentIdentifier : public TextDocumentIdentifier { + // The version number of this document. If a versioned text document + // identifier is sent from the server to the client and the file is not open + // in the editor (the server has not received an open notification before) the + // server can send `null` to indicate that the version is known and the + // content on disk is the master (as speced with document content ownership). + // + // The version number of a document will increase after each change, including + // undo/redo. The number doesn't need to be consecutive. + llvm::Optional version; +}; +llvm::json::Value toJSON(const VersionedTextDocumentIdentifier &); +bool fromJSON(const llvm::json::Value &, VersionedTextDocumentIdentifier &); + struct Position { /// Line position in a document (zero-based). int line = 0; @@ -223,7 +237,7 @@ struct TextDocumentItem { std::string languageId; /// The version number of this document (it will strictly increase after each - int version = 0; + std::int64_t version = 0; /// The content of the opened text document. std::string text; @@ -643,7 +657,7 @@ struct DidChangeTextDocumentParams { /// The document that did change. The version number points /// to the version after all provided content changes have /// been applied. - TextDocumentIdentifier textDocument; + VersionedTextDocumentIdentifier textDocument; /// The actual content changes. std::vector contentChanges; diff --git a/clang-tools-extra/clangd/unittests/DraftStoreTests.cpp b/clang-tools-extra/clangd/unittests/DraftStoreTests.cpp index 1840892cd5e9..5ff9f254b55d 100644 --- a/clang-tools-extra/clangd/unittests/DraftStoreTests.cpp +++ b/clang-tools-extra/clangd/unittests/DraftStoreTests.cpp @@ -9,6 +9,7 @@ #include "Annotations.h" #include "DraftStore.h" #include "SourceCode.h" +#include "llvm/Testing/Support/Error.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -36,7 +37,7 @@ void stepByStep(llvm::ArrayRef Steps) { constexpr llvm::StringLiteral Path("/hello.cpp"); // Set the initial content. - DS.addDraft(Path, InitialSrc.code()); + EXPECT_EQ(0, DS.addDraft(Path, llvm::None, InitialSrc.code())); for (size_t i = 1; i < Steps.size(); i++) { Annotations SrcBefore(Steps[i - 1].Src); @@ -48,10 +49,12 @@ void stepByStep(llvm::ArrayRef Steps) { Contents.str(), }; - llvm::Expected Result = DS.updateDraft(Path, {Event}); + llvm::Expected Result = + DS.updateDraft(Path, llvm::None, {Event}); ASSERT_TRUE(!!Result); - EXPECT_EQ(*Result, SrcAfter.code()); - EXPECT_EQ(*DS.getDraft(Path), SrcAfter.code()); + EXPECT_EQ(Result->Contents, SrcAfter.code()); + EXPECT_EQ(DS.getDraft(Path)->Contents, SrcAfter.code()); + EXPECT_EQ(Result->Version, static_cast(i)); } } @@ -75,13 +78,15 @@ void allAtOnce(llvm::ArrayRef Steps) { } // Set the initial content. - DS.addDraft(Path, InitialSrc.code()); + EXPECT_EQ(0, DS.addDraft(Path, llvm::None, InitialSrc.code())); - llvm::Expected Result = DS.updateDraft(Path, Changes); + llvm::Expected Result = + DS.updateDraft(Path, llvm::None, Changes); ASSERT_TRUE(!!Result) << llvm::toString(Result.takeError()); - EXPECT_EQ(*Result, FinalSrc.code()); - EXPECT_EQ(*DS.getDraft(Path), FinalSrc.code()); + EXPECT_EQ(Result->Contents, FinalSrc.code()); + EXPECT_EQ(DS.getDraft(Path)->Contents, FinalSrc.code()); + EXPECT_EQ(Result->Version, 1); } TEST(DraftStoreIncrementalUpdateTest, Simple) { @@ -184,7 +189,7 @@ TEST(DraftStoreIncrementalUpdateTest, WrongRangeLength) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -194,7 +199,8 @@ TEST(DraftStoreIncrementalUpdateTest, WrongRangeLength) { Change.range->end.character = 2; Change.rangeLength = 10; - Expected Result = DS.updateDraft(File, {Change}); + Expected Result = + DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ( @@ -206,7 +212,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndBeforeStart) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -215,7 +221,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndBeforeStart) { Change.range->end.line = 0; Change.range->end.character = 3; - Expected Result = DS.updateDraft(File, {Change}); + auto Result = DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), @@ -226,7 +232,7 @@ TEST(DraftStoreIncrementalUpdateTest, StartCharOutOfRange) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -236,7 +242,7 @@ TEST(DraftStoreIncrementalUpdateTest, StartCharOutOfRange) { Change.range->end.character = 100; Change.text = "foo"; - Expected Result = DS.updateDraft(File, {Change}); + auto Result = DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), @@ -247,7 +253,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndCharOutOfRange) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -257,7 +263,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndCharOutOfRange) { Change.range->end.character = 100; Change.text = "foo"; - Expected Result = DS.updateDraft(File, {Change}); + auto Result = DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), @@ -268,7 +274,7 @@ TEST(DraftStoreIncrementalUpdateTest, StartLineOutOfRange) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -278,7 +284,7 @@ TEST(DraftStoreIncrementalUpdateTest, StartLineOutOfRange) { Change.range->end.character = 0; Change.text = "foo"; - Expected Result = DS.updateDraft(File, {Change}); + auto Result = DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), "Line value is out of range (100)"); @@ -288,7 +294,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndLineOutOfRange) { DraftStore DS; Path File = "foo.cpp"; - DS.addDraft(File, "int main() {}\n"); + DS.addDraft(File, llvm::None, "int main() {}\n"); TextDocumentContentChangeEvent Change; Change.range.emplace(); @@ -298,7 +304,7 @@ TEST(DraftStoreIncrementalUpdateTest, EndLineOutOfRange) { Change.range->end.character = 0; Change.text = "foo"; - Expected Result = DS.updateDraft(File, {Change}); + auto Result = DS.updateDraft(File, llvm::None, {Change}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), "Line value is out of range (100)"); @@ -311,7 +317,7 @@ TEST(DraftStoreIncrementalUpdateTest, InvalidRangeInASequence) { Path File = "foo.cpp"; StringRef OriginalContents = "int main() {}\n"; - DS.addDraft(File, OriginalContents); + EXPECT_EQ(0, DS.addDraft(File, llvm::None, OriginalContents)); // The valid change TextDocumentContentChangeEvent Change1; @@ -331,15 +337,51 @@ TEST(DraftStoreIncrementalUpdateTest, InvalidRangeInASequence) { Change2.range->end.character = 100; Change2.text = "something"; - Expected Result = DS.updateDraft(File, {Change1, Change2}); + auto Result = DS.updateDraft(File, llvm::None, {Change1, Change2}); EXPECT_TRUE(!Result); EXPECT_EQ(toString(Result.takeError()), "utf-16 offset 100 is invalid for line 0"); - Optional Contents = DS.getDraft(File); + Optional Contents = DS.getDraft(File); EXPECT_TRUE(Contents); - EXPECT_EQ(*Contents, OriginalContents); + EXPECT_EQ(Contents->Contents, OriginalContents); + EXPECT_EQ(Contents->Version, 0); +} + +TEST(DraftStore, Version) { + DraftStore DS; + Path File = "foo.cpp"; + + EXPECT_EQ(25, DS.addDraft(File, 25, "")); + EXPECT_EQ(25, DS.getDraft(File)->Version); + + EXPECT_EQ(26, DS.addDraft(File, llvm::None, "")); + EXPECT_EQ(26, DS.getDraft(File)->Version); + + // We allow versions to go backwards. + EXPECT_EQ(7, DS.addDraft(File, 7, "")); + EXPECT_EQ(7, DS.getDraft(File)->Version); + + // Valid (no-op) change modifies version. + auto Result = DS.updateDraft(File, 10, {}); + EXPECT_TRUE(!!Result); + EXPECT_EQ(10, Result->Version); + EXPECT_EQ(10, DS.getDraft(File)->Version); + + Result = DS.updateDraft(File, llvm::None, {}); + EXPECT_TRUE(!!Result); + EXPECT_EQ(11, Result->Version); + EXPECT_EQ(11, DS.getDraft(File)->Version); + + TextDocumentContentChangeEvent InvalidChange; + InvalidChange.range.emplace(); + InvalidChange.rangeLength = 99; + + Result = DS.updateDraft(File, 15, {InvalidChange}); + EXPECT_FALSE(!!Result); + consumeError(Result.takeError()); + EXPECT_EQ(11, DS.getDraft(File)->Version); } } // namespace