Merge remote-tracking branch 'origin/master' into rsgowman/protobuf_cpp

Also "fixed" BadFieldValueTagWithOtherValidTagsPresent test by changing
'false' to 'true'. Details: Depending on the version of nanopb, nanopb
would explicitly encode 'false', which shouldn't be done in proto3. When
it's explicitly encoded, the test worked properly. But when it was
(properly) dropped, the invalid tag is the only field that's actually
encoded, thus violating the assumptions of the test, leading to a test
failure. s/false/true fixes it, as now the boolean_value field is
(properly) encoded regardless of version.

12 files changed, 283 insertions, 125 deletions

diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.cc b/Firestore/core/src/firebase/firestore/nanopb/reader.cc
index 7a12900..69e3d83 100644
--- a/Firestore/core/src/firebase/firestore/nanopb/reader.cc
+++ b/Firestore/core/src/firebase/firestore/nanopb/reader.cc
@@ -16,7 +16,7 @@
 
 #include "Firestore/core/src/firebase/firestore/nanopb/reader.h"
 
-#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.pb.h"
+#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.nanopb.h"
 
 namespace firebase {
 namespace firestore {
@@ -136,6 +136,14 @@ std::string Reader::ReadString() {
   return result;
 }
 
+void Reader::SkipField(const Tag& tag) {
+  if (!status_.ok()) return;
+
+  if (!pb_skip_field(&stream_, tag.wire_type)) {
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+  }
+}
+
 }  // namespace nanopb
 }  // namespace firestore
 }  // namespace firebase
diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.h b/Firestore/core/src/firebase/firestore/nanopb/reader.h
index 930211a..2c16ec4 100644
--- a/Firestore/core/src/firebase/firestore/nanopb/reader.h
+++ b/Firestore/core/src/firebase/firestore/nanopb/reader.h
@@ -89,6 +89,15 @@ class Reader {
   template <typename T>
   T ReadNestedMessage(const std::function<T(Reader*)>& read_message_fn);
 
+  /**
+   * Discards the bytes associated with the given tag.
+   *
+   * @param tag The tag associated with the field that is otherwise about to be
+   * read. This method uses the tag to determine how many bytes should be
+   * discarded.
+   */
+  void SkipField(const Tag& tag);
+
   size_t bytes_left() const {
     return stream_.bytes_left;
   }
diff --git a/Firestore/core/src/firebase/firestore/nanopb/writer.cc b/Firestore/core/src/firebase/firestore/nanopb/writer.cc
index c3ffaac..7f32d4e 100644
--- a/Firestore/core/src/firebase/firestore/nanopb/writer.cc
+++ b/Firestore/core/src/firebase/firestore/nanopb/writer.cc
@@ -16,7 +16,7 @@
 
 #include "Firestore/core/src/firebase/firestore/nanopb/writer.h"
 
-#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.pb.h"
+#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.nanopb.h"
 
 namespace firebase {
 namespace firestore {
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 6ea5844..19a068b 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -24,8 +24,8 @@
 #include <string>
 #include <utility>
 
-#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.pb.h"
-#include "Firestore/Protos/nanopb/google/firestore/v1beta1/firestore.pb.h"
+#include "Firestore/Protos/nanopb/google/firestore/v1beta1/document.nanopb.h"
+#include "Firestore/Protos/nanopb/google/firestore/v1beta1/firestore.nanopb.h"
 #include "Firestore/core/include/firebase/firestore/firestore_errors.h"
 #include "Firestore/core/include/firebase/firestore/timestamp.h"
 #include "Firestore/core/src/firebase/firestore/model/document.h"
@@ -167,77 +167,102 @@ void EncodeFieldValueImpl(Writer* writer, const FieldValue& field_value) {
 FieldValue DecodeFieldValueImpl(Reader* reader) {
   if (!reader->status().ok()) return FieldValue::NullValue();
 
-  Tag tag = reader->ReadTag();
-  if (!reader->status().ok()) return FieldValue::NullValue();
+  // There needs to be at least one entry in the FieldValue.
+  if (reader->bytes_left() == 0) {
+    reader->set_status(Status(FirestoreErrorCode::DataLoss,
+                              "Input Value proto missing contents"));
+    return FieldValue::NullValue();
+  }
 
-  // Ensure the tag matches the wire type
-  switch (tag.field_number) {
-    case google_firestore_v1beta1_Value_null_value_tag:
-    case google_firestore_v1beta1_Value_boolean_value_tag:
-    case google_firestore_v1beta1_Value_integer_value_tag:
-      if (tag.wire_type != PB_WT_VARINT) {
-        reader->set_status(
-            Status(FirestoreErrorCode::DataLoss,
-                   "Input proto bytes cannot be parsed (mismatch between "
-                   "the wiretype and the field number (tag))"));
-      }
-      break;
+  FieldValue result = FieldValue::NullValue();
 
-    case google_firestore_v1beta1_Value_string_value_tag:
-    case google_firestore_v1beta1_Value_timestamp_value_tag:
-    case google_firestore_v1beta1_Value_map_value_tag:
-      if (tag.wire_type != PB_WT_STRING) {
-        reader->set_status(
-            Status(FirestoreErrorCode::DataLoss,
-                   "Input proto bytes cannot be parsed (mismatch between "
-                   "the wiretype and the field number (tag))"));
-      }
-      break;
+  while (reader->bytes_left()) {
+    Tag tag = reader->ReadTag();
+    if (!reader->status().ok()) return FieldValue::NullValue();
 
-    default:
-      // We could get here for one of two reasons; either because the input
-      // bytes are corrupt, or because we're attempting to parse a tag that we
-      // haven't implemented yet. Long term, the latter reason should become
-      // less likely (especially in production), so we'll assume former.
-
-      // TODO(rsgowman): While still in development, we'll contradict the above
-      // and assume the latter. Remove the following assertion when we're
-      // confident that we're handling all the tags in the protos.
-      HARD_FAIL(
-          "Unhandled message field number (tag): %s. (Or possibly "
-          "corrupt input bytes)",
-          tag.field_number);
-      reader->set_status(Status(
-          FirestoreErrorCode::DataLoss,
-          "Input proto bytes cannot be parsed (invalid field number (tag))"));
-  }
+    // Ensure the tag matches the wire type
+    switch (tag.field_number) {
+      case google_firestore_v1beta1_Value_null_value_tag:
+      case google_firestore_v1beta1_Value_boolean_value_tag:
+      case google_firestore_v1beta1_Value_integer_value_tag:
+        if (tag.wire_type != PB_WT_VARINT) {
+          reader->set_status(
+              Status(FirestoreErrorCode::DataLoss,
+                     "Input proto bytes cannot be parsed (mismatch between "
+                     "the wiretype and the field number (tag))"));
+        }
+        break;
 
-  if (!reader->status().ok()) return FieldValue::NullValue();
+      case google_firestore_v1beta1_Value_string_value_tag:
+      case google_firestore_v1beta1_Value_timestamp_value_tag:
+      case google_firestore_v1beta1_Value_map_value_tag:
+        if (tag.wire_type != PB_WT_STRING) {
+          reader->set_status(
+              Status(FirestoreErrorCode::DataLoss,
+                     "Input proto bytes cannot be parsed (mismatch between "
+                     "the wiretype and the field number (tag))"));
+        }
+        break;
 
-  switch (tag.field_number) {
-    case google_firestore_v1beta1_Value_null_value_tag:
-      reader->ReadNull();
-      return FieldValue::NullValue();
-    case google_firestore_v1beta1_Value_boolean_value_tag:
-      return FieldValue::BooleanValue(reader->ReadBool());
-    case google_firestore_v1beta1_Value_integer_value_tag:
-      return FieldValue::IntegerValue(reader->ReadInteger());
-    case google_firestore_v1beta1_Value_string_value_tag:
-      return FieldValue::StringValue(reader->ReadString());
-    case google_firestore_v1beta1_Value_timestamp_value_tag:
-      return FieldValue::TimestampValue(
-          reader->ReadNestedMessage<Timestamp>(DecodeTimestamp));
-    case google_firestore_v1beta1_Value_map_value_tag:
-      return FieldValue::ObjectValueFromMap(
-          reader->ReadNestedMessage<ObjectValue::Map>(DecodeMapValue));
+      case google_firestore_v1beta1_Value_double_value_tag:
+      case google_firestore_v1beta1_Value_bytes_value_tag:
+      case google_firestore_v1beta1_Value_reference_value_tag:
+      case google_firestore_v1beta1_Value_geo_point_value_tag:
+      case google_firestore_v1beta1_Value_array_value_tag:
+        // TODO(b/74243929): Implement remaining types.
+        HARD_FAIL("Unhandled message field number (tag): %i.",
+                  tag.field_number);
 
-    default:
-      // This indicates an internal error as we've already ensured that this is
-      // a valid field_number.
-      HARD_FAIL(
-          "Somehow got an unexpected field number (tag) after verifying that "
-          "the field number was expected.");
+      default:
+        // Unknown tag. According to the proto spec, we need to ignore these. No
+        // action required here, though we'll need to skip the relevant bytes
+        // below.
+        break;
+    }
+
+    if (!reader->status().ok()) return FieldValue::NullValue();
+
+    switch (tag.field_number) {
+      case google_firestore_v1beta1_Value_null_value_tag:
+        reader->ReadNull();
+        result = FieldValue::NullValue();
+        break;
+      case google_firestore_v1beta1_Value_boolean_value_tag:
+        result = FieldValue::BooleanValue(reader->ReadBool());
+        break;
+      case google_firestore_v1beta1_Value_integer_value_tag:
+        result = FieldValue::IntegerValue(reader->ReadInteger());
+        break;
+      case google_firestore_v1beta1_Value_string_value_tag:
+        result = FieldValue::StringValue(reader->ReadString());
+        break;
+      case google_firestore_v1beta1_Value_timestamp_value_tag:
+        result = FieldValue::TimestampValue(
+            reader->ReadNestedMessage<Timestamp>(DecodeTimestamp));
+        break;
+      case google_firestore_v1beta1_Value_map_value_tag:
+        // TODO(rsgowman): We should merge the existing map (if any) with the
+        // newly parsed map.
+        result = FieldValue::ObjectValueFromMap(
+            reader->ReadNestedMessage<ObjectValue::Map>(DecodeMapValue));
+        break;
+
+      case google_firestore_v1beta1_Value_double_value_tag:
+      case google_firestore_v1beta1_Value_bytes_value_tag:
+      case google_firestore_v1beta1_Value_reference_value_tag:
+      case google_firestore_v1beta1_Value_geo_point_value_tag:
+      case google_firestore_v1beta1_Value_array_value_tag:
+        // TODO(b/74243929): Implement remaining types.
+        HARD_FAIL("Unhandled message field number (tag): %i.",
+                  tag.field_number);
+
+      default:
+        // Unknown tag. According to the proto spec, we need to ignore these.
+        reader->SkipField(tag);
+    }
   }
+
+  return result;
 }
 
 /**
@@ -504,7 +529,7 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
   // Initialize BatchGetDocumentsResponse fields to their default values
   std::unique_ptr<MaybeDocument> found;
   std::string missing;
-  // TODO(rsgowman): transaction
+  // We explicitly ignore the 'transaction' field
   SnapshotVersion read_time = SnapshotVersion::None();
 
   while (reader->bytes_left()) {
@@ -515,6 +540,7 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
     switch (tag.field_number) {
       case google_firestore_v1beta1_BatchGetDocumentsResponse_found_tag:
       case google_firestore_v1beta1_BatchGetDocumentsResponse_missing_tag:
+      case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag:
       case google_firestore_v1beta1_BatchGetDocumentsResponse_read_time_tag:
         if (tag.wire_type != PB_WT_STRING) {
           reader->set_status(
@@ -524,14 +550,11 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
         }
         break;
 
-      case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag:
-        // TODO(rsgowman)
-        abort();
-
       default:
-        reader->set_status(Status(
-            FirestoreErrorCode::DataLoss,
-            "Input proto bytes cannot be parsed (invalid field number (tag))"));
+        // Unknown tag. According to the proto spec, we need to ignore these. No
+        // action required here, though we'll need to skip the relevant bytes
+        // below.
+        break;
     }
 
     if (!reader->status().ok()) return nullptr;
@@ -559,8 +582,13 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
         break;
 
       case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag:
-        // TODO(rsgowman)
-        abort();
+        // This field is ignored by the client sdk, but we still need to extract
+        // it.
+        // TODO(rsgowman) switch this to reader->SkipField() (or whatever we end
+        // up calling it) once that exists. Possibly group this with other
+        // ignored and/or unknown fields
+        reader->ReadString();
+        break;
 
       case google_firestore_v1beta1_BatchGetDocumentsResponse_read_time_tag:
         read_time = SnapshotVersion{
@@ -568,11 +596,8 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
         break;
 
       default:
-        // This indicates an internal error as we've already ensured that this
-        // is a valid field_number.
-        HARD_FAIL(
-            "Somehow got an unexpected field number (tag) after verifying that "
-            "the field number was expected.");
+        // Unknown tag. According to the proto spec, we need to ignore these.
+        reader->SkipField(tag);
     }
   }
 
@@ -581,9 +606,10 @@ std::unique_ptr<MaybeDocument> Serializer::DecodeBatchGetDocumentsResponse(
   } else if (!missing.empty()) {
     return absl::make_unique<NoDocument>(DecodeKey(missing), read_time);
   } else {
-    // Neither 'found' nor 'missing' fields were set.
-    // TODO(rsgowman): Handle the error case.
-    abort();
+    reader->set_status(Status(FirestoreErrorCode::DataLoss,
+                              "Invalid BatchGetDocumentsReponse message: "
+                              "Neither 'found' nor 'missing' fields set."));
+    return nullptr;
   }
 }
 
diff --git a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt
index 043713f..30589a0 100644
--- a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt
+++ b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt
@@ -29,6 +29,7 @@ cc_library(
     absl_strings
 )
 
+
 ## assert and log
 
 cc_library(
@@ -158,16 +159,6 @@ else()
 
 endif()
 
-cc_library(
-  firebase_firestore_util_async_queue
-  SOURCES
-    async_queue.cc
-    async_queue.h
-  DEPENDS
-    ${FIREBASE_FIRESTORE_UTIL_EXECUTOR}
-    ${FIREBASE_FIRESTORE_UTIL_LOG}
-  EXCLUDE_FROM_ALL
-)
 
 ## main library
 
@@ -179,6 +170,8 @@ configure_file(
 cc_library(
   firebase_firestore_util
   SOURCES
+    async_queue.cc
+    async_queue.h
     autoid.cc
     autoid.h
     bits.cc
@@ -200,10 +193,11 @@ cc_library(
     statusor_internals.h
     string_util.cc
     string_util.h
+    type_traits.h
   DEPENDS
     absl_base
     firebase_firestore_util_base
-    firebase_firestore_util_async_queue
+    ${FIREBASE_FIRESTORE_UTIL_EXECUTOR}
     ${FIREBASE_FIRESTORE_UTIL_LOG}
     ${FIREBASE_FIRESTORE_UTIL_RANDOM}
 )
diff --git a/Firestore/core/src/firebase/firestore/util/comparison.cc b/Firestore/core/src/firebase/firestore/util/comparison.cc
index 5ac4c27..d1cdbfa 100644
--- a/Firestore/core/src/firebase/firestore/util/comparison.cc
+++ b/Firestore/core/src/firebase/firestore/util/comparison.cc
@@ -31,6 +31,12 @@ bool Comparator<absl::string_view>::operator()(
   return left < right;
 }
 
+bool Comparator<std::string>::operator()(const std::string& left,
+                                         const std::string& right) const {
+  // TODO(wilhuff): truncation aware comparison
+  return left < right;
+}
+
 bool Comparator<double>::operator()(double left, double right) const {
   // NaN sorts equal to itself and before any other number.
   if (left < right) {
diff --git a/Firestore/core/src/firebase/firestore/util/comparison.h b/Firestore/core/src/firebase/firestore/util/comparison.h
index d7f4dfd..a7d7944 100644
--- a/Firestore/core/src/firebase/firestore/util/comparison.h
+++ b/Firestore/core/src/firebase/firestore/util/comparison.h
@@ -86,6 +86,11 @@ struct Comparator<absl::string_view> {
                   const absl::string_view& right) const;
 };
 
+template <>
+struct Comparator<std::string> {
+  bool operator()(const std::string& left, const std::string& right) const;
+};
+
 /** Compares two bools: false < true. */
 template <>
 struct Comparator<bool> : public std::less<bool> {};
diff --git a/Firestore/core/src/firebase/firestore/util/hard_assert_apple.mm b/Firestore/core/src/firebase/firestore/util/hard_assert_apple.mm
index 3324fe8..6abd324 100644
--- a/Firestore/core/src/firebase/firestore/util/hard_assert_apple.mm
+++ b/Firestore/core/src/firebase/firestore/util/hard_assert_apple.mm
@@ -32,8 +32,8 @@ void Fail(const char* file,
           const int line,
           const std::string& message) {
   [[NSAssertionHandler currentHandler]
-      handleFailureInFunction:WrapNSStringNoCopy(func)
-                         file:WrapNSStringNoCopy(file)
+      handleFailureInFunction:WrapNSString(func)
+                         file:WrapNSString(file)
                    lineNumber:line
                   description:@"FIRESTORE INTERNAL ASSERTION FAILED: %s",
                               message.c_str()];
diff --git a/Firestore/core/src/firebase/firestore/util/hashing.h b/Firestore/core/src/firebase/firestore/util/hashing.h
index d8058c8..21c0bd6 100644
--- a/Firestore/core/src/firebase/firestore/util/hashing.h
+++ b/Firestore/core/src/firebase/firestore/util/hashing.h
@@ -18,6 +18,7 @@
 #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_HASHING_H_
 
 #include <iterator>
+#include <string>
 #include <type_traits>
 
 namespace firebase {
@@ -49,6 +50,41 @@ namespace util {
 namespace impl {
 
 /**
+ * A type trait that identifies whether or not std::hash is available for a
+ * given type.
+ *
+ * This type should not be necessary since specialization failure on an
+ * expression like `decltype(std::hash<K>{}(value)` should be enough to disable
+ * overloads that require `std::hash` to be defined but unfortunately some
+ * standard libraries ship with std::hash defined for all types that only
+ * fail later (e.g. via static_assert). One such implementation is the libc++
+ * that ships with Xcode 8.3.3, which is a supported platform.
+ */
+template <typename T>
+struct has_std_hash {
+  // There may be other types for which std::hash is defined but they don't
+  // matter for our purposes.
+  enum {
+    value = std::is_arithmetic<T>{} || std::is_pointer<T>{} ||
+            std::is_same<T, std::string>{}
+  };
+
+  constexpr operator bool() const {
+    return value;
+  }
+};
+
+/**
+ * A type that's equivalent to size_t if std::hash<T> is defined or a compile
+ * error otherwise.
+ *
+ * This is effectively just a safe implementation of
+ * `decltype(std::hash<T>{}(std::declval<T>()))`.
+ */
+template <typename T>
+using std_hash_type = typename std::enable_if<has_std_hash<T>{}, size_t>::type;
+
+/**
  * Combines a hash_value with whatever accumulated state there is so far.
  */
 inline size_t Combine(size_t state, size_t hash_value) {
@@ -100,8 +136,7 @@ auto RankedInvokeHash(const K& value, HashChoice<0>) -> decltype(value.Hash()) {
  * @return The result of `std::hash<K>{}(value)`
  */
 template <typename K>
-auto RankedInvokeHash(const K& value, HashChoice<1>)
-    -> decltype(std::hash<K>{}(value)) {
+std_hash_type<K> RankedInvokeHash(const K& value, HashChoice<1>) {
   return std::hash<K>{}(value);
 }
 
diff --git a/Firestore/core/src/firebase/firestore/util/string_format.h b/Firestore/core/src/firebase/firestore/util/string_format.h
index d691984..01776a9 100644
--- a/Firestore/core/src/firebase/firestore/util/string_format.h
+++ b/Firestore/core/src/firebase/firestore/util/string_format.h
@@ -22,6 +22,7 @@
 #include <utility>
 
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
+#include "Firestore/core/src/firebase/firestore/util/type_traits.h"
 #include "absl/base/attributes.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
@@ -43,7 +44,7 @@ template <int I>
 struct FormatChoice : FormatChoice<I + 1> {};
 
 template <>
-struct FormatChoice<4> {};
+struct FormatChoice<5> {};
 
 }  // namespace internal
 
@@ -87,8 +88,8 @@ class FormatArg : public absl::AlphaNum {
    */
   template <
       typename T,
-      typename = typename std::enable_if<std::is_base_of<NSObject, T>{}>::type>
-  FormatArg(T* object, internal::FormatChoice<0>)
+      typename = typename std::enable_if<is_objective_c_pointer<T>{}>::type>
+  FormatArg(T object, internal::FormatChoice<1>)
       : AlphaNum{MakeStringView([object description])} {
   }
 
@@ -96,20 +97,9 @@ class FormatArg : public absl::AlphaNum {
    * Creates a FormatArg from any Objective-C Class type. Objective-C Class
    * types are a special struct that aren't of a type derived from NSObject.
    */
-  FormatArg(Class object, internal::FormatChoice<0>)
+  FormatArg(Class object, internal::FormatChoice<1>)
       : AlphaNum{MakeStringView(NSStringFromClass(object))} {
   }
-
-  /**
-   * Creates a FormatArg from any id pointer. Note that instances of `id<Foo>`
-   * (which means "pointer conforming to the protocol Foo") do not match this
-   * without first casting to type `id`. There's no way to express a template of
-   * `id<T>` since `id<Foo>` isn't actually a C++ template and `id` isn't a
-   * parameterized C++ class.
-   */
-  FormatArg(id object, internal::FormatChoice<0>)
-      : AlphaNum{MakeStringView([object description])} {
-  }
 #endif
 
   /**
@@ -117,7 +107,7 @@ class FormatArg : public absl::AlphaNum {
    * handled specially to avoid ambiguity with generic pointers, which are
    * handled differently.
    */
-  FormatArg(std::nullptr_t, internal::FormatChoice<1>) : AlphaNum{"null"} {
+  FormatArg(std::nullptr_t, internal::FormatChoice<2>) : AlphaNum{"null"} {
   }
 
   /**
@@ -125,7 +115,7 @@ class FormatArg : public absl::AlphaNum {
    * handled specially to avoid ambiguity with generic pointers, which are
    * handled differently.
    */
-  FormatArg(const char* string_value, internal::FormatChoice<2>)
+  FormatArg(const char* string_value, internal::FormatChoice<3>)
       : AlphaNum{string_value == nullptr ? "null" : string_value} {
   }
 
@@ -134,7 +124,7 @@ class FormatArg : public absl::AlphaNum {
    * hexidecimal integer literal.
    */
   template <typename T>
-  FormatArg(T* pointer_value, internal::FormatChoice<3>)
+  FormatArg(T* pointer_value, internal::FormatChoice<4>)
       : AlphaNum{absl::Hex{reinterpret_cast<uintptr_t>(pointer_value)}} {
   }
 
@@ -143,7 +133,7 @@ class FormatArg : public absl::AlphaNum {
    * absl::AlphaNum accepts.
    */
   template <typename T>
-  FormatArg(T&& value, internal::FormatChoice<4>)
+  FormatArg(T&& value, internal::FormatChoice<5>)
       : AlphaNum{std::forward<T>(value)} {
   }
 };
diff --git a/Firestore/core/src/firebase/firestore/util/string_util.h b/Firestore/core/src/firebase/firestore/util/string_util.h
index 96ba0b0..86acc56 100644
--- a/Firestore/core/src/firebase/firestore/util/string_util.h
+++ b/Firestore/core/src/firebase/firestore/util/string_util.h
@@ -65,11 +65,6 @@ std::string PrefixSuccessor(absl::string_view prefix);
  */
 std::string ImmediateSuccessor(absl::string_view s);
 
-/**
- * Returns true if the given value starts with the given prefix.
- */
-bool StartsWith(const std::string &value, const std::string &prefix);
-
 }  // namespace util
 }  // namespace firestore
 }  // namespace firebase
diff --git a/Firestore/core/src/firebase/firestore/util/type_traits.h b/Firestore/core/src/firebase/firestore/util/type_traits.h
new file mode 100644
index 0000000..52feb6b
--- /dev/null
+++ b/Firestore/core/src/firebase/firestore/util/type_traits.h
@@ -0,0 +1,90 @@
+/*
+ * Copyright 2018 Google
+ *
+ * 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.
+ */
+
+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_
+
+#if __OBJC__
+#import <objc/objc.h>  // for id
+#endif
+
+#include <type_traits>
+
+namespace firebase {
+namespace firestore {
+namespace util {
+
+#if __OBJC__
+
+/**
+ * A type trait that identifies whether or not the given pointer points to an
+ * Objective-C object.
+ *
+ * is_objective_c_pointer<NSObject*>::value == true
+ * is_objective_c_pointer<NSArray<NSString*>*>::value == true
+ *
+ * // id is a dynamically typed pointer to an Objective-C object.
+ * is_objective_c_pointer<id>::value == true
+ *
+ * // pointers to C++ classes are not Objective-C pointers.
+ * is_objective_c_pointer<void*>::value == false
+ * is_objective_c_pointer<std::string*>::value == false
+ * is_objective_c_pointer<std::unique_ptr<int>>::value == false
+ */
+template <typename T>
+struct is_objective_c_pointer {
+ private:
+  using yes_type = char (&)[10];
+  using no_type = char (&)[1];
+
+  /**
+   * A non-existent function declared to produce a pointer to type T (which is
+   * consistent with the way Objective-C objects are referenced).
+   *
+   * Note that there is no definition for this function but that's okay because
+   * we only need it to reason about the function's type at compile type.
+   */
+  static T Pointer();
+
+  static yes_type Choose(id value);
+  static no_type Choose(...);
+
+ public:
+  using value_type = bool;
+
+  enum { value = sizeof(Choose(Pointer())) == sizeof(yes_type) };
+
+  constexpr operator bool() const {
+    return value;
+  }
+
+  constexpr bool operator()() const {
+    return value;
+  }
+};
+
+// Hard-code the answer for `void` because you can't pass arguments of type
+// `void` to another function.
+template <>
+struct is_objective_c_pointer<void> : public std::false_type {};
+
+#endif  // __OBJC__
+
+}  // namespace util
+}  // namespace firestore
+}  // namespace firebase
+
+#endif  // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_