From 03b17d9af7e4e3ad3f9ec078b76d0ffa33a3290e Mon Sep 17 00:00:00 2001 From: Neil Dhar Date: Thu, 15 Dec 2022 19:18:15 -0800 Subject: [PATCH] Clarify const-ness of JSI references Summary: Many operations on references in JS can modify the referent by executing additional JS, including operations that we currently mark as const such as `getProperty`. Because of this, the current distinction between const and non-const operations on references like `jsi::Object` is somewhat arbitrary. A more consistent approach is to mark all operations as const, so that it is clear that the const-ness applies to the reference and not the referent. This is analogous to how smart pointers work, since something like `const shared_ptr` only makes the pointer const, as opposed to `shared_ptr`. This also gives users better guarantees and more flexibility in where these references may be used. Changelog: [General][Changed] - Mark methods on JSI references as const. Reviewed By: fbmal7 Differential Revision: D41599116 fbshipit-source-id: 40b1537581b09c5a34d0928ee04e7db2b50d26ea --- ReactCommon/jsc/JSCRuntime.cpp | 20 +++++++++-------- ReactCommon/jsi/jsi/decorator.h | 26 +++++++++++++--------- ReactCommon/jsi/jsi/jsi-inl.h | 12 ++++++----- ReactCommon/jsi/jsi/jsi.h | 38 +++++++++++++++++++-------------- 4 files changed, 56 insertions(+), 40 deletions(-) diff --git a/ReactCommon/jsc/JSCRuntime.cpp b/ReactCommon/jsc/JSCRuntime.cpp index bbc606561ee353..7958519e683f6d 100644 --- a/ReactCommon/jsc/JSCRuntime.cpp +++ b/ReactCommon/jsc/JSCRuntime.cpp @@ -187,11 +187,11 @@ class JSCRuntime : public jsi::Runtime { bool hasProperty(const jsi::Object &, const jsi::String &name) override; bool hasProperty(const jsi::Object &, const jsi::PropNameID &name) override; void setPropertyValue( - jsi::Object &, + const jsi::Object &, const jsi::String &name, const jsi::Value &value) override; void setPropertyValue( - jsi::Object &, + const jsi::Object &, const jsi::PropNameID &name, const jsi::Value &value) override; bool isArray(const jsi::Object &) const override; @@ -203,7 +203,7 @@ class JSCRuntime : public jsi::Runtime { // TODO: revisit this implementation jsi::WeakObject createWeakObject(const jsi::Object &) override; - jsi::Value lockWeakObject(jsi::WeakObject &) override; + jsi::Value lockWeakObject(const jsi::WeakObject &) override; jsi::Array createArray(size_t length) override; jsi::ArrayBuffer createArrayBuffer( @@ -212,8 +212,10 @@ class JSCRuntime : public jsi::Runtime { size_t size(const jsi::ArrayBuffer &) override; uint8_t *data(const jsi::ArrayBuffer &) override; jsi::Value getValueAtIndex(const jsi::Array &, size_t i) override; - void setValueAtIndexImpl(jsi::Array &, size_t i, const jsi::Value &value) - override; + void setValueAtIndexImpl( + const jsi::Array &, + size_t i, + const jsi::Value &value) override; jsi::Function createFunctionFromHostFunction( const jsi::PropNameID &name, @@ -949,7 +951,7 @@ bool JSCRuntime::hasProperty( } void JSCRuntime::setPropertyValue( - jsi::Object &object, + const jsi::Object &object, const jsi::PropNameID &name, const jsi::Value &value) { JSValueRef exc = nullptr; @@ -964,7 +966,7 @@ void JSCRuntime::setPropertyValue( } void JSCRuntime::setPropertyValue( - jsi::Object &object, + const jsi::Object &object, const jsi::String &name, const jsi::Value &value) { JSValueRef exc = nullptr; @@ -1064,7 +1066,7 @@ jsi::WeakObject JSCRuntime::createWeakObject(const jsi::Object &obj) { #endif } -jsi::Value JSCRuntime::lockWeakObject(jsi::WeakObject &obj) { +jsi::Value JSCRuntime::lockWeakObject(const jsi::WeakObject &obj) { #ifdef RN_FABRIC_ENABLED // TODO: revisit this implementation JSObjectRef objRef = objectRef(obj); @@ -1107,7 +1109,7 @@ jsi::Value JSCRuntime::getValueAtIndex(const jsi::Array &arr, size_t i) { } void JSCRuntime::setValueAtIndexImpl( - jsi::Array &arr, + const jsi::Array &arr, size_t i, const jsi::Value &value) { JSValueRef exc = nullptr; diff --git a/ReactCommon/jsi/jsi/decorator.h b/ReactCommon/jsi/jsi/decorator.h index 9df3a2bc8f505c..baece80fe71d0f 100644 --- a/ReactCommon/jsi/jsi/decorator.h +++ b/ReactCommon/jsi/jsi/decorator.h @@ -264,11 +264,13 @@ class RuntimeDecorator : public Base, private jsi::Instrumentation { bool hasProperty(const Object& o, const String& name) override { return plain_.hasProperty(o, name); }; - void setPropertyValue(Object& o, const PropNameID& name, const Value& value) - override { + void setPropertyValue( + const Object& o, + const PropNameID& name, + const Value& value) override { plain_.setPropertyValue(o, name, value); }; - void setPropertyValue(Object& o, const String& name, const Value& value) + void setPropertyValue(const Object& o, const String& name, const Value& value) override { plain_.setPropertyValue(o, name, value); }; @@ -295,7 +297,7 @@ class RuntimeDecorator : public Base, private jsi::Instrumentation { WeakObject createWeakObject(const Object& o) override { return plain_.createWeakObject(o); }; - Value lockWeakObject(WeakObject& wo) override { + Value lockWeakObject(const WeakObject& wo) override { return plain_.lockWeakObject(wo); }; @@ -318,7 +320,8 @@ class RuntimeDecorator : public Base, private jsi::Instrumentation { Value getValueAtIndex(const Array& a, size_t i) override { return plain_.getValueAtIndex(a, i); }; - void setValueAtIndexImpl(Array& a, size_t i, const Value& value) override { + void setValueAtIndexImpl(const Array& a, size_t i, const Value& value) + override { plain_.setValueAtIndexImpl(a, i, value); }; @@ -656,12 +659,14 @@ class WithRuntimeDecorator : public RuntimeDecorator { Around around{with_}; return RD::hasProperty(o, name); }; - void setPropertyValue(Object& o, const PropNameID& name, const Value& value) - override { + void setPropertyValue( + const Object& o, + const PropNameID& name, + const Value& value) override { Around around{with_}; RD::setPropertyValue(o, name, value); }; - void setPropertyValue(Object& o, const String& name, const Value& value) + void setPropertyValue(const Object& o, const String& name, const Value& value) override { Around around{with_}; RD::setPropertyValue(o, name, value); @@ -696,7 +701,7 @@ class WithRuntimeDecorator : public RuntimeDecorator { Around around{with_}; return RD::createWeakObject(o); }; - Value lockWeakObject(WeakObject& wo) override { + Value lockWeakObject(const WeakObject& wo) override { Around around{with_}; return RD::lockWeakObject(wo); }; @@ -725,7 +730,8 @@ class WithRuntimeDecorator : public RuntimeDecorator { Around around{with_}; return RD::getValueAtIndex(a, i); }; - void setValueAtIndexImpl(Array& a, size_t i, const Value& value) override { + void setValueAtIndexImpl(const Array& a, size_t i, const Value& value) + override { Around around{with_}; RD::setValueAtIndexImpl(a, i, value); }; diff --git a/ReactCommon/jsi/jsi/jsi-inl.h b/ReactCommon/jsi/jsi/jsi-inl.h index a7e51583492668..4ce00adb883c59 100644 --- a/ReactCommon/jsi/jsi/jsi-inl.h +++ b/ReactCommon/jsi/jsi/jsi-inl.h @@ -111,19 +111,21 @@ inline bool Object::hasProperty(Runtime& runtime, const PropNameID& name) } template -void Object::setProperty(Runtime& runtime, const char* name, T&& value) { +void Object::setProperty(Runtime& runtime, const char* name, T&& value) const { setProperty( runtime, String::createFromAscii(runtime, name), std::forward(value)); } template -void Object::setProperty(Runtime& runtime, const String& name, T&& value) { +void Object::setProperty(Runtime& runtime, const String& name, T&& value) + const { setPropertyValue( runtime, name, detail::toValue(runtime, std::forward(value))); } template -void Object::setProperty(Runtime& runtime, const PropNameID& name, T&& value) { +void Object::setProperty(Runtime& runtime, const PropNameID& name, T&& value) + const { setPropertyValue( runtime, name, detail::toValue(runtime, std::forward(value))); } @@ -229,12 +231,12 @@ inline Array Object::getPropertyNames(Runtime& runtime) const { return runtime.getPropertyNames(*this); } -inline Value WeakObject::lock(Runtime& runtime) { +inline Value WeakObject::lock(Runtime& runtime) const { return runtime.lockWeakObject(*this); } template -void Array::setValueAtIndex(Runtime& runtime, size_t i, T&& value) { +void Array::setValueAtIndex(Runtime& runtime, size_t i, T&& value) const { setValueAtIndexImpl( runtime, i, detail::toValue(runtime, std::forward(value))); } diff --git a/ReactCommon/jsi/jsi/jsi.h b/ReactCommon/jsi/jsi/jsi.h index b50780964ad23a..ec7fd7723f5402 100644 --- a/ReactCommon/jsi/jsi/jsi.h +++ b/ReactCommon/jsi/jsi/jsi.h @@ -336,10 +336,12 @@ class JSI_EXPORT Runtime { virtual Value getProperty(const Object&, const String& name) = 0; virtual bool hasProperty(const Object&, const PropNameID& name) = 0; virtual bool hasProperty(const Object&, const String& name) = 0; + virtual void setPropertyValue( + const Object&, + const PropNameID& name, + const Value& value) = 0; virtual void - setPropertyValue(Object&, const PropNameID& name, const Value& value) = 0; - virtual void - setPropertyValue(Object&, const String& name, const Value& value) = 0; + setPropertyValue(const Object&, const String& name, const Value& value) = 0; virtual bool isArray(const Object&) const = 0; virtual bool isArrayBuffer(const Object&) const = 0; @@ -349,7 +351,7 @@ class JSI_EXPORT Runtime { virtual Array getPropertyNames(const Object&) = 0; virtual WeakObject createWeakObject(const Object&) = 0; - virtual Value lockWeakObject(WeakObject&) = 0; + virtual Value lockWeakObject(const WeakObject&) = 0; virtual Array createArray(size_t length) = 0; virtual ArrayBuffer createArrayBuffer( @@ -358,7 +360,8 @@ class JSI_EXPORT Runtime { virtual size_t size(const ArrayBuffer&) = 0; virtual uint8_t* data(const ArrayBuffer&) = 0; virtual Value getValueAtIndex(const Array&, size_t i) = 0; - virtual void setValueAtIndexImpl(Array&, size_t i, const Value& value) = 0; + virtual void + setValueAtIndexImpl(const Array&, size_t i, const Value& value) = 0; virtual Function createFunctionFromHostFunction( const PropNameID& name, @@ -666,7 +669,7 @@ class JSI_EXPORT Object : public Pointer { } /// \return the result of `this instanceOf ctor` in JS. - bool instanceOf(Runtime& rt, const Function& ctor) { + bool instanceOf(Runtime& rt, const Function& ctor) const { return rt.instanceOf(*this, ctor); } @@ -701,19 +704,19 @@ class JSI_EXPORT Object : public Pointer { /// used to make one: nullptr_t, bool, double, int, const char*, /// String, or Object. template - void setProperty(Runtime& runtime, const char* name, T&& value); + void setProperty(Runtime& runtime, const char* name, T&& value) const; /// Sets the property value from a Value or anything which can be /// used to make one: nullptr_t, bool, double, int, const char*, /// String, or Object. template - void setProperty(Runtime& runtime, const String& name, T&& value); + void setProperty(Runtime& runtime, const String& name, T&& value) const; /// Sets the property value from a Value or anything which can be /// used to make one: nullptr_t, bool, double, int, const char*, /// String, or Object. template - void setProperty(Runtime& runtime, const PropNameID& name, T&& value); + void setProperty(Runtime& runtime, const PropNameID& name, T&& value) const; /// \return true iff JS \c Array.isArray() would return \c true. If /// so, then \c getArray() will succeed. @@ -832,15 +835,17 @@ class JSI_EXPORT Object : public Pointer { Array getPropertyNames(Runtime& runtime) const; protected: - void - setPropertyValue(Runtime& runtime, const String& name, const Value& value) { + void setPropertyValue( + Runtime& runtime, + const String& name, + const Value& value) const { return runtime.setPropertyValue(*this, name, value); } void setPropertyValue( Runtime& runtime, const PropNameID& name, - const Value& value) { + const Value& value) const { return runtime.setPropertyValue(*this, name, value); } @@ -866,7 +871,7 @@ class JSI_EXPORT WeakObject : public Pointer { /// otherwise returns \c undefined. Note that this method has nothing to do /// with threads or concurrency. The name is based on std::weak_ptr::lock() /// which serves a similar purpose. - Value lock(Runtime& runtime); + Value lock(Runtime& runtime) const; friend class Runtime; }; @@ -902,7 +907,7 @@ class JSI_EXPORT Array : public Object { /// value behaves as with Object::setProperty(). If \c i is out of /// range [ 0..\c length ] throws a JSIException. template - void setValueAtIndex(Runtime& runtime, size_t i, T&& value); + void setValueAtIndex(Runtime& runtime, size_t i, T&& value) const; /// There is no current API for changing the size of an array once /// created. We'll probably need that eventually. @@ -920,7 +925,8 @@ class JSI_EXPORT Array : public Object { friend class Object; friend class Value; - void setValueAtIndexImpl(Runtime& runtime, size_t i, const Value& value) { + void setValueAtIndexImpl(Runtime& runtime, size_t i, const Value& value) + const { return runtime.setValueAtIndexImpl(*this, i, value); } @@ -946,7 +952,7 @@ class JSI_EXPORT ArrayBuffer : public Object { return runtime.size(*this); } - uint8_t* data(Runtime& runtime) { + uint8_t* data(Runtime& runtime) const { return runtime.data(*this); }