[yaml][clang-tidy] Fix multiline YAML serialization

[DmitryPolukhin]

Summary

Newline duplication logic introduced in https://reviews.llvm.org/D63482
has two issues: (1) there is no logic that removes duplicate newlines
when clang-apply-replacment reads YAML and (2) in general such logic
should be applied to all strings and should happen on string
serialization level instead in YAML parser.

This diff changes multiline strings quotation from single quote ' to
double ". It solves problems with internal newlines because now they are
escaped. Also double quotation solves the problem with leading whitespace after
newline. In case of single quotation YAML parsers should remove leading
whitespace according to specification. In case of double quotation these
leading are internal space and they are preserved. There is no way to
instruct YAML parsers to preserve leading whitespaces after newline so
double quotation is the only viable option that solves all problems at
once.

[DmitryPolukhin]

Fix clang-tidy warnings

[DmitryPolukhin]

Fixed second string after new line + more test case

[mgehre]

Thanks for doing this! I didn't work on the YAML parser before, so I cannot give formal approval.

llvm/lib/Support/YAMLTraits.cpp

888

I wonder whether using StringRef::split() would lead to an easier implementation (https://llvm.org/doxygen/classllvm_1_1StringRef.html#af0284e4c41c0e09c0bc4767bc77a899d)

[DmitryPolukhin]

+ @gribozavr, @Eugene.Zelenko, @thegameg who touched/reviewed this code, please take a look.

llvm/lib/Support/YAMLTraits.cpp

888

I'm not sure that it will be easier to read or more efficient (StringRef::split will require additional vector).

[mgehre]

llvm/lib/Support/YAMLTraits.cpp

888

I don't expect the difference in efficiency to be noticable, but the code would look like

SmallVector<StringRef, 2> Lines;
StringRef(Val).split(Lines, '\n');
bool First = true;
for (StringRef Line: Lines) {
  if (First)
    First = false;
  else
    Out << '\n';
  Out << Line;
}

which I personally find easier to follow than CurrentPos, LineBreakPos and their arithmetic.

[DmitryPolukhin]

Apply suggested changes with string split

[njames93]

llvm/lib/Support/YAMLTraits.cpp

888

If you want to do the same here...

SmallVector<StringRef, 8> Lines;
Scalar.split(Lines, "\n\n");
Val = llvm::join(Lines, "\n");
return StringRef();

[DmitryPolukhin]

Rewrite input string split too

[DmitryPolukhin]

Fix spelling

[DmitryPolukhin]

llvm/lib/Support/YAMLTraits.cpp

888

Personally I don't like approach with string split because it is potentially less efficient due to memory allocations in case of multiple newlines that can be avoided. BUT I would like to move the needle and fix this real bug in clang ASAP. So I'm ready to rewrite it to something that (1) works and (2) moves review forward. So thank you for review and please take another look.

[DmitryPolukhin]

Fix single new line handling, it should be replace with space

[DmitryPolukhin]

Rebase

[DmitryPolukhin]

Rebase

[DmitryPolukhin]

@njames93 - friendly ping, could you please take another look.

[DmitryPolukhin]

It looks like there is no support for the proposed solution so I found alternative solution that might be even better. We can use double quotation " for multiline strings. It solves problem because in case of double quotation LLVM escapes new line like \n so there is no need to double newlines. Escaped newlines can be parsed correctly by other YAML parsers like pyyaml. BTW, LLVM YAML reading also has issue with not removing leading spaces for multiline strings so multiline strings serialised by pyyaml with single quotation cannot be parsed correctly by clang-apply-replacements. With double quotation it seems to work fine in both directions.

What do you think about using double quotation for multiline strings?

[DmitryPolukhin]

Use double quotation for multiline strings. It solves problems with internal newlines because now they are escaped. Also double quotation solves the problem with leading whitespace after newline. In case of single quotation YAML parsers should remove leading whitespace according to specification. In case of double quotation these leading are internal space and they are preserved. There is no way to instruct YAML parsers to preserve leading whitespaces after newline so double quotation is the only viable option that solves all problems at once.

[DmitryPolukhin]

Fix test

[DmitryPolukhin]

@njames93 and @aaron.ballman - please take a look to this diff. Multiline replacements in YAML are broken and cannot be applied correctly.

[aaron.ballman]

LGTM, this seems like a pretty clean solution. Thanks!

[DmitryPolukhin]

@aaron.ballman - thank you for the review!

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(R.getReplacementText()) {}
 
     clang::tooling::Replacement denormalize(const IO &) {
       return clang::tooling::Replacement(FilePath, Offset, Length,
                                          ReplacementText);
     }
 
     std::string FilePath;
     unsigned int Offset;

clang/unittests/Tooling/ReplacementsYamlTest.cpp

TEST(ReplacementsYamlTest, serializesNewLines) {
 
   // 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"
+               "    ReplacementText: \"#include <utility>\\n\"\n"
                "...\n",
                YamlContentStream.str().c_str());
 }
 
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
                             "MainSourceFile:      /path/to/source.cpp\n"
                             "Replacements:\n"

