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

[jdenny]

Summary

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.

[jdenny]

Rebase, and add a unit test to demonstrate the bug.

[jdenny]

Ping.

[jdenny]

Ping.

[jkorous]

Hi @jdenny, thanks for the warning! I think having the test disabled is fine - the main upside I see is that we won't check it fails over and over on our CI systems.
Could you please mention the test/reproducer in those FIXMEs?
Otherwise LGTM.

clang/unittests/Rewrite/RewriteBufferTest.cpp

17

I think in case you really wanted to have an "XFAIL", you could have just used EXPECT_NE here.

[jdenny]

In D61466#1602917, @jkorous wrote:

Hi @jdenny, thanks for the warning!

Hi. Thanks for the review.

I think having the test disabled is fine - the main upside I see is that we won't check it fails over and over on our CI systems.

In an inline comment, you also mentioned the alternative of replacing EXPECT_EQ with EXPECT_NE. Neither solution is the XFAIL I'm used to (from lit for example).

I chose disabling purely because I saw some other unit tests do this. What do people normally recommend for llvm unit tests?

Could you please mention the test/reproducer in those FIXMEs?

Will do.

[jkorous]

In D61466#1602928, @jdenny wrote:

In D61466#1602917, @jkorous wrote:

In an inline comment, you also mentioned the alternative of replacing EXPECT_EQ with EXPECT_NE. Neither solution is the XFAIL I'm used to (from lit for example).

I'm not aware of any assertion with this semantics in Google Test.

[jdenny]

Note the reproducer in the implementation's FIXME. The other FIXME points there, so I figure it's best not to duplicate the note.

[jdenny]

In D61466#1613571, @jdenny wrote:

Note the reproducer in the implementation's FIXME. The other FIXME points there, so I figure it's best not to duplicate the note.

@jkorous, is this change sufficient? Thanks.

[jkorous]

LGTM. Thanks!

clang/include/clang/Rewrite/Core/Rewriter.h

struct RewriteOptions {
     bool IncludeInsertsAtBeginOfRange = true;
 
     /// Given a source range, true to include previous inserts at the
     /// end of the range as part of the range itself (true by default).
     bool IncludeInsertsAtEndOfRange = true;
 
     /// 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() {}
   };
 
   using buffer_iterator = std::map<FileID, RewriteBuffer>::iterator;
   using const_buffer_iterator = std::map<FileID, RewriteBuffer>::const_iterator;
 

clang/lib/Rewrite/Rewriter.cpp

if (removeLineIfEmpty) {
     unsigned lineSize = 0;
     posI = curLineStart;
     while (posI != end() && isWhitespaceExceptNL(*posI)) {
       ++posI;
       ++lineSize;
     }
     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'*/));
     }
   }
 }
 
 void RewriteBuffer::InsertText(unsigned OrigOffset, StringRef Str,
                                bool InsertAfter) {
   // Nothing to insert, exit early.

clang/unittests/Rewrite/RewriteBufferTest.cpp

 //===- unittests/Rewrite/RewriteBufferTest.cpp - RewriteBuffer tests ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 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;
   raw_string_ostream(BeginTag) << '<' << tagName << '>';
   std::string EndTag;
   raw_string_ostream(EndTag) << "</" << tagName << '>';
 
   Buf.InsertTextAfter(Offset, BeginTag);
TEST(RewriteBuffer, TagRanges) {
   size_t Pos = Input.find(RemoveStr);
   Buf.RemoveText(Pos, RemoveStr.size());
 
   StringRef TagStr = "hello";
   Pos = Input.find(TagStr);
   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