From c1b1ee0fbbbe535692060baea4c24634d6be052b Mon Sep 17 00:00:00 2001 From: Emmanuel Leblond Date: Fri, 11 Oct 2024 23:18:59 +0200 Subject: [PATCH] Address @kjvalencik comments --- crates/neon/src/sys/tag.rs | 36 ------ crates/neon/src/types_impl/map.rs | 175 ++++++++++++++++-------------- crates/neon/src/types_impl/mod.rs | 11 +- test/napi/lib/types.js | 11 ++ test/napi/src/js/map.rs | 69 +++++++----- test/napi/src/js/types.rs | 8 ++ test/napi/src/lib.rs | 22 +--- 7 files changed, 163 insertions(+), 169 deletions(-) diff --git a/crates/neon/src/sys/tag.rs b/crates/neon/src/sys/tag.rs index 6d5e1853c..91b2266a6 100644 --- a/crates/neon/src/sys/tag.rs +++ b/crates/neon/src/sys/tag.rs @@ -1,5 +1,3 @@ -use crate::sys; - use super::{ bindings as napi, raw::{Env, Local}, @@ -139,37 +137,3 @@ pub unsafe fn check_object_type_tag(env: Env, object: Local, tag: &super::TypeTa pub unsafe fn is_bigint(env: Env, val: Local) -> bool { is_type(env, val, napi::ValueType::BigInt) } - -pub unsafe fn is_map(env: Env, val: Local) -> bool { - let global_object = unsafe { - let mut out: super::raw::Local = std::mem::zeroed(); - super::scope::get_global(env, &mut out); - out - }; - - let map_literal = { - let mut out: super::raw::Local = std::mem::zeroed(); - let literal = b"Map"; - assert_eq!( - super::string::new(&mut out, env, literal.as_ptr(), literal.len() as _), - true - ); - out - }; - - let map_constructor = { - let mut out: super::raw::Local = std::mem::zeroed(); - assert_eq!( - sys::object::get(&mut out, env, global_object, map_literal), - true - ); - out - }; - - let mut result = false; - assert_eq!( - napi::instanceof(env, val, map_constructor, &mut result), - napi::Status::Ok - ); - result -} diff --git a/crates/neon/src/types_impl/map.rs b/crates/neon/src/types_impl/map.rs index 0c0c990e4..554463672 100644 --- a/crates/neon/src/types_impl/map.rs +++ b/crates/neon/src/types_impl/map.rs @@ -1,16 +1,8 @@ -use std::{error, fmt, mem::MaybeUninit}; - use crate::{ - context::{internal::Env, Context}, - handle::{internal::TransparentNoCopyWrapper, root::NapiRef, Handle}, - macro_internal::NeonResultTag, - object::Object, - result::{JsResult, NeonResult, ResultExt}, - sys::{self, raw}, - types::{private, JsFunction, JsObject, Value}, + context::{internal::Env, Context, Cx}, handle::{internal::TransparentNoCopyWrapper, Handle, Root}, object::Object, result::{JsResult, NeonResult}, sys::raw, thread::LocalKey, types::{private, JsFunction, JsObject, Value} }; -use super::{JsBoolean, JsNumber, JsUndefined}; +use super::extract::{TryFromJs, TryIntoJs}; #[derive(Debug)] #[repr(transparent)] @@ -20,10 +12,7 @@ use super::{JsBoolean, JsNumber, JsUndefined}; pub struct JsMap(raw::Local); impl JsMap { - pub fn new<'cx, C>(cx: &mut C) -> NeonResult> - where - C: Context<'cx>, - { + pub fn new<'cx>(cx: &mut Cx<'cx>) -> NeonResult> { let map = cx .global::("Map")? .construct_with(cx) @@ -32,104 +21,107 @@ impl JsMap { Ok(map.downcast_or_throw(cx)?) } - pub fn size<'a, C: Context<'a>>(&self, cx: &mut C) -> NeonResult> { - Object::get(self, cx, "size") + pub fn size(&self, cx: &mut Cx) -> NeonResult { + self.prop(cx, "size").get() } - // TODO: is the return type important here ? - // I see three possibilities here: - // 1. Stick to the JS and return the `undefined` (this is what we do now) - // 2. Check we get an `undefined` and return `Ok(())` - // 3. Just discard the return value and return `Ok(())` - // Solutions 2 & 3 are more user-friendly, but make more assumptions (though it - // should be fine given `Map` is not expected to be overridden ?). - pub fn clear<'a, C: Context<'a>>(&self, cx: &mut C) -> NeonResult> { - Object::call_method_with(self, cx, "clear")?.apply(cx) + pub fn clear(&self, cx: &mut Cx) -> NeonResult<()> { + self.method(cx, "clear")?.call() } - pub fn delete<'a, C: Context<'a>, K: Value>( + pub fn delete<'cx, K>( &self, - cx: &mut C, - key: Handle<'a, K>, - ) -> NeonResult { - Object::call_method_with(self, cx, "delete")? - .arg(key) - .apply::(cx) - .map(|v| v.value(cx)) + cx: &mut Cx<'cx>, + key: K, + ) -> NeonResult + where K: TryIntoJs<'cx> { + self.method(cx, "delete")?.arg(key)?.call() } - pub fn entries<'a, C: Context<'a>>(&self, cx: &mut C) -> NeonResult> { - Object::call_method_with(self, cx, "entries")?.apply(cx) + pub fn entries<'cx, R>(&self, cx: &mut Cx<'cx>) -> NeonResult + where R: TryFromJs<'cx> + { + self.method(cx, "entries")?.call() } - pub fn for_each<'a, C: Context<'a>, F: Value>( + pub fn for_each<'cx, F, R>( &self, - cx: &mut C, - cb: Handle<'a, F>, - ) -> NeonResult> { - Object::call_method_with(self, cx, "forEach")? - .arg(cb) - .apply(cx) + cx: &mut Cx<'cx>, + cb: F, + ) -> NeonResult + where F: TryIntoJs<'cx>, R: TryFromJs<'cx> + { + self.method(cx, "forEach")?.arg(cb)?.call() } - pub fn get<'a, C: Context<'a>, K: Value, R: Value>( + pub fn get<'cx, K, R>( &self, - cx: &mut C, - key: Handle<'a, K>, - ) -> NeonResult> { - Object::call_method_with(self, cx, "get")? - .arg(key) - .apply(cx) + cx: &mut Cx<'cx>, + key: K, + ) -> NeonResult + where + K: TryIntoJs<'cx>, + R: TryFromJs<'cx> + { + self.method(cx, "get")?.arg(key)?.call() } - pub fn has<'a, C: Context<'a>, K: Value>( + pub fn has<'cx, K>( &self, - cx: &mut C, - key: Handle<'a, K>, - ) -> NeonResult { - Object::call_method_with(self, cx, "has")? - .arg(key) - .apply::(cx) - .map(|v| v.value(cx)) + cx: &mut Cx<'cx>, + key: K, + ) -> NeonResult + where + K: TryIntoJs<'cx>, + { + self.method(cx, "has")?.arg(key)?.call() } - pub fn keys<'a, C: Context<'a>, R: Value>(&self, cx: &mut C) -> NeonResult> { - Object::call_method_with(self, cx, "keys")?.apply(cx) + pub fn keys<'cx, R>(&self, cx: &mut Cx<'cx>) -> NeonResult + where + R: TryFromJs<'cx> + { + self.method(cx, "keys")?.call() } - pub fn set<'a, C: Context<'a>, K: Value, V: Value>( + pub fn set<'cx, K, V>( &self, - cx: &mut C, - key: Handle<'a, K>, - value: Handle<'a, V>, - ) -> NeonResult> { - Object::call_method_with(self, cx, "set")? - .arg(key) - .arg(value) - .apply(cx) + cx: &mut Cx<'cx>, + key: K, + value: V, + ) -> NeonResult> + where + K: TryIntoJs<'cx>, + V: TryIntoJs<'cx> + { + self.method(cx, "set")?.arg(key)?.arg(value)?.call() } - pub fn values<'a, C: Context<'a>, R: Value>(&self, cx: &mut C) -> NeonResult> { - Object::call_method_with(self, cx, "values")?.apply(cx) + pub fn values<'cx, R>(&self, cx: &mut Cx<'cx>) -> NeonResult + where + R: TryFromJs<'cx> + { + self.method(cx, "values")?.call() } - pub fn group_by<'a, C: Context<'a>, A: Value, B: Value, R: Value>( - cx: &mut C, - elements: Handle<'a, A>, - cb: Handle<'a, B>, - ) -> NeonResult> { + pub fn group_by<'cx, A, B, R>( + cx: &mut Cx<'cx>, + elements: A, + cb: B, + ) -> NeonResult + where + A: TryIntoJs<'cx>, + B: TryIntoJs<'cx>, + R: TryFromJs<'cx> + { // TODO: This is broken and leads to a `failed to downcast any to object` error // when trying to downcast `Map.groupBy` into a `JsFunction`... cx.global::("Map")? - .call_method_with(cx, "groupBy")? - .arg(elements) - .arg(cb) - .apply(cx) + .method(cx, "groupBy")? + .arg(elements)? + .arg(cb)? + .call() } - - // TODO: should we implementd those as well ? - // Map[Symbol.species] - // Map.prototype[Symbol.iterator]() } unsafe impl TransparentNoCopyWrapper for JsMap { @@ -146,7 +138,10 @@ impl private::ValueInternal for JsMap { } fn is_typeof(env: Env, other: &Other) -> bool { - unsafe { sys::tag::is_map(env.to_raw(), other.to_local()) } + Cx::with_context(env, |mut cx| { + let ctor = map_constructor(&mut cx).unwrap(); + other.instance_of(&mut cx, &*ctor) + }) } fn to_local(&self) -> raw::Local { @@ -161,3 +156,15 @@ impl private::ValueInternal for JsMap { impl Value for JsMap {} impl Object for JsMap {} + +fn global_map_constructor<'cx>(cx: &mut Cx<'cx>) -> JsResult<'cx, JsFunction> { + cx.global::("Map") +} + +fn map_constructor<'cx>(cx: &mut Cx<'cx>) -> JsResult<'cx, JsFunction> { + static MAP_CONSTRUCTOR: LocalKey> = LocalKey::new(); + + MAP_CONSTRUCTOR + .get_or_try_init(cx, |cx| global_map_constructor(cx).map(|f| f.root(cx))) + .map(|f| f.to_inner(cx)) +} diff --git a/crates/neon/src/types_impl/mod.rs b/crates/neon/src/types_impl/mod.rs index 3d60bd163..16680cfae 100644 --- a/crates/neon/src/types_impl/mod.rs +++ b/crates/neon/src/types_impl/mod.rs @@ -25,7 +25,7 @@ use private::prepare_call; use smallvec::smallvec; use crate::{ - context::{internal::Env, Context, Cx, FunctionContext}, + context::{internal::{ContextInternal, Env}, Context, Cx, FunctionContext}, handle::{ internal::{SuperType, TransparentNoCopyWrapper}, Handle, @@ -100,6 +100,15 @@ pub trait Value: ValueInternal { JsValue::new_internal(self.to_local()) } + fn instance_of(&self, cx: &mut Cx, ctor: &V) -> bool { + let mut result = false; + assert_eq!( + unsafe { sys::bindings::instanceof(cx.env().to_raw(), self.to_local(), ctor.to_local(), &mut result) }, + sys::bindings::Status::Ok + ); + result + } + #[cfg(feature = "sys")] #[cfg_attr(docsrs, doc(cfg(feature = "sys")))] /// Get a raw reference to the wrapped Node-API value. diff --git a/test/napi/lib/types.js b/test/napi/lib/types.js index a4f32d5c9..4beff33d6 100644 --- a/test/napi/lib/types.js +++ b/test/napi/lib/types.js @@ -90,4 +90,15 @@ describe("type checks", function () { assert(!addon.strict_equals(o1, o2)); assert(!addon.strict_equals(o1, 17)); }); + + it("instance_of", function () { + assert(addon.instance_of(new Error(), Error)); + assert(!addon.instance_of(new Error(), Map)); + assert(addon.instance_of(new Map(), Map)); + + function Car() {} + function Bike() {} + assert(addon.instance_of(new Car(), Car)); + assert(!addon.instance_of(new Car(), Bike)); + }); }); diff --git a/test/napi/src/js/map.rs b/test/napi/src/js/map.rs index 7f5881bfe..97f0ca655 100644 --- a/test/napi/src/js/map.rs +++ b/test/napi/src/js/map.rs @@ -1,84 +1,97 @@ use neon::prelude::*; -pub fn return_js_map(mut cx: FunctionContext) -> JsResult { - JsMap::new(&mut cx) +#[neon::export] +pub fn return_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsMap> { + JsMap::new(cx) } -pub fn return_js_map_with_number_as_keys_and_values(mut cx: FunctionContext) -> JsResult { - let map = JsMap::new(&mut cx)?; +#[neon::export] +pub fn return_js_map_with_number_as_keys_and_values<'cx>( + cx: &mut FunctionContext<'cx>, +) -> JsResult<'cx, JsMap> { + let map = JsMap::new(cx)?; { let key = cx.number(1); let val = cx.number(1000); - map.set(&mut cx, key, val)?; + map.set(cx, key, val)?; } { let key = cx.number(-1); let val = cx.number(-1000); - map.set(&mut cx, key, val)?; + map.set(cx, key, val)?; } Ok(map) } -pub fn return_js_map_with_heterogeneous_keys_and_values( - mut cx: FunctionContext, -) -> JsResult { - let map = JsMap::new(&mut cx)?; +#[neon::export] +pub fn return_js_map_with_heterogeneous_keys_and_values<'cx>( + cx: &mut FunctionContext<'cx>, +) -> JsResult<'cx, JsMap> { + let map = JsMap::new(cx)?; { let key = cx.string("a"); let val = cx.number(1); - map.set(&mut cx, key, val)?; + map.set(cx, key, val)?; } { let key = cx.number(26); let val = cx.string("z"); - map.set(&mut cx, key, val)?; + map.set(cx, key, val)?; } Ok(map) } -pub fn read_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn read_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsValue> { let map = cx.argument::(0)?; let key = cx.argument::(1)?; - map.get(&mut cx, key) + map.get(cx, key) } -pub fn get_js_map_size(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn get_js_map_size<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsNumber> { let map = cx.argument::(0)?; - map.size(&mut cx) + map.size(cx).map(|x| cx.number(x)) } -pub fn modify_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn modify_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsMap> { let map = cx.argument::(0)?; let key = cx.argument::(1)?; let value = cx.argument::(2)?; - map.set(&mut cx, key, value) + map.set(cx, key, value) } -pub fn clear_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn clear_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsUndefined> { let map = cx.argument::(0)?; - map.clear(&mut cx) + map.clear(cx).map(|_| cx.undefined()) } -pub fn delete_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn delete_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsBoolean> { let map = cx.argument::(0)?; let key = cx.argument::(1)?; - map.delete(&mut cx, key).map(|v| cx.boolean(v)) + map.delete(cx, key).map(|v| cx.boolean(v)) } -pub fn has_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn has_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsBoolean> { let map = cx.argument::(0)?; let key = cx.argument::(1)?; - map.has(&mut cx, key).map(|v| cx.boolean(v)) + map.has(cx, key).map(|v| cx.boolean(v)) } -pub fn for_each_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn for_each_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsUndefined> { let map = cx.argument::(0)?; let cb: Handle<'_, JsValue> = cx.argument::(1)?; - map.for_each(&mut cx, cb) + map.for_each(cx, cb) } -pub fn group_by_js_map(mut cx: FunctionContext) -> JsResult { +#[neon::export] +pub fn group_by_js_map<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsMap> { let elements = cx.argument::(0)?; let cb = cx.argument::(1)?; - JsMap::group_by(&mut cx, elements, cb) + JsMap::group_by(cx, elements, cb) } diff --git a/test/napi/src/js/types.rs b/test/napi/src/js/types.rs index 6115a1f72..f2860a7cc 100644 --- a/test/napi/src/js/types.rs +++ b/test/napi/src/js/types.rs @@ -72,3 +72,11 @@ pub fn strict_equals(mut cx: FunctionContext) -> JsResult { let eq = v1.strict_equals(&mut cx, v2); Ok(cx.boolean(eq)) } + +#[neon::export] +pub fn instance_of<'cx>(cx: &mut FunctionContext<'cx>) -> JsResult<'cx, JsBoolean> { + let val: Handle = cx.argument(0)?; + let ctor: Handle = cx.argument(1)?; + let result = val.instance_of(cx, &*ctor); + Ok(cx.boolean(result)) +} diff --git a/test/napi/src/lib.rs b/test/napi/src/lib.rs index d546586e2..1ce6b0937 100644 --- a/test/napi/src/lib.rs +++ b/test/napi/src/lib.rs @@ -3,8 +3,8 @@ use once_cell::sync::OnceCell; use tokio::runtime::Runtime; use crate::js::{ - arrays::*, boxed::*, coercions::*, date::*, errors::*, functions::*, map::*, numbers::*, - objects::*, strings::*, threads::*, typedarrays::*, types::*, + arrays::*, boxed::*, coercions::*, date::*, errors::*, functions::*, numbers::*, objects::*, + strings::*, threads::*, typedarrays::*, types::*, }; mod js { @@ -234,24 +234,6 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> { cx.export_function("return_js_array_with_string", return_js_array_with_string)?; cx.export_function("read_js_array", read_js_array)?; - cx.export_function("return_js_map", return_js_map)?; - cx.export_function( - "return_js_map_with_number_as_keys_and_values", - return_js_map_with_number_as_keys_and_values, - )?; - cx.export_function( - "return_js_map_with_heterogeneous_keys_and_values", - return_js_map_with_heterogeneous_keys_and_values, - )?; - cx.export_function("read_js_map", read_js_map)?; - cx.export_function("get_js_map_size", get_js_map_size)?; - cx.export_function("modify_js_map", modify_js_map)?; - cx.export_function("clear_js_map", clear_js_map)?; - cx.export_function("delete_js_map", delete_js_map)?; - cx.export_function("has_js_map", has_js_map)?; - cx.export_function("for_each_js_map", for_each_js_map)?; - cx.export_function("group_by_js_map", group_by_js_map)?; - cx.export_function("to_string", to_string)?; cx.export_function("return_js_global_object", return_js_global_object)?;