[YAML] Trim trailing whitespace from plain scalars

[rkayaith]

Summary

In some cases plain scalars are currently parsed with a trailing
newline. In particular this shows up often when parsing JSON files, e.g.
note the \n after 456 below:

$ cat test.yaml
{
  "foo": 123,
  "bar": 456
}
$ yaml-bench test.yaml -canonical
%YAML 1.2
---
!!map {
  ? !!str "foo"
  : !!str "123",
  ? !!str "bar"
  : !!str "456\n",
}
...

The trailing whitespace ends up causing the conversion of the scalar to
int/bool/etc. to fail, causing the issue seen here:
https://github.com/llvm/llvm-project/issues/15877

From reading the YAML spec (https://yaml.org/spec/1.2.2/#733-plain-style)
it seems like plain scalars should never end with whitespace, so this
change trims all trailing whitespace characters from the
value (specifically b-line-feed, b-carriage-return, s-space, and
s-tab).

[rkayaith]

llvm/lib/Support/YAMLParser.cpp

2044–2049

An alternative to this might be to change the scanner to match the spec more closely, but I'm not very familiar with the spec or that code so I couldn't tell if that would be straightforward to do.

[rkayaith]

update summary

[rkayaith]

Adding reviewers based on recent commits

[rkayaith]

@scott.linder ping

[scott.linder]

Sorry for the delay in reviewing! LGTM with a couple nits

llvm/lib/Support/YAMLParser.cpp

2043–2044

Nit: I believe this is just a stale comment from the partial implementation of block scalars before https://reviews.llvm.org/D9503

Could you remove the mention of "block" while you're at it?

2044–2048

Could you mention the alternative of handling this in the scanner? I am not certain this is the right place for this, but if it is moving us closer to the spec I think it is OK regardless. The YAML spec is so vast and impenetrable that I don't suspect we will ever be fully compliant anyway :)

[rkayaith]

update comments

[rkayaith]

In D137118#4116616, @scott.linder wrote:

Sorry for the delay in reviewing!

No worries, thanks for the review!

llvm/lib/Support/YAMLParser.cpp

if (i != StringRef::npos) {
         llvm::append_range(Storage, Valid);
         Storage.push_back('\'');
         UnquotedValue = UnquotedValue.substr(i + 2);
       }
       llvm::append_range(Storage, UnquotedValue);
       return StringRef(Storage.begin(), Storage.size());
     }
     return UnquotedValue;
   }
-  // Plain or block.
-  return Value.rtrim(' ');
+  // Plain.
+  // Trim whitespace ('b-char' and 's-white').
+  // NOTE: Alternatively we could change the scanner to not include whitespace
+  //       here in the first place.
+  return Value.rtrim("\x0A\x0D\x20\x09");
 }
 
 StringRef ScalarNode::unescapeDoubleQuoted( StringRef UnquotedValue
                                           , StringRef::size_type i
                                           , SmallVectorImpl<char> &Storage)
                                           const {
   // Use Storage to build proper value.
   Storage.clear();
   Storage.reserve(UnquotedValue.size());

llvm/test/YAMLParser/json.test

+# RUN: yaml-bench -canonical %s | FileCheck %s
+
+# CHECK: !!map {
+# CHECK:   ? !!str "foo"
+# CHECK:   : !!str "123",
+# CHECK:   ? !!str "bar"
+# CHECK:   : !!str "456",
+# CHECK: }
+
+{
+  "foo": 123,
+  "bar": 456
+}

llvm/unittests/Support/YAMLIOTest.cpp

TEST(YAMLIO, TestMapRead) {
   {
     Input yin("{foo: 3, bar: 5}");
     yin >> doc;
 
     EXPECT_FALSE(yin.error());
     EXPECT_EQ(doc.foo, 3);
     EXPECT_EQ(doc.bar, 5);
   }
+
+  {
+    Input yin("{\"foo\": 3\n, \"bar\": 5}");
+    yin >> doc;
+
+    EXPECT_FALSE(yin.error());
+    EXPECT_EQ(doc.foo, 3);
+    EXPECT_EQ(doc.bar, 5);
+  }
 }
 
 TEST(YAMLIO, TestMalformedMapRead) {
   FooBar doc;
   Input yin("{foo: 3; bar: 5}", nullptr, suppressErrorMessages);
   yin >> doc;
   EXPECT_TRUE(!!yin.error());
 }