llvm/include/llvm/Support/YAMLTraits.h

 // 5.1. Character Set
 // The allowed character range explicitly excludes the C0 control block #x0-#x1F
 // (except for TAB #x9, LF #xA, and CR #xD which are allowed), DEL #x7F, the C1
 // control block #x80-#x9F (except for NEL #x85 which is allowed), the surrogate
 // block #xD800-#xDFFF, #xFFFE, and #xFFFF.
 inline QuotingType needsQuotes(StringRef S) {
   if (S.empty())
     return QuotingType::Single;
+
+  QuotingType MaxQuotingNeeded = QuotingType::None;
   if (isSpace(static_cast<unsigned char>(S.front())) ||
       isSpace(static_cast<unsigned char>(S.back())))
-    return QuotingType::Single;
+    MaxQuotingNeeded = QuotingType::Single;
   if (isNull(S))
-    return QuotingType::Single;
+    MaxQuotingNeeded = QuotingType::Single;
   if (isBool(S))
-    return QuotingType::Single;
+    MaxQuotingNeeded = QuotingType::Single;
   if (isNumeric(S))
-    return QuotingType::Single;
+    MaxQuotingNeeded = QuotingType::Single;
 
   // 7.3.3 Plain Style
   // Plain scalars must not begin with most indicators, as this would cause
   // ambiguity with other YAML constructs.
   static constexpr char Indicators[] = R"(-?:\,[]{}#&*!|>'"%@`)";
   if (S.find_first_of(Indicators) == 0)
-    return QuotingType::Single;
+    MaxQuotingNeeded = QuotingType::Single;
 
-  QuotingType MaxQuotingNeeded = QuotingType::None;
   for (unsigned char C : S) {
     // Alphanum is safe.
     if (isAlnum(C))
       continue;
 
     switch (C) {
     // Safe scalar characters.
     case '_':
     case '-':
     case '^':
     case '.':
     case ',':
     case ' ':
     // TAB (0x9) is allowed in unquoted strings.
     case 0x9:
       continue;
     // LF(0xA) and CR(0xD) may delimit values and so require at least single
-    // quotes.
+    // quotes. LLVM YAML parser cannot handle single quoted multiline so use
+    // double quoting to produce valid YAML.
     case 0xA:
     case 0xD:
-      MaxQuotingNeeded = QuotingType::Single;
-      continue;
+      return QuotingType::Double;
     // DEL (0x7F) are excluded from the allowed character range.
     case 0x7F:
       return QuotingType::Double;
     // Forward slash is allowed to be unquoted, but we quote it anyway.  We have
     // many tests that use FileCheck against YAML output, and this output often
     // contains paths.  If we quote backslashes but not forward slashes then
     // paths will come out either quoted or unquoted depending on which platform
     // the test is run on, making FileCheck comparisons difficult.

llvm/lib/Support/YAMLTraits.cpp

 
 StringRef ScalarTraits<StringRef>::input(StringRef Scalar, void *,
                                          StringRef &Val) {
   Val = Scalar;
   return StringRef();
 }
 
 void ScalarTraits<std::string>::output(const std::string &Val, void *,
                                        raw_ostream &Out) {
   Out << Val;
 }
 
 StringRef ScalarTraits<std::string>::input(StringRef Scalar, void *,
                                            std::string &Val) {
   Val = Scalar.str();
   return StringRef();
 }
 
 void ScalarTraits<uint8_t>::output(const uint8_t &Val, void *,
                                    raw_ostream &Out) {
   // use temp uin32_t because ostream thinks uint8_t is a character
   uint32_t Num = Val;
   Out << Num;
 }

llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll

 ; YAML-NEXT:  Args:
 ; YAML-NEXT:    - String:          'Lowered with '
 ; YAML-NEXT:    - NumStores:       '4'
 ; YAML-NEXT:    - String:          ' stores, '
 ; YAML-NEXT:    - NumLoads:        '0'
 ; YAML-NEXT:    - String:          ' loads, '
 ; YAML-NEXT:    - NumComputeOps:   '0'
 ; YAML-NEXT:    - String:          ' compute ops'
-; YAML-NEXT:    - String:          ',
-; YAML-NEXT:  additionally '
+; YAML-NEXT:    - String:          ",\nadditionally "
 ; YAML-NEXT:    - NumStores:       '0'
 ; YAML-NEXT:    - String:          ' stores, '
 ; YAML-NEXT:    - NumLoads:        '4'
 ; YAML-NEXT:    - String:          ' loads, '
 ; YAML-NEXT:    - NumFPOps:        '16'
 ; YAML-NEXT:    - String:          ' compute ops'
 ; YAML-NEXT:    - String:          ' are shared with other expressions'
 ; YAML-NEXT:    - String:           |

 ; YAML-NEXT:  Args:
 ; YAML-NEXT:    - String:          'Lowered with '
 ; YAML-NEXT:    - NumStores:       '30'
 ; YAML-NEXT:    - String:          ' stores, '
 ; YAML-NEXT:    - NumLoads:        '45'
 ; YAML-NEXT:    - String:          ' loads, '
 ; YAML-NEXT:    - NumComputeOps:   '120'
 ; YAML-NEXT:    - String:          ' compute ops'
-; YAML-NEXT:    - String:          ',
-; YAML-NEXT:  additionally '
+; YAML-NEXT:    - String:          ",\nadditionally "
 ; YAML-NEXT:    - NumStores:       '0'
 ; YAML-NEXT:    - String:          ' stores, '
 ; YAML-NEXT:    - NumLoads:        '4'
 ; YAML-NEXT:    - String:          ' loads, '
 ; YAML-NEXT:    - NumFPOps:        '16'
 ; YAML-NEXT:    - String:          ' compute ops'
 ; YAML-NEXT:    - String:          ' are shared with other expressions'
 ; YAML-NEXT:    - String:           |

llvm/unittests/Support/YAMLIOTest.cpp

TEST(YAMLIO, MultilineStrings) {
 
   std::string Serialized;
   {
     llvm::raw_string_ostream OS(Serialized);
     Output YOut(OS);
     YOut << Original;
   }
   auto Expected = "---\n"
-                  "str1:            'a multiline string\n"
-                  "foobarbaz'\n"
-                  "str2:            'another one\r"
-                  "foobarbaz'\n"
+                  "str1:            \"a multiline string\\nfoobarbaz\"\n"
+                  "str2:            \"another one\\rfoobarbaz\"\n"
                   "str3:            a one-line string\n"
                   "...\n";
   ASSERT_EQ(Serialized, Expected);
 
   // Also check it parses back without the errors.
   WithStringField Deserialized;
   {
     Input YIn(Serialized);