From 4c407017f3b5ba221823c86c8fd68a00efaa689f Mon Sep 17 00:00:00 2001
From: Sam McCall <sam.mccall@gmail.com>
Date: Fri, 9 Oct 2020 15:33:56 +0200
Subject: [PATCH] [JSON] Add ObjectMapper::mapOptional to validate optional
 data.

Currently the idiom for mapping optional fields is:
  ObjectMapper O(Val, P);
  if (!O.map("required1", Out.R1) || !O.map("required2", Out.R2))
    return false;
  O.map("optional1", Out.O1); // ignore result
  return true;

If `optional1` is present but malformed, then we won't detect/report
that error. We may even leave `Out` in an incomplete state while returning true.
Instead, we'd often prefer to ignore `optional1` if it is absent, but otherwise
behave just like map().

Differential Revision: https://reviews.llvm.org/D89128
---
 include/llvm/Support/JSON.h    | 17 +++++++++++++----
 unittests/Support/JSONTest.cpp | 35 +++++++++++++++-------------------
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/include/llvm/Support/JSON.h b/include/llvm/Support/JSON.h
index 455673e42e9..9a8f915eeef 100644
--- a/include/llvm/Support/JSON.h
+++ b/include/llvm/Support/JSON.h
@@ -741,10 +741,9 @@ template <typename T> Value toJSON(const llvm::Optional<T> &Opt) {
 /// \code
 ///   bool fromJSON(const Value &E, MyStruct &R, Path P) {
 ///     ObjectMapper O(E, P);
-///     if (!O || !O.map("mandatory_field", R.MandatoryField))
-///       return false; // error details are already reported
-///     O.map("optional_field", R.OptionalField);
-///     return true;
+///     // When returning false, error details were already reported.
+///     return O && O.map("mandatory_field", R.MandatoryField) &&
+///         O.mapOptional("optional_field", R.OptionalField);
 ///   }
 /// \endcode
 class ObjectMapper {
@@ -780,6 +779,16 @@ public:
     return true;
   }
 
+  /// Maps a property to a field, if it exists.
+  /// If the property exists and is invalid, reports an error.
+  /// If the property does not exist, Out is unchanged.
+  template <typename T> bool mapOptional(StringLiteral Prop, T &Out) {
+    assert(*this && "Must check this is an object before calling map()");
+    if (const Value *E = O->get(Prop))
+      return fromJSON(*E, Out, P.field(Prop));
+    return true;
+  }
+
 private:
   const Object *O;
   Path P;
diff --git a/unittests/Support/JSONTest.cpp b/unittests/Support/JSONTest.cpp
index 9f17c98b4db..ed9a72d36b0 100644
--- a/unittests/Support/JSONTest.cpp
+++ b/unittests/Support/JSONTest.cpp
@@ -375,10 +375,8 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
 }
 bool fromJSON(const Value &E, CustomStruct &R, Path P) {
   ObjectMapper O(E, P);
-  if (!O || !O.map("str", R.S) || !O.map("int", R.I))
-    return false;
-  O.map("bool", R.B);
-  return true;
+  return O && O.map("str", R.S) && O.map("int", R.I) &&
+         O.mapOptional("bool", R.B);
 }
 
 static std::string errorContext(const Value &V, const Path::Root &R) {
@@ -392,24 +390,18 @@ TEST(JSONTest, Deserialize) {
   std::map<std::string, std::vector<CustomStruct>> R;
   CustomStruct ExpectedStruct = {"foo", 42, true};
   std::map<std::string, std::vector<CustomStruct>> Expected;
-  Value J = Object{
-      {"foo",
-       Array{
-           Object{
-               {"str", "foo"},
-               {"int", 42},
-               {"bool", true},
-               {"unknown", "ignored"},
-           },
-           Object{{"str", "bar"}},
-           Object{
-               {"str", "baz"}, {"bool", "string"}, // OK, deserialize ignores.
-           },
-       }}};
+  Value J = Object{{"foo", Array{
+                               Object{
+                                   {"str", "foo"},
+                                   {"int", 42},
+                                   {"bool", true},
+                                   {"unknown", "ignored"},
+                               },
+                               Object{{"str", "bar"}},
+                           }}};
   Expected["foo"] = {
       CustomStruct("foo", 42, true),
       CustomStruct("bar", llvm::None, false),
-      CustomStruct("baz", llvm::None, false),
   };
   Path::Root Root("CustomStruct");
   ASSERT_TRUE(fromJSON(J, R, Root));
@@ -423,7 +415,6 @@ TEST(JSONTest, Deserialize) {
   "foo": [
     /* error: expected object */
     123,
-    { ... },
     { ... }
   ]
 })";
@@ -443,6 +434,10 @@ TEST(JSONTest, Deserialize) {
   // Optional<T> must parse as the correct type if present.
   EXPECT_FALSE(fromJSON(Object{{"str", "1"}, {"int", "string"}}, V, Root));
   EXPECT_EQ("expected integer at CustomStruct.int", toString(Root.getError()));
+
+  // mapOptional must parse as the correct type if present.
+  EXPECT_FALSE(fromJSON(Object{{"str", "1"}, {"bool", "string"}}, V, Root));
+  EXPECT_EQ("expected boolean at CustomStruct.bool", toString(Root.getError()));
 }
 
 TEST(JSONTest, ParseDeserialize) {