From 11c9f480af29bf0b0baecc1bcc58bfa76827236e Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Fri, 22 Nov 2019 20:05:04 -0800
Subject: [PATCH] Fabric: Handling duplicating sub-props in RawPropsKeyMap

Summary:
This diff makes the parsing *Props object with sub-props with duplicating names deterministic: only the first one will be assigned. Before this change, the order was not guaranteed because `qsort` sorting algorithm is not stable and because binary search that we use to search in the sorted array does not handle duplicates deterministically.

The behavior when the only sub-prop (the first one) is being assigned actually desirable feature that exists on purpose. There are places where it's desired behavior (like TextInputProps) and in cases where it's not desired (we don't really have those), it's easy to "fix" implementing the custom constructor.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: JoshuaGross

Differential Revision: D18607657

fbshipit-source-id: aa53a320e6a48877a0dd1b9351dfcc8a9f419b38
---
 .../fabric/core/primitives/RawPropsKeyMap.cpp | 38 +++++++++++++------
 .../fabric/core/primitives/RawPropsKeyMap.h   |  7 +++-
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/ReactCommon/fabric/core/primitives/RawPropsKeyMap.cpp b/ReactCommon/fabric/core/primitives/RawPropsKeyMap.cpp
index 19c8749541d..889cdbead36 100644
--- a/ReactCommon/fabric/core/primitives/RawPropsKeyMap.cpp
+++ b/ReactCommon/fabric/core/primitives/RawPropsKeyMap.cpp
@@ -7,6 +7,7 @@
 
 #include "RawPropsKeyMap.h"
 
+#include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <cstring>
@@ -14,15 +15,19 @@
 namespace facebook {
 namespace react {
 
-int RawPropsKeyMap::comparator(void const *lhs, void const *rhs) noexcept {
-  auto a = static_cast<RawPropsKeyMap::Item const *>(lhs);
-  auto b = static_cast<RawPropsKeyMap::Item const *>(rhs);
+bool RawPropsKeyMap::hasSameName(Item const &lhs, Item const &rhs) noexcept {
+  return lhs.length == rhs.length &&
+      (std::memcmp(lhs.name, rhs.name, lhs.length) == 0);
+}
 
-  if (a->length != b->length) {
-    return a->length - b->length;
+bool RawPropsKeyMap::shouldFirstOneBeBeforeSecondOne(
+    Item const &lhs,
+    Item const &rhs) noexcept {
+  if (lhs.length != rhs.length) {
+    return lhs.length < rhs.length;
   }
 
-  return std::memcmp(a->name, b->name, a->length);
+  return std::memcmp(lhs.name, rhs.name, rhs.length) < 0;
 }
 
 void RawPropsKeyMap::insert(
@@ -36,11 +41,22 @@ void RawPropsKeyMap::insert(
 
 void RawPropsKeyMap::reindex() noexcept {
   // Sorting `items_` by property names length and then lexicographically.
-  std::qsort(
-      items_.data(),
-      items_.size(),
-      sizeof(decltype(items_)::value_type),
-      &RawPropsKeyMap::comparator);
+  // Note, sort algorithm must be stable.
+  std::stable_sort(
+      items_.begin(),
+      items_.end(),
+      &RawPropsKeyMap::shouldFirstOneBeBeforeSecondOne);
+
+  // Filtering out duplicating keys.
+  // If some `*Props` object requests a prop more than once, only the first
+  // request will be fulfilled. E.g. `TextInputProps` class has a sub-property
+  // `backgroundColor` twice, the first time as part of `ViewProps` base-class
+  // and the second as part of `BaseTextProps` base-class. In this
+  // configuration, the only one which comes first (from `ViewProps`, which
+  // appear first) will be assigned.
+  items_.erase(
+      std::unique(items_.begin(), items_.end(), &RawPropsKeyMap::hasSameName),
+      items_.end());
 
   buckets_.resize(kPropNameLengthHardCap);
 
diff --git a/ReactCommon/fabric/core/primitives/RawPropsKeyMap.h b/ReactCommon/fabric/core/primitives/RawPropsKeyMap.h
index f5ecd2d2117..82dd163f34a 100644
--- a/ReactCommon/fabric/core/primitives/RawPropsKeyMap.h
+++ b/ReactCommon/fabric/core/primitives/RawPropsKeyMap.h
@@ -44,14 +44,17 @@ class RawPropsKeyMap final {
       RawPropsPropNameLength length) noexcept;
 
  private:
-  static int comparator(void const *lhs, void const *rhs) noexcept;
-
   struct Item {
     RawPropsValueIndex value;
     RawPropsPropNameLength length;
     char name[kPropNameLengthHardCap];
   };
 
+  static bool shouldFirstOneBeBeforeSecondOne(
+      Item const &lhs,
+      Item const &rhs) noexcept;
+  static bool hasSameName(Item const &lhs, Item const &rhs) noexcept;
+
   better::small_vector<Item, kNumberOfExplicitlySpecifedPropsSoftCap> items_{};
   better::small_vector<RawPropsPropNameLength, kPropNameLengthHardCap>
       buckets_{};