Clean up warnings in the Firestore project (#2622)

* Add nanopb::CheckedSize

Fix implicit narrowing warnings and reduce pb_size_t copypasta.

* Rename local::DocumentReference to DocumentKeyReference

This cleans up a build-time warning about a duplicate file in the Copy
Headers phase and cleans up a massive duplicate id warning during pod
install.

Author: wilhuff

Firestore/core/src/firebase/firestore/local/CMakeLists.txt

@@ -54,8 +54,8 @@ endif()
 cc_library(
   firebase_firestore_local
   SOURCES
-    document_reference.h
-    document_reference.cc
+    document_key_reference.h
+    document_key_reference.cc
     index_manager.h
     listen_sequence.h
     local_documents_view.h

Firestore/core/src/firebase/firestore/local/document_reference.cc renamed to Firestore/core/src/firebase/firestore/local/document_key_reference.cc

@@ -14,10 +14,11 @@
  * limitations under the License.
  */
 
+#include "Firestore/core/src/firebase/firestore/local/document_key_reference.h"
+
 #include <string>
 #include <utility>
 
-#include "Firestore/core/src/firebase/firestore/local/document_reference.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/util/comparison.h"
 #include "Firestore/core/src/firebase/firestore/util/hashing.h"
@@ -29,22 +30,23 @@ namespace local {
 using model::DocumentKey;
 using util::ComparisonResult;
 
-bool operator==(const DocumentReference& lhs, const DocumentReference& rhs) {
+bool operator==(const DocumentKeyReference& lhs,
+                const DocumentKeyReference& rhs) {
   return lhs.key_ == rhs.key_ && lhs.ref_id_ == rhs.ref_id_;
 }
 
-size_t DocumentReference::Hash() const {
+size_t DocumentKeyReference::Hash() const {
   return util::Hash(key_.ToString(), ref_id_);
 }
 
-std::string DocumentReference::ToString() const {
-  return util::StringFormat("<DocumentReference: key=%s, id=%s>",
+std::string DocumentKeyReference::ToString() const {
+  return util::StringFormat("<DocumentKeyReference: key=%s, id=%s>",
                             key_.ToString(), ref_id_);
 }
 
 /** Sorts document references by key then ID. */
-bool DocumentReference::ByKey::operator()(const DocumentReference& lhs,
-                                          const DocumentReference& rhs) const {
+bool DocumentKeyReference::ByKey::operator()(
+    const DocumentKeyReference& lhs, const DocumentKeyReference& rhs) const {
   util::Comparator<model::DocumentKey> key_less;
   if (key_less(lhs.key_, rhs.key_)) return true;
   if (key_less(rhs.key_, lhs.key_)) return false;
@@ -54,8 +56,8 @@ bool DocumentReference::ByKey::operator()(const DocumentReference& lhs,
 }
 
 /** Sorts document references by ID then key. */
-bool DocumentReference::ById::operator()(const DocumentReference& lhs,
-                                         const DocumentReference& rhs) const {
+bool DocumentKeyReference::ById::operator()(
+    const DocumentKeyReference& lhs, const DocumentKeyReference& rhs) const {
   util::Comparator<int32_t> id_less;
   if (id_less(lhs.ref_id_, rhs.ref_id_)) return true;
   if (id_less(rhs.ref_id_, lhs.ref_id_)) return false;

Firestore/core/src/firebase/firestore/local/document_reference.h renamed to Firestore/core/src/firebase/firestore/local/document_key_reference.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_REFERENCE_H_
-#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_REFERENCE_H_
+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_KEY_REFERENCE_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_KEY_REFERENCE_H_
 
 #include <string>
 #include <utility>
@@ -39,13 +39,13 @@ namespace local {
  *
  * Not to be confused with FIRDocumentReference.
  */
-class DocumentReference {
+class DocumentKeyReference {
  public:
-  DocumentReference() {
+  DocumentKeyReference() {
   }
 
   /** Initializes the document reference with the given key and Id. */
-  DocumentReference(model::DocumentKey key, int32_t ref_id)
+  DocumentKeyReference(model::DocumentKey key, int32_t ref_id)
       : key_{std::move(key)}, ref_id_{ref_id} {
   }
 
@@ -63,23 +63,23 @@ class DocumentReference {
     return ref_id_;
   }
 
-  friend bool operator==(const DocumentReference& lhs,
-                         const DocumentReference& rhs);
+  friend bool operator==(const DocumentKeyReference& lhs,
+                         const DocumentKeyReference& rhs);
 
   size_t Hash() const;
 
   std::string ToString() const;
 
   /** Sorts document references by key then Id. */
-  struct ByKey : public util::Comparator<DocumentReference> {
-    bool operator()(const DocumentReference& lhs,
-                    const DocumentReference& rhs) const;
+  struct ByKey : public util::Comparator<DocumentKeyReference> {
+    bool operator()(const DocumentKeyReference& lhs,
+                    const DocumentKeyReference& rhs) const;
   };
 
   /** Sorts document references by Id then key. */
-  struct ById : public util::Comparator<DocumentReference> {
-    bool operator()(const DocumentReference& lhs,
-                    const DocumentReference& rhs) const;
+  struct ById : public util::Comparator<DocumentKeyReference> {
+    bool operator()(const DocumentKeyReference& lhs,
+                    const DocumentKeyReference& rhs) const;
   };
 
  private:
@@ -94,4 +94,4 @@ class DocumentReference {
 }  // namespace firestore
 }  // namespace firebase
 
-#endif  // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_REFERENCE_H_
+#endif  // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_DOCUMENT_KEY_REFERENCE_H_

Firestore/core/src/firebase/firestore/local/local_serializer.cc

@@ -29,6 +29,7 @@
 #include "Firestore/core/src/firebase/firestore/model/no_document.h"
 #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
 #include "Firestore/core/src/firebase/firestore/model/unknown_document.h"
+#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/string_format.h"
 
@@ -44,6 +45,7 @@ using model::MutationBatch;
 using model::NoDocument;
 using model::SnapshotVersion;
 using model::UnknownDocument;
+using nanopb::CheckedSize;
 using nanopb::Reader;
 using nanopb::Writer;
 using remote::MakeArray;
@@ -123,10 +125,8 @@ google_firestore_v1_Document LocalSerializer::EncodeDocument(
       rpc_serializer_.EncodeString(rpc_serializer_.EncodeKey(doc.key()));
 
   // Encode Document.fields (unless it's empty)
-  size_t count = doc.data().GetInternalValue().size();
-  HARD_ASSERT(count <= std::numeric_limits<pb_size_t>::max(),
-              "Unable to encode specified document. Too many fields.");
-  result.fields_count = static_cast<pb_size_t>(count);
+  pb_size_t count = CheckedSize(doc.data().GetInternalValue().size());
+  result.fields_count = count;
   result.fields = MakeArray<google_firestore_v1_Document_FieldsEntry>(count);
   int i = 0;
   for (const auto& kv : doc.data().GetInternalValue()) {
@@ -257,10 +257,8 @@ firestore_client_WriteBatch LocalSerializer::EncodeMutationBatch(
   firestore_client_WriteBatch result{};
 
   result.batch_id = mutation_batch.batch_id();
-  size_t count = mutation_batch.mutations().size();
-  HARD_ASSERT(count <= std::numeric_limits<pb_size_t>::max(),
-              "Unable to encode specified mutation batch. Too many mutations.");
-  result.writes_count = static_cast<pb_size_t>(count);
+  pb_size_t count = CheckedSize(mutation_batch.mutations().size());
+  result.writes_count = count;
   result.writes = MakeArray<google_firestore_v1_Write>(count);
   int i = 0;
   for (const std::unique_ptr<Mutation>& mutation : mutation_batch.mutations()) {

Firestore/core/src/firebase/firestore/local/memory_mutation_queue.h

@@ -29,7 +29,7 @@
 #import "Firestore/Source/Public/FIRTimestamp.h"
 
 #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h"
-#include "Firestore/core/src/firebase/firestore/local/document_reference.h"
+#include "Firestore/core/src/firebase/firestore/local/document_key_reference.h"
 #include "Firestore/core/src/firebase/firestore/local/mutation_queue.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
@@ -94,8 +94,8 @@ class MemoryMutationQueue : public MutationQueue {
   void SetLastStreamToken(NSData* _Nullable token) override;
 
  private:
-  using DocumentReferenceSet =
-      immutable::SortedSet<DocumentReference, DocumentReference::ByKey>;
+  using DocumentKeyReferenceSet =
+      immutable::SortedSet<DocumentKeyReference, DocumentKeyReference::ByKey>;
 
   std::vector<FSTMutationBatch*> AllMutationBatchesWithIds(
       const std::set<model::BatchId>& batch_ids);
@@ -147,7 +147,7 @@ class MemoryMutationQueue : public MutationQueue {
   NSData* _Nullable last_stream_token_;
 
   /** An ordered mapping between documents and the mutation batch IDs. */
-  DocumentReferenceSet batches_by_document_key_;
+  DocumentKeyReferenceSet batches_by_document_key_;
 };
 
 }  // namespace local

Firestore/core/src/firebase/firestore/local/memory_mutation_queue.mm

@@ -25,7 +25,7 @@
 #import "Firestore/Source/Model/FSTMutation.h"
 #import "Firestore/Source/Model/FSTMutationBatch.h"
 
-#include "Firestore/core/src/firebase/firestore/local/document_reference.h"
+#include "Firestore/core/src/firebase/firestore/local/document_key_reference.h"
 #include "Firestore/core/src/firebase/firestore/model/resource_path.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 
@@ -98,7 +98,7 @@
   // Track references by document key and index collection parents.
   for (FSTMutation* mutation : [batch mutations]) {
     batches_by_document_key_ = batches_by_document_key_.insert(
-        DocumentReference{mutation.key, batch_id});
+        DocumentKeyReference{mutation.key, batch_id});
 
     persistence_.indexManager->AddToCollectionParentIndex(
         mutation.key.path().PopLast());
@@ -121,7 +121,7 @@
     const DocumentKey& key = mutation.key;
     [persistence_.referenceDelegate removeMutationReference:key];
 
-    DocumentReference reference{key, batch.batchID};
+    DocumentKeyReference reference{key, batch.batchID};
     batches_by_document_key_ = batches_by_document_key_.erase(reference);
   }
 }
@@ -132,7 +132,7 @@
   // First find the set of affected batch IDs.
   std::set<BatchId> batch_ids;
   for (const DocumentKey& key : document_keys) {
-    DocumentReference start{key, 0};
+    DocumentKeyReference start{key, 0};
 
     for (const auto& reference : batches_by_document_key_.values_from(start)) {
       if (key != reference.key()) break;
@@ -149,7 +149,7 @@
     const DocumentKey& key) {
   std::vector<FSTMutationBatch*> result;
 
-  DocumentReference start{key, 0};
+  DocumentKeyReference start{key, 0};
   for (const auto& reference : batches_by_document_key_.values_from(start)) {
     if (key != reference.key()) break;
 
@@ -178,7 +178,7 @@
   if (!DocumentKey::IsDocumentKey(start_path)) {
     start_path = start_path.Append("");
   }
-  DocumentReference start{DocumentKey{start_path}, 0};
+  DocumentKeyReference start{DocumentKey{start_path}, 0};
 
   // Find unique batchIDs referenced by all documents potentially matching the
   // query.
@@ -242,7 +242,7 @@
 bool MemoryMutationQueue::ContainsKey(const model::DocumentKey& key) {
   // Create a reference with a zero ID as the start position to find any
   // document reference with this key.
-  DocumentReference reference{key, 0};
+  DocumentKeyReference reference{key, 0};
   auto range = batches_by_document_key_.values_from(reference);
   auto begin = range.begin();
   return begin != range.end() && begin->key() == key;

Firestore/core/src/firebase/firestore/local/reference_set.cc

@@ -15,8 +15,9 @@
  */
 
 #include "Firestore/core/src/firebase/firestore/local/reference_set.h"
+
 #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h"
-#include "Firestore/core/src/firebase/firestore/local/document_reference.h"
+#include "Firestore/core/src/firebase/firestore/local/document_key_reference.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 
 namespace firebase {
@@ -27,7 +28,7 @@ using model::DocumentKey;
 using model::DocumentKeySet;
 
 void ReferenceSet::AddReference(const DocumentKey& key, int id) {
-  DocumentReference reference{key, id};
+  DocumentKeyReference reference{key, id};
   by_key_ = by_key_.insert(reference);
   by_id_ = by_id_.insert(reference);
 }
@@ -39,7 +40,7 @@ void ReferenceSet::AddReferences(const DocumentKeySet& keys, int id) {
 }
 
 void ReferenceSet::RemoveReference(const DocumentKey& key, int id) {
-  RemoveReference(DocumentReference{key, id});
+  RemoveReference(DocumentKeyReference{key, id});
 }
 
 void ReferenceSet::RemoveReferences(
@@ -50,8 +51,8 @@ void ReferenceSet::RemoveReferences(
 }
 
 DocumentKeySet ReferenceSet::RemoveReferences(int id) {
-  DocumentReference start{DocumentKey::Empty(), id};
-  DocumentReference end{DocumentKey::Empty(), id + 1};
+  DocumentKeyReference start{DocumentKey::Empty(), id};
+  DocumentKeyReference end{DocumentKey::Empty(), id + 1};
 
   DocumentKeySet removed{};
 
@@ -65,19 +66,19 @@ DocumentKeySet ReferenceSet::RemoveReferences(int id) {
 
 void ReferenceSet::RemoveAllReferences() {
   auto initial = by_key_;
-  for (const DocumentReference& reference : initial) {
+  for (const DocumentKeyReference& reference : initial) {
     RemoveReference(reference);
   }
 }
 
-void ReferenceSet::RemoveReference(const DocumentReference& reference) {
+void ReferenceSet::RemoveReference(const DocumentKeyReference& reference) {
   by_key_ = by_key_.erase(reference);
   by_id_ = by_id_.erase(reference);
 }
 
 DocumentKeySet ReferenceSet::ReferencedKeys(int id) {
-  DocumentReference start{DocumentKey::Empty(), id};
-  DocumentReference end{DocumentKey::Empty(), id + 1};
+  DocumentKeyReference start{DocumentKey::Empty(), id};
+  DocumentKeyReference end{DocumentKey::Empty(), id + 1};
 
   DocumentKeySet keys;
   for (const auto& reference : by_id_.values_in(start, end)) {
@@ -89,7 +90,7 @@ DocumentKeySet ReferenceSet::ReferencedKeys(int id) {
 bool ReferenceSet::ContainsKey(const DocumentKey& key) {
   // Create a reference with a zero ID as the start position to find any
   // document reference with this key.
-  DocumentReference start{key, 0};
+  DocumentKeyReference start{key, 0};
 
   auto range = by_key_.values_from(start);
   auto begin = range.begin();

Firestore/core/src/firebase/firestore/local/reference_set.h

@@ -18,7 +18,7 @@
 #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_REFERENCE_SET_H_
 
 #include "Firestore/core/src/firebase/firestore/immutable/sorted_set.h"
-#include "Firestore/core/src/firebase/firestore/local/document_reference.h"
+#include "Firestore/core/src/firebase/firestore/local/document_key_reference.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
 
@@ -31,7 +31,7 @@ namespace local {
  * (either a TargetId or BatchId). As references are added to or removed from
  * the set corresponding events are emitted to a registered garbage collector.
  *
- * Each reference is represented by a DocumentReference object. Each of them
+ * Each reference is represented by a DocumentKeyReference object. Each of them
  * contains enough information to uniquely identify the reference. They are all
  * stored primarily in a set sorted by key. A document is considered garbage if
  * there's no references in that set (this can be efficiently checked thanks to
@@ -81,10 +81,11 @@ class ReferenceSet {
   bool ContainsKey(const model::DocumentKey& key);
 
  private:
-  void RemoveReference(const DocumentReference& reference);
+  void RemoveReference(const DocumentKeyReference& reference);
 
-  immutable::SortedSet<DocumentReference, DocumentReference::ByKey> by_key_;
-  immutable::SortedSet<DocumentReference, DocumentReference::ById> by_id_;
+  immutable::SortedSet<DocumentKeyReference, DocumentKeyReference::ByKey>
+      by_key_;
+  immutable::SortedSet<DocumentKeyReference, DocumentKeyReference::ById> by_id_;
 };
 
 }  // namespace local

Firestore/core/src/firebase/firestore/nanopb/nanopb_string.cc

@@ -19,6 +19,8 @@
 #include <cstdlib>
 #include <utility>
 
+#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
+
 namespace firebase {
 namespace firestore {
 namespace nanopb {
@@ -28,7 +30,7 @@ String::~String() {
 }
 
 /* static */ pb_bytes_array_t* String::MakeBytesArray(absl::string_view value) {
-  auto size = static_cast<pb_size_t>(value.size());
+  pb_size_t size = CheckedSize(value.size());
 
   // Allocate one extra byte for the null terminator that's not necessarily
   // there in a string_view. As long as we're making a copy, might as well