Add tests for folly::dynamic ordering comparison operators on scalar/string...

Add tests for folly::dynamic ordering comparison operators on scalar/string types and highlight some bugs

Summary:
# Summary
Currently nullptr vs nullptr and int vs double ordering comparison operators (e.g. `operator<`) are inconsistent with their equality operators.

This diff just adds tests to increase coverage and highlight the broken areas. The subsequent diffs will address the issues individually.

The behavior on nulls at least has been present since the very beginning of when a CompareOp was added for it in 2017.

More details are below.

# Bug 1: nullptr vs nullptr
If we have
```
folly::dynamic a(nullptr);
folly::dynamic b(nullptr);

a < b // returns true
a == b // returns true
```
This violates that the two should be equal if `!(a < b) && !(b < a)` given that this expression will return `false`, whereas `a == b` returns `true`.

This property is required by e.g. std::sets.
If one puts a `folly::dynamic(nullptr)` in an std::set with the current behavior, when iterating it will be found, and will contribute to its size, but `std::set::find()` on it will return false. Hash-based sets are unaffected - given that the hash and `operator==` implementations are correct.

The fix is straightforward.
`struct dynamic::CompareOp<std::nullptr_t>` in dynamic-inl.h should return false.

# Bug  2: int vs double (hash + ordering operators)
The `folly::dynamic==` operator does numeric comparison for int64 vs double - whereas the `folly::dynamic<` operator does not - instead falling back on comparing against the Type enum value. Similar to the above this violates the principle of `operator<` being usable for checking equality - given that it is really just checking type equality - but has already been established in the code path that they're different types.

Hashing is also inconsistent given that a hash of dynamic(2.0) will not equal dynamic(2) even though they are equal, given that each uses the hash of the underlying type. When ordering is fixed, hashing should be too so that hash based and ordering based containers have the same behavior.

The fixes are straightforward.

operator< in dynamic.cpp should pull the same logic from operator== with regards int vs double.
See https://www.internalfb.com/code/fbsource/[4e70ca24b9ec]/fbcode/folly/dynamic.cpp?lines=97-126

hashing can be resolved by having doubles use integer hashing in dynamic.cpp - https://www.internalfb.com/code/fbsource/[4e70ca24b9ec]/fbcode/folly/dynamic.cpp?lines=307

 ---

Reviewed By: Gownta

Differential Revision: D33465564

fbshipit-source-id: 43b926837e20f4a9afe15ee0032d469864e360c0

folly/dynamic.h

