Skip to content

Commit

Permalink
Clarify const-ness of JSI references
Browse files Browse the repository at this point in the history
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<T>` only makes the pointer const, as opposed to
`shared_ptr<const T>`.

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
  • Loading branch information
neildhar authored and facebook-github-bot committed Dec 16, 2022
1 parent c4862a2 commit 03b17d9
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 40 deletions.
20 changes: 11 additions & 9 deletions ReactCommon/jsc/JSCRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
26 changes: 16 additions & 10 deletions ReactCommon/jsi/jsi/decorator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
Expand All @@ -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);
};

Expand All @@ -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);
};

Expand Down Expand Up @@ -656,12 +659,14 @@ class WithRuntimeDecorator : public RuntimeDecorator<Plain, Base> {
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);
Expand Down Expand Up @@ -696,7 +701,7 @@ class WithRuntimeDecorator : public RuntimeDecorator<Plain, Base> {
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);
};
Expand Down Expand Up @@ -725,7 +730,8 @@ class WithRuntimeDecorator : public RuntimeDecorator<Plain, Base> {
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);
};
Expand Down
12 changes: 7 additions & 5 deletions ReactCommon/jsi/jsi/jsi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,19 +111,21 @@ inline bool Object::hasProperty(Runtime& runtime, const PropNameID& name)
}

template <typename T>
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<T>(value));
}

template <typename T>
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<T>(value)));
}

template <typename T>
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<T>(value)));
}
Expand Down Expand Up @@ -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 <typename T>
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<T>(value)));
}
Expand Down
38 changes: 22 additions & 16 deletions ReactCommon/jsi/jsi/jsi.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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 <typename T>
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 <typename T>
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 <typename T>
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.
Expand Down Expand Up @@ -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);
}

Expand All @@ -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;
};
Expand Down Expand Up @@ -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 <typename T>
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.
Expand All @@ -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);
}

Expand All @@ -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);
}

Expand Down

0 comments on commit 03b17d9

Please sign in to comment.