[clang] tooling replacements are escaped when exporting to YAML

[njames93]

Summary

Escapes replacement text when exporting to yaml and unescapes when importing from yaml.

[njames93]

• Moved escape/unescape impl to source file

[njames93]

• Small tidy of code

[njames93]

• Fix test case not being ran

[njames93]

clang/unittests/Tooling/ReplacementsYamlTest.cpp

68

Can this ever be false. Does execution of a test case stop once an ASSERT_EQ fails

[njames93]

• Fix broken dependencies

[AlexanderLanin]

clang/lib/Tooling/ReplacementsYaml.cpp

22 ↗

(On Diff #249875)

Just so I have asked ;-)
Escaping every \ would be incorrect? Basically duplicate every '\'.

clang/unittests/Tooling/ReplacementsYamlTest.cpp

55

I think it would be worthwhile to test other characters as well.
50% of that would be purely for documentation purposes. What would happen when you escape \x and unescape \\x?

68

Yes, assert stops the test case. After the assert you can safely assume they are identical.

76

I assume this kind of test would have been green even without your change? Or would it fail?
You are testing that it is reconstructed correctly (which is indeed the main point), but not the escaping and unescaping.
You should probably test a concrete example with(escaped text, expected escaped test).

[njames93]

• Fix another broken dependency

[njames93]

• Extend tests

clang/lib/Tooling/ReplacementsYaml.cpp

22 ↗

