[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

23

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

23

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-tools-extra/clang-apply-replacements/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangApplyReplacements
   lib/Tooling/ApplyReplacements.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangRewrite
+  clangTooling
   clangToolingCore
   clangToolingRefactoring
   )
 
 include_directories(
   ${CMAKE_CURRENT_SOURCE_DIR}
   include
   )
 add_subdirectory(tool)

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())) {}
+
+    std::string escape(StringRef Str);
+
+    std::string unescape(StringRef Str);
 
     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/lib/Tooling/CMakeLists.txt

add_clang_library(clangTooling
   ExpandResponseFilesCompilationDatabase.cpp
   FileMatchTrie.cpp
   FixIt.cpp
   GuessTargetAndModeCompilationDatabase.cpp
   InterpolatingCompilationDatabase.cpp
   JSONCompilationDatabase.cpp
   Refactoring.cpp
   RefactoringCallbacks.cpp
+  ReplacementsYaml.cpp
   StandaloneExecution.cpp
   Tooling.cpp
 
   DEPENDS
   ClangDriverOptions
 
   LINK_LIBS
   clangAST

clang/lib/Tooling/Refactoring/CMakeLists.txt

add_clang_library(clangToolingRefactoring
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangIndex
   clangLex
   clangRewrite
+  clangTooling
   clangToolingCore
   )

clang/lib/Tooling/ReplacementsYaml.cpp

+//===-- ReplacementsYaml.cpp -- Serialiazation for Replacements -----------===//
+//
+// 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/Tooling/ReplacementsYaml.h"
+
+namespace llvm {
+namespace yaml {
+
+namespace {
+struct Escapes {
+  char EscapeChar;
+  char WrittenChar;
+};
+} // namespace
+
+static constexpr Escapes EscapeChars[] = {
+    {'\\', '\\'}, {'\a', 'a'}, {'\b', 'b'}, {'\f', 'f'},
+    {'\n', 'n'},  {'\r', 'r'}, {'\t', 't'}, {'\v', 'v'}};
+
+std::string
+MappingTraits<clang::tooling::Replacement>::NormalizedReplacement::escape(
+    StringRef Str) {
+  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
+MappingTraits<clang::tooling::Replacement>::NormalizedReplacement::unescape(
+    StringRef Str) {
+  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;
+}
+
+} // namespace yaml
+} // namespace llvm

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, "'\\ \a \b \f \n \r \t \v");
 
-  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_FALSE(IYAML.error());
+  ASSERT_EQ(Doc.MainSourceFile, NewDoc.MainSourceFile);
+  ASSERT_EQ(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"
TEST(ReplacementsYamlTest, deserializesWithoutContext) {
   ASSERT_FALSE(YAML.error());
   ASSERT_EQ("/path/to/source.cpp", DocActual.MainSourceFile);
   ASSERT_EQ(1u, DocActual.Replacements.size());
   ASSERT_EQ("target_file.h", DocActual.Replacements[0].getFilePath());
   ASSERT_EQ(1u, DocActual.Replacements[0].getOffset());
   ASSERT_EQ(10u, DocActual.Replacements[0].getLength());
   ASSERT_EQ("replacement", DocActual.Replacements[0].getReplacementText());
 }
+
+TEST(ReplacementsYamlTest, deserializesEscapedReplacements) {
+  StringRef YamlContent =
+      "---\n"
+      "MainSourceFile:      /path/to/source.cpp\n"
+      "Replacements:\n"
+      "  - FilePath:        /path/to/file1.h\n"
+      "    Offset:          232\n"
+      "    Length:          56\n"
+      "    ReplacementText: '\\\\ \\a \\b \\f \\n \\r \\t \\v'\n"
+      "...\n";
+  TranslationUnitReplacements DocActual;
+  yaml::Input YAML(YamlContent);
+  YAML >> DocActual;
+  ASSERT_FALSE(YAML.error());
+  ASSERT_EQ(1u, DocActual.Replacements.size());
+  ASSERT_EQ("/path/to/source.cpp", DocActual.MainSourceFile);
+  ASSERT_EQ("/path/to/file1.h", DocActual.Replacements[0].getFilePath());
+  ASSERT_EQ(232u, DocActual.Replacements[0].getOffset());
+  ASSERT_EQ(56u, DocActual.Replacements[0].getLength());
+  ASSERT_EQ("\\ \a \b \f \n \r \t \v",
+            DocActual.Replacements[0].getReplacementText());
+}
+
+TEST(ReplacementsYamlTest, serializesEscapingReplacements) {
+
+  TranslationUnitReplacements Doc;
+
+  Doc.MainSourceFile = "/path/to/source.cpp";
+  Doc.Replacements.emplace_back("/path/to/file1.h", 232, 56,
+                                "\\ \a \b \f \n \r \t \v");
+
+  SmallString<256> YamlContent;
+  llvm::raw_svector_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << Doc;
+
+  // NOTE: If this test starts to fail for no obvious reason, check whitespace.
+  ASSERT_EQ(
+      StringRef("---\n"
+                "MainSourceFile:  '/path/to/source.cpp'\n"
+                "Replacements:\n"
+                "  - FilePath:        '/path/to/file1.h'\n"
+                "    Offset:          232\n"
+                "    Length:          56\n"
+                "    ReplacementText: '\\\\ \\a \\b \\f \\n \\r \\t \\v'\n"
+                "...\n"),
+      YamlContentStream.str());
+}