@@ -771,9 +771,7 @@ struct dynamic {
 
   /*
    * Get a hash code.  This function is called by a std::hash<>
-   * specialization, also.
-   *
-   * Throws TypeError if this is an object, array, or null.
+   * specialization.
    */
   std::size_t hash() const;
 

folly/test/ComparisonOperatorTestUtil.h

+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <functional>
+#include <sstream>
+#include <folly/portability/GTest.h>
+
+namespace folly {
+namespace test {
+namespace detail {
+
+/**
+ * Returns a lambda, which when called, will return a string describing the
+ * first and second values. Using a lambda vs direct returning the string will
+ * ensure the object under test will not have any other methods called
+ * (including those invoked by an ostream operator) until the lambda is invoked.
+ * This will allow usage in `EXPECT_TRUE(...) << theReturnedLambda()` that will
+ * only trigger when a test has already failed.
+ *
+ * NOTE: The returned lambda will have captured the provided references and thus
+ * is only intended for use while those references remain valid. We
+ * intentionally don't capture by value to again leave the object intact, but
+ * also not every object has a copy constructor. We take by reference_wrapper to
+ * make this more clear in the calling code, and communicate that it should not
+ * be null.
+ */
+template <typename FirstType, typename SecondType>
+auto getComparisonTestDescriptionGenerator(
+    const std::string& firstVarName,
+    std::reference_wrapper<const FirstType> firstValueRef,
+    const std::string& secondVarName,
+    std::reference_wrapper<const SecondType> secondValueRef) {
+  return [=]() {
+    std::stringstream ss;
+
+    ss << firstVarName << ": " << ::testing::PrintToString(firstValueRef.get())
+       << ", compared to " << secondVarName << ": "
+       << ::testing::PrintToString(secondValueRef.get());
+
+    return ss.str();
+  };
+}
+
+/**
+ * NOTE: All the below operator testing functions will ensure that each value
+ * will show up on both sides of the operator under test, so as to exercise the
+ * operators of each type/value - given that the logic may vary.
+ *
+ * NOTE: All the test functions use EXPECT gtest macros and thus will emit
+ * multiple gtest failures for all operator failures within the function.
+ *
+ * NOTE: In all functions manual `EXPECT_TRUE/FALSE(valueA op valueB)` type
+ * calls are done, as opposed to using the 1EXPECT_LT/LE/EQ/NE/GE/GT` macros
+ * which can accomplish the same. This is purely for clarity of visual
+ * inspection.
+ */
+
+/**
+ * Tests equality operators (`==`, `!=`) for values which should be
+ * considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testEqualityOperatorsForEqualValues(
+    const TypeA& valueA, const TypeB& valueB) {
+  auto genDescription = getComparisonTestDescriptionGenerator(
+      "valueA", std::cref(valueA), "valueB", std::cref(valueB));
+
+  EXPECT_TRUE(valueA == valueB) << genDescription();
+  EXPECT_TRUE(valueB == valueA) << genDescription();
+
+  EXPECT_FALSE(valueA != valueB) << genDescription();
+  EXPECT_FALSE(valueB != valueA) << genDescription();
+}
+
+/**
+ * Tests ordering operators (`<`, `<=`, `>`, `>=`) for values
+ * which should be considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testOrderingOperatorsForEqualValues(
+    const TypeA& valueA, const TypeB& valueB) {
+  auto genDescription = getComparisonTestDescriptionGenerator(
+      "valueA", std::cref(valueA), "valueB", std::cref(valueB));
+
+  EXPECT_FALSE(valueA < valueB) << genDescription();
+  EXPECT_FALSE(valueB < valueA) << genDescription();
+
+  EXPECT_TRUE(valueA <= valueB) << genDescription();
+  EXPECT_TRUE(valueB <= valueA) << genDescription();
+
+  EXPECT_TRUE(valueA >= valueB) << genDescription();
+  EXPECT_TRUE(valueB >= valueA) << genDescription();
+
+  EXPECT_FALSE(valueA > valueB) << genDescription();
+  EXPECT_FALSE(valueB > valueA) << genDescription();
+}
+
+/**
+ * Tests all comparison operators (`==`, `!=`, `<`, `<=`, `>`, `>=`) for values
+ * which should be considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testComparisonOperatorsForEqualValues(
+    const TypeA& valueA, const TypeB& valueB) {
+  testEqualityOperatorsForEqualValues(valueA, valueB);
+  testOrderingOperatorsForEqualValues(valueA, valueB);
+}
+
+/**
+ * Tests equality operators (`==`, `!=`) for values which should not be
+ * considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testEqualityOperatorsForNotEqualValues(
+    const TypeA& valueA, const TypeB& valueB) {
+  auto genDescription = getComparisonTestDescriptionGenerator(
+      "valueA", std::cref(valueA), "valueB", std::cref(valueB));
+
+  EXPECT_FALSE(valueA == valueB) << genDescription();
+  EXPECT_FALSE(valueB == valueA) << genDescription();
+
+  EXPECT_TRUE(valueA != valueB) << genDescription();
+  EXPECT_TRUE(valueB != valueA) << genDescription();
+}
+
+/**
+ * Tests ordering operators (`<`, `<=`, `>`, `>=`) for values
+ * which should not be considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testOrderingOperatorsForNotEqualValues(
+    const TypeA& smallerValue, const TypeB& largerValue) {
+  auto genDescription = getComparisonTestDescriptionGenerator(
+      "smallerValue",
+      std::cref(smallerValue),
+      "largerValue",
+      std::cref(largerValue));
+
+  EXPECT_TRUE(smallerValue < largerValue) << genDescription();
+  EXPECT_FALSE(largerValue < smallerValue) << genDescription();
+
+  EXPECT_TRUE(smallerValue <= largerValue) << genDescription();
+  EXPECT_FALSE(largerValue <= smallerValue) << genDescription();
+
+  EXPECT_FALSE(smallerValue >= largerValue) << genDescription();
+  EXPECT_TRUE(largerValue >= smallerValue) << genDescription();
+
+  EXPECT_FALSE(smallerValue > largerValue) << genDescription();
+  EXPECT_TRUE(largerValue > smallerValue) << genDescription();
+}
+
+/**
+ * Tests all comparison operators (`==`, `!=`, `<`, `<=`, `>`, `>=`) for values
+ * which should not be considered equal.
+ */
+template <typename TypeA, typename TypeB>
+void testComparisonOperatorsForNotEqualValues(
+    const TypeA& smallerValue, const TypeB& largerValue) {
+  testEqualityOperatorsForNotEqualValues(smallerValue, largerValue);
+  testOrderingOperatorsForNotEqualValues(smallerValue, largerValue);
+}
+
+} // namespace detail
+} // namespace test
+} // namespace folly

folly/test/DynamicTest.cpp

@@ -23,10 +23,16 @@
 #include <folly/Range.h>
 #include <folly/json.h>
 #include <folly/portability/GTest.h>
+#include <folly/test/ComparisonOperatorTestUtil.h>
 
 using folly::dynamic;
 using folly::StringPiece;
 
+namespace folly {
+namespace test {
+
+using namespace detail;
+
 TEST(Dynamic, Default) {
   dynamic obj;
   EXPECT_TRUE(obj.isNull());
@@ -500,6 +506,204 @@ TEST(Dynamic, Operator) {
   EXPECT_EQ(math, 3);
 }
 
+namespace {
+
+void testOrderingOperatorsThrowForObjectTypes(
+    const dynamic& valueA, const dynamic& valueB) {
+  ASSERT_TRUE(valueA.isObject() || valueB.isObject())
+      << "This function is only intended for objects";
+
+  // The compiler will complain with "relational comparison result unused" if
+  // we don't send the result of comparison operations somewhere. So we just use
+  // an empty lambda which seems to satisfy it.
+  auto swallow = [](bool /*unused*/) {};
+
+#define FB_EXPECT_THROW(boolExpr) \
+  EXPECT_THROW(swallow(boolExpr), folly::TypeError)
+
+  FB_EXPECT_THROW(valueA < valueB);
+  FB_EXPECT_THROW(valueB < valueA);
+
+  FB_EXPECT_THROW(valueA <= valueB);
+  FB_EXPECT_THROW(valueB <= valueA);
+
+  FB_EXPECT_THROW(valueA >= valueB);
+  FB_EXPECT_THROW(valueB >= valueA);
+
+  FB_EXPECT_THROW(valueA > valueB);
+  FB_EXPECT_THROW(valueB > valueA);
+
+#undef FB_EXPECT_THROW
+}
+
+void testComparisonOperatorsForEqualDynamicValues(
+    const dynamic& valueA, const dynamic& valueB) {
+  testEqualityOperatorsForEqualValues(valueA, valueB);
+
+  if (valueA.isNull() && valueB.isNull()) {
+    // There's a bug in null vs null ordering comparison. This test is mainly
+    // for highlighting the behavior which will be fixed in a subsequent commit.
+    EXPECT_TRUE(valueA < valueB);
+    EXPECT_TRUE(valueA > valueB);
+    EXPECT_FALSE(valueB <= valueA);
+    EXPECT_FALSE(valueB >= valueA);
+  } else if (valueA.isObject() || valueB.isObject()) {
+    // Objects don't support ordering
+    testOrderingOperatorsThrowForObjectTypes(valueA, valueB);
+  } else {
+    testOrderingOperatorsForEqualValues(valueA, valueB);
+  }
+}
+
+void testOrderingOperatorsForNotEqualDynamicValues(
+    const dynamic& smallerValue, const dynamic& largerValue) {
+  if (smallerValue.isObject() || largerValue.isObject()) {
+    // Objects don't support ordering
+    testOrderingOperatorsThrowForObjectTypes(smallerValue, largerValue);
+  } else {
+    testOrderingOperatorsForNotEqualValues(smallerValue, largerValue);
+  }
+}
+
+void testComparisonOperatorsForNotEqualDynamicValues(
+    const dynamic& smallerValue, const dynamic& largerValue) {
+  testEqualityOperatorsForNotEqualValues(smallerValue, largerValue);
+
+  // Ordering is special for dynamics due to objects
+  testOrderingOperatorsForNotEqualDynamicValues(smallerValue, largerValue);
+}
+
+// Calls func on all index pair permutations of 0 to (numValues - 1) where
+// smallerIndex < largerIndex.
+void executeOnOrderedIndexPairs(
+    size_t numValues,
+    std::function<void(size_t smallerIndex, size_t largerIndex)> func) {
+  // The `Idx` naming below is used to avoid local variable shadow warnings with
+  // the func parameter names, which are unnecessary but serving as
+  // documentation.
+  for (size_t smallerIdx = 0; smallerIdx < numValues; ++smallerIdx) {
+    for (size_t largerIdx = smallerIdx + 1; largerIdx < numValues;
+         ++largerIdx) {
+      func(smallerIdx, largerIdx);
+    }
+  }
+}
+
+// Returns values of each type of dynamic, sorted in strictly increasing order
+// as defined by dynamic::operator<
+std::vector<dynamic> getUniqueOrderedValuesForAllTypes() {
+  return {
+      // NULLT
+      nullptr,
+
+      // ARRAY
+      dynamic::array(0, 1, 2),
+      dynamic::array(2, 0, 1),
+
+      // BOOL
+      false,
+      true,
+
+      // DOUBLE
+      -1.1,
+      2.2,
+
+      // INT64
+      -1,
+      2,
+
+      // OBJECT - NOTE these don't actually have ordering comparison, so could
+      // be anywhere
+      dynamic::object("a", dynamic::array(1, 2, 3)),
+      dynamic::object("b", dynamic::array(1, 2, 3)),
+
+      // STRING
+      "abc",
+      "def",
+  };
+}
+
+std::vector<std::pair<dynamic, dynamic>> getNumericallyEqualOrderedPairs() {
+  // Double is intentionally first since in dynamic::Type it is before int64.
+  return {
+      {-2.0, -2},
+      {0.0, 0},
+      {1.0, 1},
+      {dynamic::array(1.0), dynamic::array(1)},
+      {dynamic::object("a", dynamic::array(1.0)),
+       dynamic::object("a", dynamic::array(1))},
+  };
+}
+
+} // namespace
+
+TEST(Dynamic, ComparisonOperatorsOnNotEqualValuesOfAllTypes) {
+  auto values = getUniqueOrderedValuesForAllTypes();
+
+  // Test ordering operators
+  executeOnOrderedIndexPairs(
+      values.size(), [&](size_t smallerIndex, size_t largerIndex) {
+        testComparisonOperatorsForNotEqualDynamicValues(
+            values[smallerIndex] /*smallerValue*/,
+            values[largerIndex] /*largerValue*/);
+      });
+}
+
+TEST(Dynamic, ComparisonOperatorsOnSameValuesOfSameTypes) {
+  for (const auto& value : getUniqueOrderedValuesForAllTypes()) {
+    testComparisonOperatorsForEqualDynamicValues(value, value);
+  }
+}
+
+TEST(Dynamic, ComparisonOperatorsForNumericallyEqualIntAndDoubles) {
+  for (const auto& [valueA, valueB] : getNumericallyEqualOrderedPairs()) {
+    testEqualityOperatorsForEqualValues(valueA, valueB);
+
+    // Ints and doubles are equivalent for equality operators, but not for
+    // ordering operators currently. This will be fixed in an upcoming commit.
+    testOrderingOperatorsForNotEqualDynamicValues(valueA, valueB);
+  }
+}
+
+TEST(Dynamic, HashDoesNotThrow) {
+  for (const auto& value : getUniqueOrderedValuesForAllTypes()) {
+    EXPECT_NO_THROW(std::hash<dynamic>()(value)) << value;
+  }
+}
+
+// NOTE: This test currently fails - but will be fixed in an upcoming commit.
+// The reason it's not enabled with EXPECT_NE is that some build modes seem to
+// set hashing of ints/doubles to return 0 to presumably help catch issues with
+// hash dependencies.
+//
+// TEST(Dynamic, HashForNumericallyEqualIntAndDoubles) {
+//   for (const auto& [valueA, valueB] : getNumericallyEqualOrderedPairs()) {
+//     // This is just highlighting that the same hashing functions will apply
+//     // to numerically equal values.
+//     EXPECT_EQ(std::hash<dynamic>()(valueA), std::hash<dynamic>()(valueB))
+//         << "valueA: " << valueA << ", valueB: " << valueB;
+//   }
+// }
+
+TEST(Dynamic, ComparisonOperatorsForNonEquivalenctCases) {
+  // Mainly highlighting some cases for wich implicit conversion is not done.
+  // Within each group they are ordered per dynamic::Type enum value.
+  std::vector<std::vector<dynamic>> notEqualValueTestCases{
+      {nullptr, false, 0},
+      {true, 1},
+      {1, "1"},
+  };
+
+  for (const auto& testCase : notEqualValueTestCases) {
+    executeOnOrderedIndexPairs(
+        testCase.size(), [&](size_t smallerIndex, size_t largerIndex) {
+          testComparisonOperatorsForNotEqualDynamicValues(
+              testCase[smallerIndex] /*smallerValue*/,
+              testCase[largerIndex] /*largerValue*/);
+        });
+  }
+}
+
 TEST(Dynamic, Conversions) {
   dynamic str = "12.0";
   EXPECT_EQ(str.asDouble(), 12.0);
@@ -1303,3 +1507,6 @@ TEST(Dynamic, EqualNestedValues) {
   dynamic obj2 = obj1;
   EXPECT_EQ(obj1, obj2);
 }
+
+} // namespace test
+} // namespace folly