(On Diff #249875)

You need to escape the escape character to avoid ambiguity when unescaping later

Say the code has the raw string which is \ followed by n. it would be escaped as \n.
Then when it comes to unescape it will read \n as a newline.
Escaping the \ leads to the output being \\n which will be read back as a \ followed by n.

clang/unittests/Tooling/ReplacementsYamlTest.cpp

55

I have added all the c++ escape characters apart from \' as that is handled in the yaml string parser anyway and \" as that seems to be ignore anyway

76

Yes this should've passed before however the issue was one more of readability. Things like the old double newline just look confusing whereas every programmer knows that '\n' is code for newline.
I have planned to add that in.

[AlexanderLanin]

Fine for me. Fixes newline bug in https://bugs.llvm.org/show_bug.cgi?id=45150.
However I don't have "review privileges" here.

clang/include/clang/Tooling/ReplacementsYaml.h

template <> struct MappingTraits<clang::tooling::Replacement> {
   /// Helper to (de)serialize a Replacement since we don't have direct
   /// access to its data members.
   struct NormalizedReplacement {
     NormalizedReplacement(const IO &)
         : FilePath(""), Offset(0), Length(0), ReplacementText("") {}
 
     NormalizedReplacement(const IO &, const clang::tooling::Replacement &R)
         : FilePath(R.getFilePath()), Offset(R.getOffset()),
-          Length(R.getLength()), ReplacementText(R.getReplacementText()) {
-      size_t lineBreakPos = ReplacementText.find('\n');
-      while (lineBreakPos != std::string::npos) {
-        ReplacementText.replace(lineBreakPos, 1, "\n\n");
-        lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+          Length(R.getLength()),
+          ReplacementText(escape(R.getReplacementText())) {}
+
+    struct Escapes {
+      char EscapeChar;
+      char WrittenChar;
+    };
+
+    std::string escape(StringRef Str) {
+      constexpr Escapes EscapeChars[] = {
+          {'\n', 'n'}, {'\r', 'r'}, {'\t', 't'}, {'\\', '\\'}};
+      std::string Res;
+      llvm::raw_string_ostream Builder(Res);
+      for (char C : Str) {
+        if (llvm::none_of(EscapeChars, [&](Escapes Escape) {
+              if (C == Escape.EscapeChar) {
+                Builder << '\\' << Escape.WrittenChar;
+                return true;
+              }
+              return false;
+            })) {
+          Builder << C;
+        }
+      }
+      return Res;
+    }
+
+    std::string unescape(StringRef Str) {
+      constexpr Escapes EscapeChars[] = {
+          {'\n', 'n'}, {'\r', 'r'}, {'\t', 't'}, {'\\', '\\'}};
+      std::string Res;
+      llvm::raw_string_ostream Builder(Res);
+      while (!Str.empty()) {
+        if (Str.consume_front("\\")) {
+          if (Str.empty()) {
+            Builder << '\\';
+            break;
+          }
+          char C = Str.front();
+          Str = Str.drop_front();
+          if (llvm::none_of(EscapeChars, [&](Escapes Escape) {
+                if (C == Escape.WrittenChar) {
+                  Builder << Escape.EscapeChar;
+                  return true;
+                }
+                return false;
+              })) {
+            Builder << '\\' << C;
+          }
+          continue;
+        }
+        Builder << Str.front();
+        Str = Str.drop_front();
       }
+      return Res;
     }
 
     clang::tooling::Replacement denormalize(const IO &) {
       return clang::tooling::Replacement(FilePath, Offset, Length,
-                                         ReplacementText);
+                                         unescape(ReplacementText));
     }
 
     std::string FilePath;
     unsigned int Offset;
     unsigned int Length;
     std::string ReplacementText;
   };
 

clang/unittests/Tooling/ReplacementsYamlTest.cpp

ASSERT_STREQ("---\n"
                "  - FilePath:        '/path/to/file2.h'\n"
                "    Offset:          301\n"
                "    Length:          2\n"
                "    ReplacementText: 'replacement #2'\n"
                "...\n",
                YamlContentStream.str().c_str());
 }
 
-TEST(ReplacementsYamlTest, serializesNewLines) {
-  TranslationUnitReplacements Doc;
+TEST(ReplacementsYamlTest, handlesEscaped) {
+  TranslationUnitReplacements Doc, NewDoc;
 
   Doc.MainSourceFile = "/path/to/source.cpp";
-  Doc.Replacements.emplace_back("/path/to/file1.h", 0, 0, "#include <utility>\n");
+  const StringRef FilePath = "/path/to/file1.h";
+  Doc.Replacements.emplace_back(FilePath, 0, 0, "#include <utility>\n");
+  Doc.Replacements.emplace_back(FilePath, 0, 0, "'\\\t\r\n");
 
-  std::string YamlContent;
-  llvm::raw_string_ostream YamlContentStream(YamlContent);
+  SmallString<512> YamlContent;
+  llvm::raw_svector_ostream YamlContentStream(YamlContent);
 
   yaml::Output YAML(YamlContentStream);
   YAML << Doc;
+  yaml::Input IYAML(YamlContent);
+  IYAML >> NewDoc;
 
-  // NOTE: If this test starts to fail for no obvious reason, check whitespace.
-  ASSERT_STREQ("---\n"
-               "MainSourceFile:  '/path/to/source.cpp'\n"
-               "Replacements:\n"
-               "  - FilePath:        '/path/to/file1.h'\n"
-               "    Offset:          0\n"
-               "    Length:          0\n"
-               "    ReplacementText: '#include <utility>\n\n'\n"
-               "...\n",
-               YamlContentStream.str().c_str());
+  ASSERT_EQ(Doc.MainSourceFile, NewDoc.MainSourceFile);
+  ASSERT_EQ(Doc.Replacements.size(), NewDoc.Replacements.size());
+  if (Doc.Replacements.size() != NewDoc.Replacements.size()) {
+    for (auto DocR = Doc.Replacements.begin(),
+              NewDocR = NewDoc.Replacements.begin(),
+              DocE = Doc.Replacements.end();
+         DocR != DocE; DocR++, NewDocR++) {
+      ASSERT_EQ(DocR->getFilePath(), NewDocR->getFilePath());
+      ASSERT_EQ(DocR->getLength(), NewDocR->getLength());
+      ASSERT_EQ(DocR->getOffset(), NewDocR->getOffset());
+      ASSERT_EQ(DocR->getReplacementText(), NewDocR->getReplacementText());
+    }
+  }
 }
 
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
                             "MainSourceFile:      /path/to/source.cpp\n"
                             "Replacements:\n"
                             "  - FilePath:        /path/to/file1.h\n"
                             "    Offset:          232\n"