sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
As strange as it seems to our files-are-strings view of the world, some editors
that treat files as arrays of lines can get confused about whether the last line
has a newline or not.
The consequences of failing to handle a bad incremental update are catastrophic.
If an update would be valid except for a missing newline at end of file, pretend
one exists.
This fixes problems still present in neovim where deleting all text often leads
to a desync shortly afterwards: https://github.com/neovim/neovim/issues/17085
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D135508
Files:
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -954,6 +954,44 @@
"the computed range length (2)."));
}
+// Test that we correct observed buggy edits from Neovim.
+TEST(ApplyEditsTets, BuggyNeovimEdits) {
+ TextDocumentContentChangeEvent Change;
+ Change.range.emplace();
+
+ // https://github.com/neovim/neovim/issues/17085
+ // Adding a blank line after a (missing) newline
+ std::string Code = "a";
+ Change.range->start.line = 1;
+ Change.range->start.character = 0;
+ Change.range->end.line = 1;
+ Change.range->start.character = 0;
+ Change.rangeLength = 0;
+ Change.text = "\n";
+ EXPECT_THAT_ERROR(applyChange(Code, Change), llvm::Succeeded());
+ EXPECT_EQ(Code, "a\n\n");
+
+ // https://github.com/neovim/neovim/issues/17085#issuecomment-1269162264
+ // Replacing the (missing) newline with \n\n in an empty file.
+ Code = "";
+ Change.range->start.line = 0;
+ Change.range->start.character = 0;
+ Change.range->end.line = 1;
+ Change.range->end.character = 0;
+ Change.rangeLength = 1;
+ Change.text = "\n\n";
+
+ EXPECT_THAT_ERROR(applyChange(Code, Change), llvm::Succeeded());
+ EXPECT_EQ(Code, "\n\n");
+
+ // We do not apply the heuristic fixes if the rangeLength doesn't match.
+ Code = "";
+ Change.rangeLength = 0;
+ EXPECT_THAT_ERROR(applyChange(Code, Change),
+ FailedWithMessage("Change's rangeLength (0) doesn't match "
+ "the computed range length (1)."));
+}
+
TEST(ApplyEditsTest, EndBeforeStart) {
std::string Code = "int main() {}\n";
Index: clang-tools-extra/clangd/SourceCode.cpp
===================================================================
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -1063,6 +1063,40 @@
return llvm::Error::success();
}
+// Workaround for editors that have buggy handling of newlines at end of file.
+//
+// The editor is supposed to expose document contents over LSP as an exact
+// string, with whitespace and newlines well-defined. But internally many
+// editors treat text as an array of lines, and there can be ambiguity over
+// whether the last line ends with a newline or not.
+//
+// This confusion can lead to incorrect edits being sent. Failing to apply them
+// is catastrophic: we're desynced, LSP has no mechanism to get back in sync.
+// We apply a heuristic to avoid this state.
+//
+// If our current view of an N-line file does *not* end in a newline, but the
+// editor refers to the start of the next line (an impossible location), then
+// we silently add a newline to make this valid.
+// We will still validate that the rangeLength is correct, *including* the
+// inferred newline.
+//
+// See https://github.com/neovim/neovim/issues/17085
+static void inferFinalNewline(llvm::Expected<size_t> &Err,
+ std::string &Contents, const Position &Pos) {
+ if (Err)
+ return;
+ if (!Contents.empty() && Contents.back() == '\n')
+ return;
+ if (Pos.character != 0)
+ return;
+ if (Pos.line != llvm::count(Contents, '\n') + 1)
+ return;
+ log("Editor sent invalid change coordinates, inferring newline at EOF");
+ Contents.push_back('\n');
+ consumeError(Err.takeError());
+ Err = Contents.size();
+}
+
llvm::Error applyChange(std::string &Contents,
const TextDocumentContentChangeEvent &Change) {
if (!Change.range) {
@@ -1072,11 +1106,13 @@
const Position &Start = Change.range->start;
llvm::Expected<size_t> StartIndex = positionToOffset(Contents, Start, false);
+ inferFinalNewline(StartIndex, Contents, Start);
if (!StartIndex)
return StartIndex.takeError();
const Position &End = Change.range->end;
llvm::Expected<size_t> EndIndex = positionToOffset(Contents, End, false);
+ inferFinalNewline(EndIndex, Contents, End);
if (!EndIndex)
return EndIndex.takeError();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
