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 2c35622 commit b816665
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 53 deletions.
8 changes: 4 additions & 4 deletions API/hermes/TracingRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ bool TracingRuntime::hasProperty(
}

void TracingRuntime::setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::String &name,
const jsi::Value &value) {
trace_.emplace_back<SynthTrace::SetPropertyRecord>(
Expand All @@ -417,7 +417,7 @@ void TracingRuntime::setPropertyValue(
}

void TracingRuntime::setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::PropNameID &name,
const jsi::Value &value) {
trace_.emplace_back<SynthTrace::SetPropertyRecord>(
Expand Down Expand Up @@ -462,7 +462,7 @@ jsi::WeakObject TracingRuntime::createWeakObject(const jsi::Object &o) {
return RD::createWeakObject(o);
}

jsi::Value TracingRuntime::lockWeakObject(jsi::WeakObject &wo) {
jsi::Value TracingRuntime::lockWeakObject(const jsi::WeakObject &wo) {
// See comment in TracingRuntime::createWeakObject for why this function isn't
// traced.
return RD::lockWeakObject(wo);
Expand Down Expand Up @@ -505,7 +505,7 @@ jsi::Value TracingRuntime::getValueAtIndex(const jsi::Array &arr, size_t i) {
}

void TracingRuntime::setValueAtIndexImpl(
jsi::Array &arr,
const jsi::Array &arr,
size_t i,
const jsi::Value &value) {
trace_.emplace_back<SynthTrace::ArrayWriteRecord>(
Expand Down
12 changes: 7 additions & 5 deletions API/hermes/TracingRuntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,19 @@ class TracingRuntime : public jsi::RuntimeDecorator<jsi::Runtime> {
override;

void setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::String &name,
const jsi::Value &value) override;
void setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::PropNameID &name,
const jsi::Value &value) override;

jsi::Array getPropertyNames(const jsi::Object &o) override;

jsi::WeakObject createWeakObject(const jsi::Object &o) override;

jsi::Value lockWeakObject(jsi::WeakObject &wo) override;
jsi::Value lockWeakObject(const jsi::WeakObject &wo) override;

jsi::Array createArray(size_t length) override;
jsi::ArrayBuffer createArrayBuffer(
Expand All @@ -100,8 +100,10 @@ class TracingRuntime : public jsi::RuntimeDecorator<jsi::Runtime> {

jsi::Value getValueAtIndex(const jsi::Array &arr, size_t i) override;

void setValueAtIndexImpl(jsi::Array &arr, size_t i, const jsi::Value &value)
override;
void setValueAtIndexImpl(
const jsi::Array &arr,
size_t i,
const jsi::Value &value) override;

jsi::Function createFunctionFromHostFunction(
const jsi::PropNameID &name,
Expand Down
29 changes: 16 additions & 13 deletions API/hermes/hermes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -506,11 +506,12 @@ class HermesRuntimeImpl final : public HermesRuntime,
return ::hermes::vm::Handle<::hermes::vm::JSArrayBuffer>::vmcast(&phv(arr));
}

static ::hermes::vm::WeakRoot<vm::JSObject> &weakRoot(jsi::Pointer &pointer) {
static const ::hermes::vm::WeakRoot<vm::JSObject> &weakRoot(
const jsi::Pointer &pointer) {
assert(
dynamic_cast<WeakRefPointerValue *>(getPointerValue(pointer)) &&
dynamic_cast<const WeakRefPointerValue *>(getPointerValue(pointer)) &&
"Pointer does not contain a WeakRefPointerValue");
return static_cast<WeakRefPointerValue *>(getPointerValue(pointer))
return static_cast<const WeakRefPointerValue *>(getPointerValue(pointer))
->value();
}

Expand Down Expand Up @@ -645,11 +646,11 @@ class HermesRuntimeImpl final : public HermesRuntime,
bool hasProperty(const jsi::Object &, const jsi::PropNameID &name) override;
bool hasProperty(const jsi::Object &, const jsi::String &name) override;
void setPropertyValue(
jsi::Object &,
const jsi::Object &,
const jsi::PropNameID &name,
const jsi::Value &value) override;
void setPropertyValue(
jsi::Object &,
const jsi::Object &,
const jsi::String &name,
const jsi::Value &value) override;
bool isArray(const jsi::Object &) const override;
Expand All @@ -660,7 +661,7 @@ class HermesRuntimeImpl final : public HermesRuntime,
jsi::Array getPropertyNames(const jsi::Object &) override;

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 @@ -669,8 +670,10 @@ class HermesRuntimeImpl final : public HermesRuntime,
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 @@ -1916,7 +1919,7 @@ bool HermesRuntimeImpl::hasProperty(
}

void HermesRuntimeImpl::setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::String &name,
const jsi::Value &value) {
vm::GCScope gcScope(runtime_);
Expand All @@ -1931,7 +1934,7 @@ void HermesRuntimeImpl::setPropertyValue(
}

void HermesRuntimeImpl::setPropertyValue(
jsi::Object &obj,
const jsi::Object &obj,
const jsi::PropNameID &name,
const jsi::Value &value) {
vm::GCScope gcScope(runtime_);
Expand Down Expand Up @@ -2000,8 +2003,8 @@ jsi::WeakObject HermesRuntimeImpl::createWeakObject(const jsi::Object &obj) {
static_cast<vm::JSObject *>(phv(obj).getObject()), runtime_));
}

jsi::Value HermesRuntimeImpl::lockWeakObject(jsi::WeakObject &wo) {
vm::WeakRoot<vm::JSObject> &wr = weakRoot(wo);
jsi::Value HermesRuntimeImpl::lockWeakObject(const jsi::WeakObject &wo) {
const vm::WeakRoot<vm::JSObject> &wr = weakRoot(wo);

if (const auto ptr = wr.get(runtime_, runtime_.getHeap()))
return add<jsi::Object>(vm::HermesValue::encodeObjectValue(ptr));
Expand Down Expand Up @@ -2070,7 +2073,7 @@ jsi::Value HermesRuntimeImpl::getValueAtIndex(const jsi::Array &arr, size_t i) {
}

void HermesRuntimeImpl::setValueAtIndexImpl(
jsi::Array &arr,
const jsi::Array &arr,
size_t i,
const jsi::Value &value) {
vm::GCScope gcScope(runtime_);
Expand Down
26 changes: 16 additions & 10 deletions API/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 API/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 API/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 b816665

Please sign in to comment.