Fix C++ FieldValue equality (#3043)

* Add FieldValue operator==

* Normalize NaN values to a single canonical value

This preserves existing Objective-C behavior.

* Add EqualsTester

* Port equality test from Android

Author: wilhuff

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -1033,6 +1033,7 @@
 		BB92EB03E3F92485023F64ED /* Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		BD01F0E43E4E2A07B8B05099 /* Pods-Firestore_Tests_macOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Tests_macOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Tests_macOS/Pods-Firestore_Tests_macOS.debug.xcconfig"; sourceTree = "<group>"; };
 		C8522DE226C467C54E6788D8 /* mutation_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = mutation_test.cc; sourceTree = "<group>"; };
+		CD422AF3E4515FB8E9BE67A0 /* equals_tester.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = equals_tester.h; sourceTree = "<group>"; };
 		D0A6E9136804A41CEC9D55D4 /* delayed_constructor_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = delayed_constructor_test.cc; sourceTree = "<group>"; };
 		D3CC3DC5338DCAF43A211155 /* README.md */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = net.daringfireball.markdown; name = README.md; path = ../README.md; sourceTree = "<group>"; };
 		D5B2593BCB52957D62F1C9D3 /* perf_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = perf_spec_test.json; sourceTree = "<group>"; };
@@ -1228,6 +1229,7 @@
 			children = (
 				5467FB06203E6A44009C9584 /* app_testing.h */,
 				5467FB07203E6A44009C9584 /* app_testing.mm */,
+				CD422AF3E4515FB8E9BE67A0 /* equals_tester.h */,
 				54A0352820A3B3BD003E0143 /* testutil.cc */,
 				54A0352920A3B3BD003E0143 /* testutil.h */,
 				BA6E5B9D53CCF301F58A62D7 /* xcgmock.h */,

Firestore/core/src/firebase/firestore/model/field_value.cc

@@ -29,6 +29,8 @@
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/hashing.h"
 #include "Firestore/core/src/firebase/firestore/util/to_string.h"
+#include "absl/algorithm/container.h"
+#include "absl/base/casts.h"
 #include "absl/memory/memory.h"
 #include "absl/strings/escaping.h"
 
@@ -42,6 +44,7 @@ using BaseValue = FieldValue::BaseValue;
 using Type = FieldValue::Type;
 
 using util::Compare;
+using util::CompareContainer;
 using util::ComparisonResult;
 
 template <typename T>
