[clang-tools-extra] a53f895 - [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2022-11-29T13:15:20+01:00
New Revision: a53f89522f46dd10f87f163d8501738182729b8d

URL: https://github.com/llvm/llvm-project/commit/a53f89522f46dd10f87f163d8501738182729b8d
DIFF: https://github.com/llvm/llvm-project/commit/a53f89522f46dd10f87f163d8501738182729b8d.diff

LOG: [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof

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

Differential Revision: https://reviews.llvm.org/D135508

Added: 
    

Modified: 
    clang-tools-extra/clangd/SourceCode.cpp
    clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 5913db5405a34..d51bb997b5305 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -1063,6 +1063,40 @@ llvm::Error reformatEdit(Edit &E, const format::FormatStyle &Style) {
   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 @@ llvm::Error applyChange(std::string &Contents,
 
   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();
 

diff  --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
index d7fd1a09e85d3..ce22062ab4d5e 100644
--- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -954,6 +954,44 @@ TEST(ApplyEditsTest, WrongRangeLength) {
                                       "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";