From d470dee0c53993cf0c46e50d3594da6738a87f69 Mon Sep 17 00:00:00 2001 From: Mathias Stearn Date: Mon, 22 May 2023 12:04:58 -0700 Subject: [PATCH] Convert static_assert to enable_if in jsi::Value::Value(T&&) ctor (#37520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37520 Results in better error messages, especially when trying to copy a Value. With this change: ``` ../API/hermes/hermes.cpp: In member function ‘facebook::jsi::Value facebook::hermes::HermesRuntimeImpl::valueFromHermesValue(hermes::vm::HermesValue)’: ../API/hermes/hermes.cpp:726:31: error: use of deleted function ‘facebook::jsi::Value::Value(const facebook::jsi::Value&)’ 726 | auto copy = jsi::Value(val); | ^ In file included from ../API/hermes/hermes.h:18, from ../API/hermes/hermes.cpp:8: ../API/jsi/jsi/jsi.h:938:18: note: ‘facebook::jsi::Value::Value(const facebook::jsi::Value&)’ is implicitly declared as deleted because ‘facebook::jsi::Value’ declares a move constructor or move assignment operator 938 | class JSI_EXPORT Value { | ^~~~~ ``` Before: ``` In file included from ../API/hermes/hermes.h:18, from ../API/hermes/hermes.cpp:8: ../API/jsi/jsi/jsi.h: In instantiation of ‘facebook::jsi::Value::Value(T&&) [with T = facebook::jsi::Value&]’: ../API/hermes/hermes.cpp:726:31: required from here ../API/jsi/jsi/jsi.h:963:49: error: no matching function for call to ‘facebook::jsi::Value::kindOf(facebook::jsi::Value&)’ 963 | /* implicit */ Value(T&& other) : Value(kindOf(other)) { | ~~~~~~^~~~~~~ ../API/jsi/jsi/jsi.h:1176:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::Symbol&)’ 1176 | constexpr static ValueKind kindOf(const Symbol&) { | ^~~~~~ ../API/jsi/jsi/jsi.h:1176:37: note: no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::Symbol&’ 1176 | constexpr static ValueKind kindOf(const Symbol&) { | ^~~~~~~~~~~~~ ../API/jsi/jsi/jsi.h:1179:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::String&)’ 1179 | constexpr static ValueKind kindOf(const String&) { | ^~~~~~ ../API/jsi/jsi/jsi.h:1179:37: note: no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::String&’ 1179 | constexpr static ValueKind kindOf(const String&) { | ^~~~~~~~~~~~~ ../API/jsi/jsi/jsi.h:1182:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::Object&)’ 1182 | constexpr static ValueKind kindOf(const Object&) { | ^~~~~~ ../API/jsi/jsi/jsi.h:1182:37: note: no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::Object&’ 1182 | constexpr static ValueKind kindOf(const Object&) { | ^~~~~~~~~~~~~ ../API/jsi/jsi/jsi.h:966:47: error: static assertion failed: Value cannot be implicitly move-constructed from this type 965 | std::is_base_of::value || | ~~~~~~~~ 966 | std::is_base_of::value || | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~ 967 | std::is_base_of::value, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../API/jsi/jsi/jsi.h:966:47: note: ‘((((bool)std::integral_constant::value) || ((bool)std::integral_constant::value)) || ((bool)std::integral_constant::value))’ evaluates to false ../API/jsi/jsi/jsi.h:969:5: error: new cannot be applied to a reference type 969 | new (&data_.pointer) T(std::move(other)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` Among other things, this puts the actual error on the consuming code where the error is, which results in those nice red squiggles under it while typing to tell you that you screwed up. X-link: https://github.com/facebook/hermes/pull/526 Test Plan: This should only convert one type of compiler error to another, so existing tests should be sufficient. Reviewed By: tmikov Differential Revision: D46005344 Pulled By: neildhar fbshipit-source-id: 194e0483177770df578cb864281d66c88d4cdb7e --- packages/react-native/ReactCommon/jsi/jsi/jsi.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/react-native/ReactCommon/jsi/jsi/jsi.h b/packages/react-native/ReactCommon/jsi/jsi/jsi.h index ec7fd7723f5402..dd5ebe1eb90c6d 100644 --- a/packages/react-native/ReactCommon/jsi/jsi/jsi.h +++ b/packages/react-native/ReactCommon/jsi/jsi/jsi.h @@ -1098,14 +1098,14 @@ class JSI_EXPORT Value { } /// Moves a Symbol, String, or Object rvalue into a new JS value. - template + template < + typename T, + typename = std::enable_if_t< + std::is_base_of::value || + std::is_base_of::value || + std::is_base_of::value || + std::is_base_of::value>> /* implicit */ Value(T&& other) : Value(kindOf(other)) { - static_assert( - std::is_base_of::value || - std::is_base_of::value || - std::is_base_of::value || - std::is_base_of::value, - "Value cannot be implicitly move-constructed from this type"); new (&data_.pointer) T(std::move(other)); }