[Support] Make JSON handle doubles and int64s losslessly

[sammccall]

Summary

This patch adds a new "integer" ValueType, and renames Number -> Double.
This allows us to preserve the full precision of int64_t when parsing integers
from the wire, or constructing from an integer.
The API is unchanged, other than giving asInteger() a clearer contract.

In addition, always output doubles with enough precision that parsing will
reconstruct the same double.

[sammccall]

Clarify docs, add deserializer for int64_t

[simon_tatham]

Other than these nitpicks, this looks good to me. I've rebased my tablegen patch on to it and confirmed that it now handles integers the way I'd like it to, without me having had to change a line of my client code.

lib/Support/JSON.cpp

347

Tiniest nitpick ever: in this function you've explicitly said std::strtod, but used strtoll unqualified. I wasn't able to figure out which of <cstdlib> and <stdlib.h> actually ends up being included by the headers this file asks for, but std::strtoll would probably be safer, just in case it's <cstdlib>.

unittests/Support/JSONTest.cpp

268

You mentioned in D45753 that there might at some point be a need to handle uint64_t as well as int64_t. Given that, it might make sense to include some tests of negative integers now, so that if that does need to be added later, we'll catch any accidental breakage of the negative integer tests?

[sammccall]

Thanks for verifying this! (Still waiting for a decision on the master patch, I hope this doesn't block you too long!)

unittests/Support/JSONTest.cpp

268

Done. I *really* hope we never have to support both at once!

[sammccall]

Negative number test case, fix std:: qualification.

[simon_tatham]

LGTM. Thanks!

[simon_tatham]

This comment was removed by simon_tatham.

[simon_tatham]

Oops – posted a comment in the wrong browser tab :-( Sorry for the noise, and especially sorry to @nhaehnle who I didn't actually intend to subscribe to this review.

[sammccall]

Rebase

include/llvm/Support/JSON.h

 /// These can be list-initialized, or used to build up collections in a loop.
 /// json::ary(Collection) converts all items in a collection to Values.
 ///
 /// === Inspecting values ===
 ///
 /// Each Value is one of the JSON kinds:
 ///   null    (nullptr_t)
 ///   boolean (bool)
-///   number  (double)
+///   number  (double or int64)
 ///   string  (StringRef)
 ///   array   (json::Array)
 ///   object  (json::Object)
 ///
 /// The kind can be queried directly, or implicitly via the typed accessors:
 ///   if (Optional<StringRef> S = E.asString()
 ///     assert(E.kind() == Value::String);
 ///

 /// And parsed:
 ///   Expected<Value> E = json::parse("[1, 2, null]");
 ///   assert(E && E->kind() == Value::Array);
 class Value {
 public:
   enum Kind {
     Null,
     Boolean,
+    /// Number values can store both int64s and doubles at full precision,
+    /// depending on what they were constructed/parsed from.
     Number,
     String,
     Array,
     Object,
   };
 
   // It would be nice to have Value() be null. But that would make {} null too.
   Value(const Value &M) { copyFrom(M); }
public:
   Value(const llvm::SmallVectorImpl<char> &V) : Type(T_String) {
     create<std::string>(V.begin(), V.end());
   }
   Value(const llvm::formatv_object_base &V) : Value(V.str()){};
   // Strings: types with reference semantics.
   Value(llvm::StringRef V) : Type(T_StringRef) { create<llvm::StringRef>(V); }
   Value(const char *V) : Type(T_StringRef) { create<llvm::StringRef>(V); }
   Value(std::nullptr_t) : Type(T_Null) {}
-  // Prevent implicit conversions to boolean.
-  template <typename T, typename = typename std::enable_if<
-                            std::is_same<T, bool>::value>::type>
+  // Boolean (disallow implicit conversions).
+  // (The last template parameter is a dummy to keep templates distinct.)
+  template <
+      typename T,
+      typename = typename std::enable_if<std::is_same<T, bool>::value>::type,
+      bool = false>
   Value(T B) : Type(T_Boolean) {
     create<bool>(B);
   }
-  // Numbers: arithmetic types that are not boolean.
+  // Integers (except boolean). Must be non-narrowing convertible to int64_t.
   template <
       typename T,
-      typename = typename std::enable_if<std::is_arithmetic<T>::value>::type,
+      typename = typename std::enable_if<std::is_integral<T>::value>::type,
       typename = typename std::enable_if<std::integral_constant<
-          bool, !std::is_same<T, bool>::value>::value>::type>
-  Value(T D) : Type(T_Number) {
-    create<double>(D);
+          bool, !std::is_same<T, bool>::value>::value>::type,
+      int64_t = 0>
+  Value(T I) : Type(T_Integer) {
+    create<int64_t>(int64_t{I});
+  }
+  // Floating point. Must be non-narrowing convertible to double.
+  template <typename T,
+            typename =
+                typename std::enable_if<std::is_floating_point<T>::value>::type,
+            double* = nullptr>
+  Value(T D) : Type(T_Double) {
+    create<double>(double{D});
   }
   // Serializable types: with a toJSON(const T&)->Value function, found by ADL.
   template <typename T,
             typename = typename std::enable_if<std::is_same<
-                Value, decltype(toJSON(*(const T *)nullptr))>::value>>
+                Value, decltype(toJSON(*(const T *)nullptr))>::value>,
+            Value* = nullptr>
   Value(const T &V) : Value(toJSON(V)) {}
 
   Value &operator=(const Value &M) {
     destroy();
     copyFrom(M);
     return *this;
   }
   Value &operator=(Value &&M) {
     destroy();
     moveFrom(std::move(M));
     return *this;
   }
   ~Value() { destroy(); }
 
   Kind kind() const {
     switch (Type) {
     case T_Null:
       return Null;
     case T_Boolean:
       return Boolean;
-    case T_Number:
+    case T_Double:
+    case T_Integer:
       return Number;
     case T_String:
     case T_StringRef:
       return String;
     case T_Object:
       return Object;
     case T_Array:
       return Array;
     }
     llvm_unreachable("Unknown kind");
   }
 
   // Typed accessors return None/nullptr if the Value is not of this type.
   llvm::Optional<std::nullptr_t> asNull() const {
     if (LLVM_LIKELY(Type == T_Null))
       return nullptr;
     return llvm::None;
   }
   llvm::Optional<bool> asBoolean() const {
     if (LLVM_LIKELY(Type == T_Boolean))
       return as<bool>();
     return llvm::None;
   }
   llvm::Optional<double> asNumber() const {
-    if (LLVM_LIKELY(Type == T_Number))
+    if (LLVM_LIKELY(Type == T_Double))
       return as<double>();
+    if (LLVM_LIKELY(Type == T_Integer))
+      return as<int64_t>();
     return llvm::None;
   }
+  // Succeeds if the Value is a Number, and exactly representable as int64_t.
   llvm::Optional<int64_t> asInteger() const {
-    if (LLVM_LIKELY(Type == T_Number)) {
+    if (LLVM_LIKELY(Type == T_Integer))
+      return as<int64_t>();
+    if (LLVM_LIKELY(Type == T_Double)) {
       double D = as<double>();
       if (LLVM_LIKELY(std::modf(D, &D) == 0 &&
                       D >= std::numeric_limits<int64_t>::min() &&
                       D <= std::numeric_limits<int64_t>::max()))
         return D;
     }
     return llvm::None;
   }
private:
 
   template <typename Indenter>
   void print(llvm::raw_ostream &, const Indenter &) const;
   friend struct llvm::format_provider<llvm::json::Value>;
 
   enum ValueType : char {
     T_Null,
     T_Boolean,
-    // FIXME: splitting Number into Double and Integer would allow us to
-    //        round-trip 64-bit integers.
-    T_Number,
+    T_Double,
+    T_Integer,
     T_StringRef,
     T_String,
     T_Object,
     T_Array,
   };
   // All members mutable, see moveFrom().
   mutable ValueType Type;
-  mutable llvm::AlignedCharArrayUnion<bool, double, llvm::StringRef,
+  mutable llvm::AlignedCharArrayUnion<bool, double, int64_t, llvm::StringRef,
                                       std::string, json::Array, json::Object>
       Union;
 };
 
 bool operator==(const Value &, const Value &);
 inline bool operator!=(const Value &L, const Value &R) { return !(L == R); }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Value &);
 

 }
 inline bool fromJSON(const Value &E, int &Out) {
   if (auto S = E.asInteger()) {
     Out = *S;
     return true;
   }
   return false;
 }
+inline bool fromJSON(const Value &E, int64_t &Out) {
+  if (auto S = E.asInteger()) {
+    Out = *S;
+    return true;
+  }
+  return false;
+}
 inline bool fromJSON(const Value &E, double &Out) {
   if (auto S = E.asNumber()) {
     Out = *S;
     return true;
   }
   return false;
 }
 inline bool fromJSON(const Value &E, bool &Out) {

lib/Support/JSON.cpp

 Value::Value(std::initializer_list<Value> Elements)
   : Value(json::Array(Elements)) {}
 
   void Value::copyFrom(const Value &M) {
     Type = M.Type;
     switch (Type) {
       case T_Null:
       case T_Boolean:
-      case T_Number:
+      case T_Double:
+      case T_Integer:
         memcpy(Union.buffer, M.Union.buffer, sizeof(Union.buffer));
         break;
       case T_StringRef:
         create<StringRef>(M.as<StringRef>());
         break;
       case T_String:
         create<std::string>(M.as<std::string>());
         break;
       case T_Object:
         create<json::Object>(M.as<json::Object>());
         break;
       case T_Array:
         create<json::Array>(M.as<json::Array>());
         break;
     }
   }
 
 void Value::moveFrom(const Value &&M) {
   Type = M.Type;
   switch (Type) {
     case T_Null:
     case T_Boolean:
-    case T_Number:
+    case T_Double:
+    case T_Integer:
       memcpy(Union.buffer, M.Union.buffer, sizeof(Union.buffer));
       break;
     case T_StringRef:
       create<StringRef>(M.as<StringRef>());
       break;
     case T_String:
       create<std::string>(std::move(M.as<std::string>()));
       M.Type = T_Null;
       break;
     case T_Object:
       create<json::Object>(std::move(M.as<json::Object>()));
       M.Type = T_Null;
       break;
     case T_Array:
       create<json::Array>(std::move(M.as<json::Array>()));
       M.Type = T_Null;
       break;
   }
 }
 
 void Value::destroy() {
   switch (Type) {
     case T_Null:
     case T_Boolean:
-    case T_Number:
+    case T_Double:
+    case T_Integer:
       break;
     case T_StringRef:
       as<StringRef>().~StringRef();
       break;
     case T_String:
       as<std::string>().~basic_string();
       break;
     case T_Object:
// Simple recursive-descent JSON parser.
 
    private:
     void eatWhitespace() {
       while (P != End && (*P == ' ' || *P == '\r' || *P == '\n' || *P == '\t'))
         ++P;
     }
 
     // On invalid syntax, parseX() functions return false and set Err.
-    bool parseNumber(char First, double &Out);
+    bool parseNumber(char First, Value &Out);
     bool parseString(std::string &Out);
     bool parseUnicode(std::string &Out);
     bool parseError(const char *Msg); // always returns false
 
     char next() { return P == End ? 0 : *P++; }
     char peek() { return P == End ? 0 : *P; }
     static bool isNumber(char C) {
       return C == '0' || C == '1' || C == '2' || C == '3' || C == '4' ||
switch (char C = next()) {
             case '}':
               return true;
             default:
               return parseError("Expected , or } after object property");
           }
         }
       }
       default:
-        if (isNumber(C)) {
-          double Num;
-          if (parseNumber(C, Num)) {
-            Out = Num;
-            return true;
-          } else {
-            return false;
-          }
-        }
+        if (isNumber(C))
+          return parseNumber(C, Out);
         return parseError("Invalid JSON value");
     }
   }
 
-  bool Parser::parseNumber(char First, double &Out) {
+  bool Parser::parseNumber(char First, Value &Out) {
+    // Read the number into a string. (Must be null-terminated for strto*).
     SmallString<24> S;
     S.push_back(First);
     while (isNumber(peek()))
       S.push_back(next());
     char *End;
+    // Try first to parse as integer, and if so preserve full 64 bits.
+    // strtoll returns long long >= 64 bits, so check it's in range too.
+    auto I = strtoll(S.c_str(), &End, 10);
+    if (End == S.end() && I >= std::numeric_limits<int64_t>::min() &&
+        I <= std::numeric_limits<int64_t>::max()) {
+      Out = int64_t(I);
+      return true;
+    }
+    // If it's not an integer
     Out = std::strtod(S.c_str(), &End);
     return End == S.end() || parseError("Invalid JSON value (number?)");
   }
 
   bool Parser::parseString(std::string &Out) {
     // leading quote was already consumed.
     for (char C = next(); C != '"'; C = next()) {
       if (LLVM_UNLIKELY(P == End))

 void llvm::json::Value::print(raw_ostream &OS, const Indenter &I) const {
   switch (Type) {
     case T_Null:
       OS << "null";
       break;
     case T_Boolean:
       OS << (as<bool>() ? "true" : "false");
       break;
-    case T_Number:
-      OS << format("%g", as<double>());
+    case T_Double:
+      OS << format("%.*g", std::numeric_limits<double>::max_digits10,
+                   as<double>());
+      break;
+    case T_Integer:
+      OS << as<int64_t>();
       break;
     case T_StringRef:
       quote(OS, as<StringRef>());
       break;
     case T_String:
       quote(OS, as<std::string>());
       break;
     case T_Object: {

unittests/Support/JSONTest.cpp

if (I++ == 5) {
       ASSERT_TRUE(E.asObject());
       EXPECT_EQ(E.asObject()->getString("time"),
                 llvm::Optional<llvm::StringRef>("arrow"));
     } else
       EXPECT_FALSE(E.asObject());
   }
 }
 
+// Verify special integer handling - we try to preserve exact int64 values.
+TEST(JSONTest, Integers) {
+  struct {
+    const char* Desc;
+    Value Val;
+    const char* Str;
+    llvm::Optional<int64_t> AsInt;
+    llvm::Optional<double> AsNumber;
+  } TestCases[] = {
+      {
+          "Non-integer. Stored as double, not convertible.",
+          double{1.5},
+          "1.5",
+          llvm::None,
+          1.5,
+      },
+
+      {
+          "Integer, not exact double. Stored as int64, convertible.",
+          int64_t{0x4000000000000001},
+          "4611686018427387905",
+          int64_t{0x4000000000000001},
+          double{0x4000000000000000},
+      },
+
+      {
+          "Dynamically exact integer. Stored as double, convertible.",
+          double{0x6000000000000000},
+          "6.9175290276410819e+18",
+          int64_t{0x6000000000000000},
+          double{0x6000000000000000},
+      },
+
+      {
+          "Dynamically integer, >64 bits. Stored as double, not convertible.",
+          1.5 * double{0x8000000000000000},
+          "1.3835058055282164e+19",
+          llvm::None,
+          1.5 * double{0x8000000000000000},
+      },
+  };
+  for (const auto& T : TestCases) {
+    EXPECT_EQ(T.Str, s(T.Val)) << T.Desc;
+    llvm::Expected<Value> Doc = parse(T.Str);
+    EXPECT_TRUE(!!Doc) << T.Desc;
+    EXPECT_EQ(Doc->asInteger(), T.AsInt) << T.Desc;
+    EXPECT_EQ(Doc->asNumber(), T.AsNumber) << T.Desc;
+    EXPECT_EQ(T.Val, *Doc) << T.Desc;
+    EXPECT_EQ(T.Str, s(*Doc)) << T.Desc;
+  }
+}
+
 // Sample struct with typical JSON-mapping rules.
 struct CustomStruct {
   CustomStruct() : B(false) {}
   CustomStruct(std::string S, llvm::Optional<int> I, bool B)
       : S(S), I(I), B(B) {}
   std::string S;
   llvm::Optional<int> I;
   bool B;