[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

I'd like to add these comments to warn others of problems I
encountered when trying to use `RemoveLineIfEmpty`.  I originally
tried to fix the problem, but I realized I could implement the
functionality more easily and efficiently in my calling code where I
can make the simplifying assumption that there are no prior edits to
the line from which text is being removed.  While I've lost the
motivation to write a fix, which doesn't look easy, I figure a warning
to others is better than silence.

I've added a unit test to demonstrate the problem.  I don't know how
to mark it as an expected failure, so I just marked it disabled.

Reviewed By: jkorous

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

llvm-svn: 369049

1 files changed, 68 insertions, 5 deletions

diff --git a/clang/unittests/Rewrite/RewriteBufferTest.cpp b/clang/unittests/Rewrite/RewriteBufferTest.cpp
index eb8d986c4da..bf29c0ff928 100644
--- a/clang/unittests/Rewrite/RewriteBufferTest.cpp
+++ b/clang/unittests/Rewrite/RewriteBufferTest.cpp
@@ -14,6 +14,16 @@ using namespace clang;
 
 namespace {
 
+#define EXPECT_OUTPUT(Buf, Output) EXPECT_EQ(Output, writeOutput(Buf))
+
+static std::string writeOutput(const RewriteBuffer &Buf) {
+  std::string Result;
+  raw_string_ostream OS(Result);
+  Buf.write(OS);
+  OS.flush();
+  return Result;
+}
+
 static void tagRange(unsigned Offset, unsigned Len, StringRef tagName,
                      RewriteBuffer &Buf) {
   std::string BeginTag;
@@ -40,11 +50,64 @@ TEST(RewriteBuffer, TagRanges) {
   tagRange(Pos, TagStr.size(), "outer", Buf);
   tagRange(Pos, TagStr.size(), "inner", Buf);
 
-  std::string Result;
-  raw_string_ostream OS(Result);
-  Buf.write(OS);
-  OS.flush();
-  EXPECT_EQ(Output, Result);
+  EXPECT_OUTPUT(Buf, Output);
+}
+
+TEST(RewriteBuffer, DISABLED_RemoveLineIfEmpty_XFAIL) {
+  StringRef Input = "def\n"
+                    "ghi\n"
+                    "jkl\n";
+  RewriteBuffer Buf;
+  Buf.Initialize(Input);
+
+  // Insert "abc\n" at the start.
+  Buf.InsertText(0, "abc\n");
+  EXPECT_OUTPUT(Buf, "abc\n"
+                     "def\n"
+                     "ghi\n"
+                     "jkl\n");
+
+  // Remove "def\n".
+  //
+  // After the removal of "def", we have:
+  //
+  //   "abc\n"
+  //   "\n"
+  //   "ghi\n"
+  //   "jkl\n"
+  //
+  // Because removeLineIfEmpty=true, RemoveText has to remove the "\n" left on
+  // the line.  This happens correctly for the rewrite buffer itself, so the
+  // next check below passes.
+  //
+  // However, RemoveText's implementation incorrectly records the delta for
+  // removing the "\n" using the rewrite buffer offset, 4, where it was
+  // supposed to use the original input offset, 3.  Interpreted as an original
+  // input offset, 4 points to "g" not to "\n".  Thus, any future modifications
+  // at the original input's "g" will incorrectly see "g" as having become an
+  // empty string and so will map to the next character, "h", in the rewrite
+  // buffer.
+  StringRef RemoveStr0 = "def";
+  Buf.RemoveText(Input.find(RemoveStr0), RemoveStr0.size(),
+                 /*removeLineIfEmpty*/ true);
+  EXPECT_OUTPUT(Buf, "abc\n"
+                     "ghi\n"
+                     "jkl\n");
+
+  // Try to remove "ghi\n".
+  //
+  // As discussed above, the original input offset for "ghi\n" incorrectly
+  // maps to the rewrite buffer offset for "hi\nj", so we end up with:
+  //
+  //   "abc\n"
+  //   "gkl\n"
+  //
+  // To show that removeLineIfEmpty=true is the culprit, change true to false
+  // and append a newline to RemoveStr0 above.  The test then passes.
+  StringRef RemoveStr1 = "ghi\n";
+  Buf.RemoveText(Input.find(RemoveStr1), RemoveStr1.size());
+  EXPECT_OUTPUT(Buf, "abc\n"
+                     "jkl\n");
 }
 
 } // anonymous namespace