[clang][Rewriter] patch to fix bug with ReplaceText erasing too many characters when text has been inserted before the replace location.

[ajohnson-uoregon]

Summary

See https://llvm.discourse.group/t/possible-bug-in-clang-rewriter-clang-rewritebuffer-and-or-clang-rewriterope/3970 for initial explanation of bug.

[clang][Rewriter] split test for replace after insert to be more understandable and also a fix to make the test correct

[ajohnson-uoregon]

Making clang-format happy

[ajohnson-uoregon]

making patch happy???

[jdenny]

Thanks for working on this. I agree there's a bug. However, this patch doesn't fix the bug. It provides a workaround.

That is, the current default ReplaceText behavior, as revealed in the first test added by this patch, doesn't make sense. IncludeInsertsAtBeginOfRange=true (the default) should mean to also remove the text that was previously inserted before the specified range. Instead, the previously inserted text confuses the implementation, so it removes extra text after the specified range. It's hard to imagine anyone is relying on that broken behavior.

I see a couple of potential paths forward. Either:

1. Change the patch summary and the new comments to make it clear that the default behavior is broken, so specifying IncludeInsertsAtBeginOfRange=false is the only way to ensure reasonable behavior. FIXME: should prefix some of those comments.

2. Fix the bug. I've added an inline comment to suggest a possible fix, but I haven't tested it carefully.

Either way, RemoveText should be addressed in the same manner as it has the same bug.

clang/lib/Rewrite/Rewriter.cpp

132

I think this might fix the bug, but I haven't tested thoroughly.

One problem is ensuring backward compatibility here and in callers while keeping a consistent interface. That is, the previous default here was IncludeInsertsAtBeginOfRange=false, but it seems the intended default elsewhere is meant to be the opposite.

[jdenny]

clang/lib/Rewrite/Rewriter.cpp

136

I think this needs to be fixed too. That is, the removal of the inserts requires updating the insert delta separately from the replace delta. Otherwise, a future getRewrittenText ending at OrigOffset will include extra characters.

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

