[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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@369049 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Rewrite/Core/Rewriter.h

@@ -46,6 +46,17 @@ public:
 
     /// If true and removing some text leaves a blank line
     /// also remove the empty line (false by default).
+    ///
+    /// FIXME: This sometimes corrupts the file's rewrite buffer due to
+    /// incorrect indexing in the implementation (see the FIXME in
+    /// clang::RewriteBuffer::RemoveText).  Moreover, it's inefficient because
+    /// it must scan the buffer from the beginning to find the start of the
+    /// line.  When feasible, it's better for the caller to check for a blank
+    /// line and then, if found, expand the removal range to include it.
+    /// Checking for a blank line is easy if, for example, the caller can
+    /// guarantee this is the first edit of a line.  In that case, it can just
+    /// scan before and after the removal range until the next newline or
+    /// begin/end of the input.
     bool RemoveLineIfEmpty = false;
 
     RewriteOptions() {}

lib/Rewrite/Rewriter.cpp

@@ -96,6 +96,17 @@ void RewriteBuffer::RemoveText(unsigned OrigOffset, unsigned Size,
     }
     if (posI != end() && *posI == '\n') {
       Buffer.erase(curLineStartOffs, lineSize + 1/* + '\n'*/);
+      // FIXME: Here, the offset of the start of the line is supposed to be
+      // expressed in terms of the original input not the "real" rewrite
+      // buffer.  How do we compute that reliably?  It might be tempting to use
+      // curLineStartOffs + OrigOffset - RealOffset, but that assumes the
+      // difference between the original and real offset is the same at the
+      // removed text and at the start of the line, but that's not true if
+      // edits were previously made earlier on the line.  This bug is also
+      // documented by a FIXME on the definition of
+      // clang::Rewriter::RewriteOptions::RemoveLineIfEmpty.  A reproducer for
+      // the implementation below is the test RemoveLineIfEmpty in
+      // clang/unittests/Rewrite/RewriteBufferTest.cpp.
       AddReplaceDelta(curLineStartOffs, -(lineSize + 1/* + '\n'*/));
     }
   }

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