@@ -59,6 +62,13 @@ class NullValue : public FieldValue::BaseValue {
     return util::ToString(nullptr);
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    // NullValue is the only instance of itself
+    return true;
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -95,11 +105,19 @@ class SimpleFieldValue : public FieldValue::BaseValue {
     return util::ToString(value_);
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<SimpleFieldValue>(other);
+    return value_ == other_value.value();
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
 
-    return Compare(value_, Cast<SimpleFieldValue>(other).value());
+    auto& other_value = Cast<SimpleFieldValue>(other);
+    return Compare(value_, other_value.value());
   }
 
   size_t Hash() const override {
@@ -139,6 +157,17 @@ int64_t Integer(const BaseValue& rep) {
 class DoubleValue : public NumberValue<Type::Double, double> {
  public:
   using NumberValue<Type::Double, double>::NumberValue;
+
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<DoubleValue>(other);
+    return util::DoubleBitwiseEquals(value(), other_value.value());
+  }
+
+  size_t Hash() const override {
+    return util::DoubleBitwiseHash(value());
+  }
 };
 
 double Double(const BaseValue& rep) {
@@ -173,6 +202,12 @@ ComparisonResult NumberValue<type_enum, ValueType>::CompareTo(
   }
 }
 
+// TODO(wilhuff): Use SimpleFieldValue as a base once we migrate to absl::Hash.
+//
+// This can't extend SimpleFieldValue because `util::Hash` is undefined for
+// Timestamp (and you can't override a compile-time error in a base class out
+// of existence). absl::Hash allows us to implement hashing in a way that
+// requires no public declaration of conformance.
 class TimestampValue : public BaseValue {
  public:
   explicit TimestampValue(Timestamp value) : value_(value) {
@@ -186,6 +221,13 @@ class TimestampValue : public BaseValue {
     return util::ToString(value_);
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<TimestampValue>(other);
+    return value_ == other_value.value_;
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -230,6 +272,13 @@ class ServerTimestampValue : public FieldValue::BaseValue {
     return absl::StrCat("ServerTimestamp(local_write_time=", time, ")");
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<ServerTimestampValue>(other);
+    return local_write_time_ == other_value.local_write_time_;
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -291,6 +340,13 @@ class ReferenceValue : public FieldValue::BaseValue {
     return Type::Reference;
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<ReferenceValue>(other);
+    return database_id_ == other_value.database_id_ && key_ == other_value.key_;
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -328,6 +384,13 @@ class GeoPointValue : public BaseValue {
     return util::ToString(value_);
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<GeoPointValue>(other);
+    return value_ == other_value.value_;
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -357,6 +420,13 @@ class ArrayContents : public FieldValue::BaseValue {
     return Type::Array;
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<ArrayContents>(other);
+    return absl::c_equal(value_, other_value.value_);
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -390,6 +460,13 @@ class MapContents : public FieldValue::BaseValue {
     return Type::Object;
   }
 
+  bool Equals(const BaseValue& other) const override {
+    if (type() != other.type()) return false;
+
+    auto& other_value = Cast<MapContents>(other);
+    return absl::c_equal(value_, other_value.value_);
+  }
+
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
@@ -576,7 +653,25 @@ FieldValue FieldValue::FromInteger(int64_t value) {
   return FieldValue(std::make_shared<IntegerValue>(value));
 }
 
+// We use a canonical NaN bit pattern that's common for both Objective-C and
+// Java. Specifically:
+//
+//   - sign: 0
+//   - exponent: 11 bits, all 1
+//   - significand: 52 bits, MSB=1, rest=0
+//
+// This matches the Firestore backend which uses Double.doubleToLongBits from
+// the JDK (which is defined to normalize all NaNs to this value). This also
+// happens to be a common value for NAN in C++, but C++ does not require this
+// specific NaN value to be used, so we normalize.
+const uint64_t kCanonicalNanBits = 0x7ff8000000000000ULL;
+
 FieldValue FieldValue::FromDouble(double value) {
+  static double canonical_nan = absl::bit_cast<double>(kCanonicalNanBits);
+  if (std::isnan(value)) {
+    value = canonical_nan;
+  }
+
   return FieldValue(std::make_shared<DoubleValue>(value));
 }
 
@@ -636,6 +731,10 @@ FieldValue FieldValue::FromMap(FieldValue::Map&& value) {
   return FieldValue(std::make_shared<MapContents>(std::move(value)));
 }
 
+bool operator==(const FieldValue& lhs, const FieldValue& rhs) {
+  return lhs.rep_->Equals(*rhs.rep_);
+}
+
 std::ostream& operator<<(std::ostream& os, const FieldValue& value) {
   return os << value.ToString();
 }

Firestore/core/src/firebase/firestore/model/field_value.h

@@ -31,6 +31,7 @@
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/field_path.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
+#include "absl/base/attributes.h"
 #include "absl/types/optional.h"
 
 #if __OBJC__
@@ -41,12 +42,14 @@ namespace firebase {
 namespace firestore {
 namespace model {
 
+class ObjectValue;
+
 /**
  * tagged-union class representing an immutable data value as stored in
  * Firestore. FieldValue represents all the different kinds of values
  * that can be stored in fields in a document.
  */
-class FieldValue : public util::Comparable<FieldValue> {
+class FieldValue {
  public:
   using Array = std::vector<FieldValue>;
   using Map = immutable::SortedMap<std::string, FieldValue>;
@@ -77,6 +80,8 @@ class FieldValue : public util::Comparable<FieldValue> {
 
   FieldValue();
 
+  FieldValue(ObjectValue object);  // NOLINT(runtime/explicit)
+
 #if __OBJC__
   FSTFieldValue* Wrap() &&;
 #endif  // __OBJC__
@@ -153,6 +158,27 @@ class FieldValue : public util::Comparable<FieldValue> {
     return rep_->CompareTo(*rhs.rep_);
   }
 
+  /**
+   * Checks if the two values are equal, returning false if the value is
+   * perceptibly different in any regard.
+   *
+   * Comparison for FieldValues is defined by whether or not values should
+   * match for the purposes of querying. Comparison therefore makes the broadest
+   * possible allowance, looking only for logical equality. This means that e.g.
+   * -0.0, +0.0 and 0 (floating point and integer zeros) are all considered the
+   * same value for comparison purposes.
+   *
+   * Equality for FieldValues is defined by whether or not a user could
+   * perceive a change to the value. That is, a change from integer zero to
+   * a double zero can be perceived and so these values are unequal despite
+   * comparing same.
+   *
+   * This makes FieldValue one of the special cases where equality is
+   * inconsistent with comparison. There are cases where CompareTo will return
+   * Same but operator== will return false.
+   */
+  friend bool operator==(const FieldValue& lhs, const FieldValue& rhs);
+
   std::string ToString() const {
     return rep_->ToString();
   }
@@ -168,6 +194,8 @@ class FieldValue : public util::Comparable<FieldValue> {
 
     virtual std::string ToString() const = 0;
 
+    virtual bool Equals(const BaseValue& other) const = 0;
+
     virtual util::ComparisonResult CompareTo(const BaseValue& other) const = 0;
 
     virtual size_t Hash() const = 0;
@@ -254,6 +282,32 @@ class ObjectValue : public util::Comparable<ObjectValue> {
   FieldValue fv_;
 };
 
+// Pretend you can automatically upcast from ObjectValue to FieldValue.
+inline FieldValue::FieldValue(ObjectValue object)
+    : FieldValue(object.AsFieldValue()) {
+}
+
+inline bool operator!=(const FieldValue& lhs, const FieldValue& rhs) {
+  // See operator== for why this isn't using util::Same().
+  return !(lhs == rhs);
+}
+
+inline bool operator<(const FieldValue& lhs, const FieldValue& rhs) {
+  return util::Ascending(lhs.CompareTo(rhs));
+}
+inline bool operator>(const FieldValue& lhs, const FieldValue& rhs) {
+  return util::Descending(lhs.CompareTo(rhs));
+}
+inline bool operator<=(const FieldValue& lhs, const FieldValue& rhs) {
+  return !(rhs < lhs);
+}
+inline bool operator>=(const FieldValue& lhs, const FieldValue& rhs) {
+  return !(lhs < rhs);
+}
+
+// A bit pattern for our canonical NaN value. Exposed here for testing.
+ABSL_CONST_INIT extern const uint64_t kCanonicalNanBits;
+
 }  // namespace model
 }  // namespace firestore
 }  // namespace firebase

Firestore/core/test/firebase/firestore/model/CMakeLists.txt

@@ -29,6 +29,7 @@ cc_test(
   DEPENDS
     firebase_firestore_model
     firebase_firestore_testutil
+    GMock::GMock
 )
 
 cc_binary(