bool ReplaceText(SourceLocation Start, unsigned OrigLength,
                    StringRef NewStr);
 
   /// ReplaceText - This method replaces a range of characters in the input
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation. To ensure correct computation of the size of the range to
   /// replace, pass RewriteOptions with IncludeInsertsAtBeginOfRange = false
   /// (unless different behavior is desired).
   bool ReplaceText(CharSourceRange range, StringRef NewStr,
                    RewriteOptions opts = RewriteOptions()) {
     return ReplaceText(range.getBegin(), getRangeSize(range, opts), NewStr);
   }
 
   /// ReplaceText - This method replaces a range of characters in the input
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation. To ensure correct computation of the size of the range to
   /// replace, pass RewriteOptions with IncludeInsertsAtBeginOfRange = false
   /// (unless different behavior is desired).
   bool ReplaceText(SourceRange range, StringRef NewStr,
                    RewriteOptions opts = RewriteOptions()) {
     return ReplaceText(range.getBegin(), getRangeSize(range, opts), NewStr);
   }
 
   /// ReplaceText - This method replaces a range of characters in the input
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation. To ensure correct computation of the size of the ranges,
   /// pass RewriteOptions with IncludeInsertsAtBeginOfRange = false (unless
   /// different behavior is desired).
   bool ReplaceText(SourceRange range, SourceRange replacementRange,
                    RewriteOptions opts = RewriteOptions());
 
   /// Increase indentation for the lines between the given source range.
   /// To determine what the indentation should be, 'parentIndent' is used
   /// that should be at a source location with an indentation one degree
   /// lower than the given range.
   bool IncreaseIndentation(CharSourceRange range, SourceLocation parentIndent);
   bool IncreaseIndentation(SourceRange range, SourceLocation parentIndent) {
     return IncreaseIndentation(CharSourceRange::getTokenRange(range),

clang/lib/Rewrite/Rewriter.cpp

void RewriteBuffer::InsertText(unsigned OrigOffset, StringRef Str,
   AddInsertDelta(OrigOffset, Str.size());
 }
 
 /// ReplaceText - This method replaces a range of characters in the input
 /// buffer with a new string.  This is effectively a combined "remove+insert"
 /// operation.
 void RewriteBuffer::ReplaceText(unsigned OrigOffset, unsigned OrigLength,
                                 StringRef NewStr) {
   unsigned RealOffset = getMappedOffset(OrigOffset, true);
   Buffer.erase(RealOffset, OrigLength);
   Buffer.insert(RealOffset, NewStr.begin(), NewStr.end());
   if (OrigLength != NewStr.size())
     AddReplaceDelta(OrigOffset, NewStr.size() - OrigLength);
 }
 
 //===----------------------------------------------------------------------===//
 // Rewriter class
 //===----------------------------------------------------------------------===//
 
 /// getRangeSize - Return the size in bytes of the specified range if they
 /// are in the same file.  If not, this returns -1.
bool Rewriter::ReplaceText(SourceLocation Start, unsigned OrigLength,
   if (!isRewritable(Start)) return true;
   FileID StartFileID;
   unsigned StartOffs = getLocationOffsetAndFileID(Start, StartFileID);
 
   getEditBuffer(StartFileID).ReplaceText(StartOffs, OrigLength, NewStr);
   return false;
 }
 
-bool Rewriter::ReplaceText(SourceRange range, SourceRange replacementRange, RewriteOptions opts) {
+bool Rewriter::ReplaceText(SourceRange range, SourceRange replacementRange,
+                           RewriteOptions opts) {
   if (!isRewritable(range.getBegin())) return true;
   if (!isRewritable(range.getEnd())) return true;
   if (replacementRange.isInvalid()) return true;
   SourceLocation start = range.getBegin();
   unsigned origLength = getRangeSize(range, opts);
   unsigned newLength = getRangeSize(replacementRange, opts);
   FileID FID;
   unsigned newOffs = getLocationOffsetAndFileID(replacementRange.getBegin(),

clang/unittests/Rewrite/RewriterTest.cpp

TEST(Rewriter, ReplaceTextRangeTypes) {
   T.Rewrite.ReplaceText(T.CRange, "foo");
   EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "foogc;");
   T.Rewrite.ReplaceText(T.TRange, "bar");
   EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "bar;");
   T.Rewrite.ReplaceText(T.SRange, "0");
   EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "0;");
 }
 
-TEST(Rewriter, ReplaceAfterInsert) {
+TEST(Rewriter, ReplaceAfterInsertNoOpts) {
   // Check that remove/insert after insert behaves correctly based on the
-  // RewriteOptions passed.
+  // RewriteOptions passed. This should erase extra text.
 
-  StringRef code = "int main(int argc, char *argv[]) { return argc; } double f(int text_to_erase) {}";
-  //                                                  ^ insert "int answer = 42;"
-  //                                                          ^ replace with answer
-  //                                                          ^ replace with 42
-  //                                                       extra text to erase ^
+  StringRef code = "int main(int argc, char *argv[]) { return argc; } double "
+                   "f(int text_to_erase) {}";
+  //                        insert "int answer = 42;" ^
+  //                                  replace with 42 ^
+  //                                         extra text to erase ^
   std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCode(code);
   ASTContext &C = AST->getASTContext();
-  Rewriter Rewrite = Rewriter(C.getSourceManager(), C.getLangOpts());;
+  Rewriter Rewrite = Rewriter(C.getSourceManager(), C.getLangOpts());
   SourceLocation FileStart = AST->getStartOfMainFileID();
   SourceLocation insert_point = FileStart.getLocWithOffset(34);
   Rewrite.InsertText(insert_point, "int answer = 42;");
-  SourceRange replace_range = SourceRange(FileStart.getLocWithOffset(42),
+  SourceRange replace_range = SourceRange(FileStart.getLocWithOffset(34),
                                           FileStart.getLocWithOffset(45));
+  Rewrite.ReplaceText(replace_range, "42;");
+  EXPECT_EQ(
+      Rewrite.getRewrittenText(SourceRange(FileStart.getLocWithOffset(33),
+                                           FileStart.getLocWithOffset(65))),
+      "{int answer = 42;42; text_to_erase");
+}
+
+TEST(Rewriter, ReplaceAfterInsertOpts) {
+  // Check that remove/insert after insert behaves correctly based on the
+  // RewriteOptions passed. This should NOT erase extra text.
+
+  StringRef code = "int main(int argc, char *argv[]) { return argc; } double "
+                   "f(int text_to_erase) {}";
+  //                        insert "int answer = 42;" ^
+  //                                  replace with 42 ^
+  //                                         extra text to erase ^
+  std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCode(code);
+  ASTContext &C = AST->getASTContext();
+  Rewriter Rewrite = Rewriter(C.getSourceManager(), C.getLangOpts());
+  SourceLocation FileStart = AST->getStartOfMainFileID();
+  SourceLocation insert_point = FileStart.getLocWithOffset(34);
+  Rewrite.InsertText(insert_point, "int answer = 42;");
+  SourceRange replace_range = SourceRange(FileStart.getLocWithOffset(34),
+                                          FileStart.getLocWithOffset(46));
   Rewriter::RewriteOptions opts;
   opts.IncludeInsertsAtBeginOfRange = false;
-  Rewrite.ReplaceText(replace_range, "answer", opts);
-  EXPECT_EQ(Rewrite.getRewrittenText(
-      SourceRange(FileStart.getLocWithOffset(33), FileStart.getLocWithOffset(48))),
-    "{int answer = 42; return answer; }");
-  Rewrite.ReplaceText(replace_range, "42", Rewriter::RewriteOptions());
-  EXPECT_EQ(Rewrite.getRewrittenText(
-      SourceRange(FileStart.getLocWithOffset(33), FileStart.getLocWithOffset(55))),
-    "{int answer = 42; return 42; } double");
+  Rewrite.ReplaceText(replace_range, "42;", opts);
+  EXPECT_EQ(
+      Rewrite.getRewrittenText(SourceRange(FileStart.getLocWithOffset(33),
+                                           FileStart.getLocWithOffset(58))),
+      "{int answer = 42;42; } double f(");
 }
 
-
 } // anonymous namespace