Add key to prop conversion errors

Summary:
Improve errors thrown when prop conversion fails by adding the property being converted. Removes the specialization of `convertRawProp` for `std::optional` since we can handle that in a `fromRawValue` specialization instead.

To make this work, we need to remove `noexcept` from a number of calls. This noexcept behaviour was making these exceptions effectively uncatcheable. The original motivation of D23787492 (57dd48b) is correct, as we cannot reliably pass on exceptions to JS and assume that the state will be recoverable, so instead we log an error and carry on with the default value available. We should improve how the error gets reported to the user, as it will currently be hidden in logcat.

Changelog: [Internal]

Reviewed By: JoshuaGross

Differential Revision: D37055069

fbshipit-source-id: 8ce121a9b187f068d3ab790c6fae66d53e6338d2

ReactCommon/react/renderer/core/RawValue.h

@@ -96,7 +96,7 @@ class RawValue {
    * Casts the value to a specified type.
    */
   template <typename T>
-  explicit operator T() const noexcept {
+  explicit operator T() const {
     return castValue(dynamic_, (T *)nullptr);
   }
 
@@ -212,40 +212,36 @@ class RawValue {
     return RawValue(dynamic);
   }
 
-  static bool castValue(const folly::dynamic &dynamic, bool *type) noexcept {
+  static bool castValue(const folly::dynamic &dynamic, bool *type) {
     return dynamic.getBool();
   }
 
-  static int castValue(const folly::dynamic &dynamic, int *type) noexcept {
+  static int castValue(const folly::dynamic &dynamic, int *type) {
     return static_cast<int>(dynamic.asInt());
   }
 
-  static int64_t castValue(
-      const folly::dynamic &dynamic,
-      int64_t *type) noexcept {
+  static int64_t castValue(const folly::dynamic &dynamic, int64_t *type) {
     return dynamic.asInt();
   }
 
-  static float castValue(const folly::dynamic &dynamic, float *type) noexcept {
+  static float castValue(const folly::dynamic &dynamic, float *type) {
     return static_cast<float>(dynamic.asDouble());
   }
 
-  static double castValue(
-      const folly::dynamic &dynamic,
-      double *type) noexcept {
+  static double castValue(const folly::dynamic &dynamic, double *type) {
     return dynamic.asDouble();
   }
 
   static std::string castValue(
       const folly::dynamic &dynamic,
-      std::string *type) noexcept {
+      std::string *type) {
     return dynamic.getString();
   }
 
   template <typename T>
   static std::vector<T> castValue(
       const folly::dynamic &dynamic,
-      std::vector<T> *type) noexcept {
+      std::vector<T> *type) {
     react_native_assert(dynamic.isArray());
     auto result = std::vector<T>{};
     result.reserve(dynamic.size());
@@ -258,7 +254,7 @@ class RawValue {
   template <typename T>
   static std::vector<std::vector<T>> castValue(
       const folly::dynamic &dynamic,
-      std::vector<std::vector<T>> *type) noexcept {
+      std::vector<std::vector<T>> *type) {
     react_native_assert(dynamic.isArray());
     auto result = std::vector<std::vector<T>>{};
     result.reserve(dynamic.size());
@@ -271,7 +267,7 @@ class RawValue {
   template <typename T>
   static butter::map<std::string, T> castValue(
       const folly::dynamic &dynamic,
-      butter::map<std::string, T> *type) noexcept {
+      butter::map<std::string, T> *type) {
     react_native_assert(dynamic.isObject());
     auto result = butter::map<std::string, T>{};
     for (const auto &item : dynamic.items()) {

ReactCommon/react/renderer/core/propsConversions.h

@@ -10,9 +10,9 @@
 #include <optional>
 
 #include <folly/Likely.h>
-#include <folly/dynamic.h>
 #include <react/renderer/core/PropsParserContext.h>
 #include <react/renderer/core/RawProps.h>
+#include <react/renderer/core/RawPropsKey.h>
 #include <react/renderer/graphics/Color.h>
 #include <react/renderer/graphics/Geometry.h>
 #include <react/renderer/graphics/conversions.h>
@@ -43,18 +43,18 @@ template <typename T>
 void fromRawValue(
     const PropsParserContext &context,
     RawValue const &rawValue,
-    std::optional<T> &result) {
-  T res{};
-  fromRawValue(context, rawValue, res);
-  result = std::optional<T>(res);
+    T &result) {
+  result = (T)rawValue;
 }
 
 template <typename T>
 void fromRawValue(
     const PropsParserContext &context,
     RawValue const &rawValue,
-    T &result) {
-  result = (T)rawValue;
+    std::optional<T> &result) {
+  T resultValue;
+  fromRawValue(context, rawValue, resultValue);
+  result = std::optional<T>{std::move(resultValue)};
 }
 
 template <typename T>
@@ -119,7 +119,6 @@ T convertRawProp(
     char const *namePrefix = nullptr,
     char const *nameSuffix = nullptr) {
   const auto *rawValue = rawProps.at(name, namePrefix, nameSuffix);
-
   if (LIKELY(rawValue == nullptr)) {
     return sourceValue;
   }
@@ -130,35 +129,18 @@ T convertRawProp(
     return defaultValue;
   }
 
-  T result;
-  fromRawValue(context, *rawValue, result);
-  return result;
-}
-
-template <typename T>
-static std::optional<T> convertRawProp(
-    const PropsParserContext &context,
-    RawProps const &rawProps,
-    char const *name,
-    std::optional<T> const &sourceValue,
-    std::optional<T> const &defaultValue,
-    char const *namePrefix = nullptr,
-    char const *nameSuffix = nullptr) {
-  const auto *rawValue = rawProps.at(name, namePrefix, nameSuffix);
-
-  if (LIKELY(rawValue == nullptr)) {
-    return sourceValue;
-  }
-
-  // Special case: `null` always means `the prop was removed, use default
-  // value`.
-  if (UNLIKELY(!rawValue->hasValue())) {
+  try {
+    T result;
+    fromRawValue(context, *rawValue, result);
+    return result;
+  } catch (const std::exception &e) {
+    // In case of errors, log the error and fall back to the default
+    RawPropsKey key{namePrefix, name, nameSuffix};
+    // TODO: report this using ErrorUtils so it's more visible to the user
+    LOG(ERROR) << "Error while converting prop '"
+               << static_cast<std::string>(key) << "': " << e.what();
     return defaultValue;
   }
-
-  T result;
-  fromRawValue(context, *rawValue, result);
-  return std::optional<T>{result};
 }
 
 } // namespace react