From 7909b913ef98b66c6c09b3cbe69e2c2f85658e02 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 25 Aug 2022 08:15:06 -0700 Subject: [PATCH] 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 (https://github.com/facebook/react-native/commit/57dd48b2464ac04b860f2f69cb4f131990fe4dbd) 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: philIip Differential Revision: D38938632 fbshipit-source-id: 1dc9a544ca679463a8aad5cc632bd918f42b15d1 --- ReactCommon/react/renderer/core/RawValue.h | 24 ++++----- .../react/renderer/core/propsConversions.h | 52 ++++++------------- 2 files changed, 27 insertions(+), 49 deletions(-) diff --git a/ReactCommon/react/renderer/core/RawValue.h b/ReactCommon/react/renderer/core/RawValue.h index d541f11a16ed9d..c69f9e43ceacde 100644 --- a/ReactCommon/react/renderer/core/RawValue.h +++ b/ReactCommon/react/renderer/core/RawValue.h @@ -96,7 +96,7 @@ class RawValue { * Casts the value to a specified type. */ template - 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(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(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 static std::vector castValue( const folly::dynamic &dynamic, - std::vector *type) noexcept { + std::vector *type) { react_native_assert(dynamic.isArray()); auto result = std::vector{}; result.reserve(dynamic.size()); @@ -258,7 +254,7 @@ class RawValue { template static std::vector> castValue( const folly::dynamic &dynamic, - std::vector> *type) noexcept { + std::vector> *type) { react_native_assert(dynamic.isArray()); auto result = std::vector>{}; result.reserve(dynamic.size()); @@ -271,7 +267,7 @@ class RawValue { template static butter::map castValue( const folly::dynamic &dynamic, - butter::map *type) noexcept { + butter::map *type) { react_native_assert(dynamic.isObject()); auto result = butter::map{}; for (const auto &item : dynamic.items()) { diff --git a/ReactCommon/react/renderer/core/propsConversions.h b/ReactCommon/react/renderer/core/propsConversions.h index adbc07860736a0..c4221f977567f9 100644 --- a/ReactCommon/react/renderer/core/propsConversions.h +++ b/ReactCommon/react/renderer/core/propsConversions.h @@ -10,9 +10,9 @@ #include #include -#include #include #include +#include #include #include #include @@ -43,18 +43,18 @@ template void fromRawValue( const PropsParserContext &context, RawValue const &rawValue, - std::optional &result) { - T res{}; - fromRawValue(context, rawValue, res); - result = std::optional(res); + T &result) { + result = (T)rawValue; } template void fromRawValue( const PropsParserContext &context, RawValue const &rawValue, - T &result) { - result = (T)rawValue; + std::optional &result) { + T resultValue; + fromRawValue(context, rawValue, resultValue); + result = std::optional{std::move(resultValue)}; } template @@ -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 -static std::optional convertRawProp( - const PropsParserContext &context, - RawProps const &rawProps, - char const *name, - std::optional const &sourceValue, - std::optional 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(key) << "': " << e.what(); return defaultValue; } - - T result; - fromRawValue(context, *rawValue, result); - return std::optional{result}; } } // namespace react