From fb6a48a35f4f9fd35fb87b5dab4f796eb87708ca Mon Sep 17 00:00:00 2001 From: Kai Tamkun Date: Thu, 31 Oct 2024 15:16:42 -0700 Subject: [PATCH] Fix property-related NAPI methods --- src/bun.js/bindings/napi.cpp | 58 +++++++++++++++++++++++++++++++----- src/napi/napi.zig | 37 ++--------------------- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index b46ca8ced8..d95e5defaa 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -561,6 +561,7 @@ static void defineNapiProperty(Zig::GlobalObject* globalObject, JSC::JSObject* t auto getterSetter = JSC::GetterSetter::create(vm, globalObject, getter, setter); to->putDirectAccessor(globalObject, propertyName, getterSetter, PropertyAttribute::Accessor | getPropertyAttributes(property)); + } else { JSValue value = toJS(property.value); @@ -568,7 +569,8 @@ static void defineNapiProperty(Zig::GlobalObject* globalObject, JSC::JSObject* t value = JSC::jsUndefined(); } - to->putDirect(vm, propertyName, value, getPropertyAttributes(property)); + PropertyDescriptor descriptor(value, getPropertyAttributes(property)); + to->methodTable()->defineOwnProperty(to, globalObject, propertyName, descriptor, false); } } @@ -595,7 +597,7 @@ extern "C" napi_status napi_set_property(napi_env env, napi_value target, JSValue jsValue = toJS(value); - bool putResult = object->put(object, globalObject, identifier, jsValue, slot); + bool putResult = object->methodTable()->put(object, globalObject, identifier, jsValue, slot); NAPI_RETURN_IF_EXCEPTION(env); if (!putResult) return napi_set_last_error(env, napi_generic_failure); @@ -603,6 +605,44 @@ extern "C" napi_status napi_set_property(napi_env env, napi_value target, NAPI_RETURN_SUCCESS(env); } +extern "C" napi_status napi_set_element(napi_env env, napi_value object_, + uint32_t index, napi_value value_) +{ + NAPI_PREAMBLE(env); + NAPI_CHECK_ARG(env, object_); + NAPI_CHECK_ARG(env, value_); + + JSValue object = toJS(object_); + JSValue value = toJS(value_); + NAPI_RETURN_EARLY_IF_FALSE(env, !object.isEmpty() && !value.isEmpty(), napi_invalid_arg); + + JSObject* jsObject = object.getObject(); + NAPI_RETURN_EARLY_IF_FALSE(env, jsObject, napi_array_expected); + + jsObject->methodTable()->putByIndex(jsObject, toJS(env), index, value, false); + NAPI_RETURN_IF_EXCEPTION(env); + NAPI_RETURN_SUCCESS(env); +} + +extern "C" napi_status napi_has_element(napi_env env, napi_value object_, + uint32_t index, bool* result) +{ + NAPI_PREAMBLE(env); + NAPI_CHECK_ARG(env, object_); + NAPI_CHECK_ARG(env, result); + + JSValue object = toJS(object_); + NAPI_RETURN_EARLY_IF_FALSE(env, !object.isEmpty(), napi_invalid_arg); + + JSObject* jsObject = object.getObject(); + NAPI_RETURN_EARLY_IF_FALSE(env, jsObject, napi_array_expected); + + bool has_property = jsObject->hasProperty(toJS(env), index); + *result = has_property; + + NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION(env); +} + extern "C" napi_status napi_has_property(napi_env env, napi_value object, napi_value key, bool* result) { @@ -725,7 +765,7 @@ extern "C" napi_status napi_set_named_property(napi_env env, napi_value object, // TODO should maybe be false PutPropertySlot slot(target, false); - target->put(target, globalObject, identifier, jsValue, slot); + target->methodTable()->put(target, globalObject, identifier, jsValue, slot); NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION(env); } @@ -1077,7 +1117,7 @@ extern "C" napi_status napi_define_properties(napi_env env, napi_value object, size_t property_count, const napi_property_descriptor* properties) { - NAPI_PREAMBLE(env); + NAPI_PREAMBLE_NO_THROW_SCOPE(env); NAPI_CHECK_ARG(env, object); NAPI_RETURN_EARLY_IF_FALSE(env, properties || property_count == 0, napi_invalid_arg); @@ -1097,7 +1137,7 @@ napi_define_properties(napi_env env, napi_value object, size_t property_count, } throwScope.release(); - NAPI_RETURN_SUCCESS(env); + return napi_set_last_error(env, napi_ok); } static JSC::ErrorInstance* createErrorWithCode(JSC::JSGlobalObject* globalObject, const WTF::String& code, const WTF::String& message, JSC::ErrorType type) @@ -1821,13 +1861,17 @@ extern "C" napi_status napi_get_all_property_names( jsc_property_mode = PropertyNameMode::Strings; } else if (key_filter & napi_key_skip_strings) { jsc_property_mode = PropertyNameMode::Symbols; + } else { + jsc_key_mode = DontEnumPropertiesMode::Include; } auto globalObject = toJS(env); JSArray* exportKeys = ownPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode); - napi_key_filter filter_by_any_descriptor = static_cast(napi_key_enumerable | napi_key_writable | napi_key_configurable); + NAPI_RETURN_IF_EXCEPTION(env); + + auto filter_by_any_descriptor = static_cast(napi_key_enumerable | napi_key_writable | napi_key_configurable); // avoid expensive iteration if they don't care whether keys are enumerable, writable, or configurable if (key_filter & filter_by_any_descriptor) { JSArray* filteredKeys = JSArray::create(globalObject->vm(), globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous), 0); @@ -2288,7 +2332,7 @@ extern "C" napi_status napi_delete_element(napi_env env, napi_value objectValue, NAPI_RETURN_EARLY_IF_FALSE(env, jsObject, napi_object_expected); if (LIKELY(result)) { - *result = JSObject::deletePropertyByIndex(jsObject, toJS(env), index); + *result = jsObject->methodTable()->deletePropertyByIndex(jsObject, toJS(env), index); } NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION(env); } diff --git a/src/napi/napi.zig b/src/napi/napi.zig index 91a54dc34d..4b5aca6f14 100644 --- a/src/napi/napi.zig +++ b/src/napi/napi.zig @@ -549,41 +549,8 @@ pub export fn napi_get_prototype(env_: napi_env, object_: napi_value, result_: ? // result.* = // } -pub export fn napi_set_element(env_: napi_env, object_: napi_value, index: c_uint, value_: napi_value) napi_status { - log("napi_set_element", .{}); - const env = env_ orelse { - return envIsNull(); - }; - const object = object_.get(); - const value = value_.get(); - if (value.isEmpty() or object.isEmpty()) - return env.invalidArg(); - if (!object.jsType().isIndexable()) { - return env.setLastError(.array_expected); - } - JSC.C.JSObjectSetPropertyAtIndex(env.toJS().ref(), object.asObjectRef(), index, value.asObjectRef(), TODO_EXCEPTION); - return env.ok(); -} -pub export fn napi_has_element(env_: napi_env, object_: napi_value, index: c_uint, result_: ?*bool) napi_status { - log("napi_has_element", .{}); - const env = env_ orelse { - return envIsNull(); - }; - const result = result_ orelse { - return env.invalidArg(); - }; - const object = object_.get(); - if (object.isEmpty()) { - return env.invalidArg(); - } - - if (!object.jsType().isIndexable()) { - return env.setLastError(.array_expected); - } - - result.* = object.getLength(env.toJS()) > index; - return env.ok(); -} +pub extern fn napi_set_element(env_: napi_env, object_: napi_value, index: c_uint, value_: napi_value) napi_status; +pub extern fn napi_has_element(env_: napi_env, object_: napi_value, index: c_uint, result_: ?*bool) napi_status; pub extern fn napi_get_element(env: napi_env, object: napi_value, index: u32, result: *napi_value) napi_status; pub extern fn napi_delete_element(env: napi_env, object: napi_value, index: u32, result: *napi_value) napi_status; pub extern fn napi_define_properties(env: napi_env, object: napi_value, property_count: usize, properties: [*c]const napi_property_descriptor) napi_status;