[clangd] Propagate versions into DraftStore, assigning where missing. NFC

This prepares for propagating versions through the server so diagnostics
etc can be versioned.
This commit is contained in:
Sam McCall 2020-03-03 15:57:39 +01:00
parent dfe8f5da4c
commit caf5a4d57f
6 changed files with 161 additions and 60 deletions

View file

@ -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<std::string> Contents =
DraftMgr.updateDraft(File, Params.contentChanges);
if (!Contents) {
llvm::Expected<DraftStore::Draft> 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<WorkspaceEdit> Reply) {
Path File = std::string(Params.textDocument.uri.file());
llvm::Optional<std::string> Code = DraftMgr.getDraft(File);
if (!Code)
if (!DraftMgr.getDraft(File))
return Reply(llvm::make_error<LSPError>(
"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);
}

View file

@ -7,13 +7,14 @@
//===----------------------------------------------------------------------===//
#include "DraftStore.h"
#include "Logger.h"
#include "SourceCode.h"
#include "llvm/Support/Errc.h"
namespace clang {
namespace clangd {
llvm::Optional<std::string> DraftStore::getDraft(PathRef File) const {
llvm::Optional<DraftStore::Draft> DraftStore::getDraft(PathRef File) const {
std::lock_guard<std::mutex> Lock(Mutex);
auto It = Drafts.find(File);
@ -33,14 +34,32 @@ std::vector<Path> DraftStore::getActiveFiles() const {
return ResultVector;
}
void DraftStore::addDraft(PathRef File, llvm::StringRef Contents) {
std::lock_guard<std::mutex> Lock(Mutex);
Drafts[File] = std::string(Contents);
static void updateVersion(DraftStore::Draft &D,
llvm::Optional<int64_t> 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;
}
}
llvm::Expected<std::string> DraftStore::updateDraft(
PathRef File, llvm::ArrayRef<TextDocumentContentChangeEvent> Changes) {
int64_t DraftStore::addDraft(PathRef File, llvm::Optional<int64_t> Version,
llvm::StringRef Contents) {
std::lock_guard<std::mutex> Lock(Mutex);
Draft &D = Drafts[File];
updateVersion(D, Version);
D.Contents = Contents.str();
return D.Version;
}
llvm::Expected<DraftStore::Draft> DraftStore::updateDraft(
PathRef File, llvm::Optional<int64_t> Version,
llvm::ArrayRef<TextDocumentContentChangeEvent> Changes) {
std::lock_guard<std::mutex> Lock(Mutex);
auto EntryIt = Drafts.find(File);
@ -49,8 +68,8 @@ llvm::Expected<std::string> 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<std::string> 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) {

View file

@ -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<std::string> getDraft(PathRef File) const;
llvm::Optional<Draft> getDraft(PathRef File) const;
/// \return List of names of the drafts in this store.
std::vector<Path> 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<int64_t> 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<std::string>
updateDraft(PathRef File,
llvm::Expected<Draft>
updateDraft(PathRef File, llvm::Optional<int64_t> Version,
llvm::ArrayRef<TextDocumentContentChangeEvent> Changes);
/// Remove the draft from the store.
@ -50,7 +61,7 @@ public:
private:
mutable std::mutex Mutex;
llvm::StringMap<std::string> Drafts;
llvm::StringMap<Draft> Drafts;
};
} // namespace clangd

View file

@ -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<const TextDocumentIdentifier &>(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<TextDocumentIdentifier &>(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);

View file

@ -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<std::int64_t> 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<TextDocumentContentChangeEvent> contentChanges;

View file

@ -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<IncrementalTestStep> 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<IncrementalTestStep> Steps) {
Contents.str(),
};
llvm::Expected<std::string> Result = DS.updateDraft(Path, {Event});
llvm::Expected<DraftStore::Draft> 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<int64_t>(i));
}
}
@ -75,13 +78,15 @@ void allAtOnce(llvm::ArrayRef<IncrementalTestStep> Steps) {
}
// Set the initial content.
DS.addDraft(Path, InitialSrc.code());
EXPECT_EQ(0, DS.addDraft(Path, llvm::None, InitialSrc.code()));
llvm::Expected<std::string> Result = DS.updateDraft(Path, Changes);
llvm::Expected<DraftStore::Draft> 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<std::string> Result = DS.updateDraft(File, {Change});
Expected<DraftStore::Draft> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> 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<std::string> Contents = DS.getDraft(File);
Optional<DraftStore::Draft> 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