From 9a2e548fc3573bfedf581d0c2a9b532fe265d8d9 Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 27 May 2023 22:36:17 -0700 Subject: [PATCH 1/5] remove JsFunction type parameter --- crates/neon/src/types_impl/mod.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/neon/src/types_impl/mod.rs b/crates/neon/src/types_impl/mod.rs index 90799c273..1b318c52b 100644 --- a/crates/neon/src/types_impl/mod.rs +++ b/crates/neon/src/types_impl/mod.rs @@ -16,7 +16,6 @@ pub(crate) mod utf8; use std::{ fmt::{self, Debug}, - marker::PhantomData, os::raw::c_void, }; @@ -1040,12 +1039,11 @@ impl Object for JsArray {} /// # Ok(f) /// # } /// ``` -pub struct JsFunction { +pub struct JsFunction { raw: raw::Local, - marker: PhantomData, } -impl Object for JsFunction {} +impl Object for JsFunction {} // Maximum number of function arguments in V8. const V8_ARGC_LIMIT: usize = 65535; @@ -1123,7 +1121,6 @@ impl JsFunction { if let Ok(raw) = sys::fun::new(cx.env().to_raw(), name, f) { Ok(Handle::new_internal(JsFunction { raw, - marker: PhantomData, })) } else { Err(Throw::new()) @@ -1132,7 +1129,7 @@ impl JsFunction { } } -impl JsFunction { +impl JsFunction { /// Calls this function. /// /// **See also:** [`JsFunction::call_with`]. @@ -1173,7 +1170,7 @@ impl JsFunction { /// Calls this function as a constructor. /// /// **See also:** [`JsFunction::construct_with`]. - pub fn construct<'a, 'b, C: Context<'a>, AS>(&self, cx: &mut C, args: AS) -> JsResult<'a, CL> + pub fn construct<'a, 'b, C: Context<'a>, AS>(&self, cx: &mut C, args: AS) -> JsResult<'a, JsObject> where AS: AsRef<[Handle<'b, JsValue>]>, { @@ -1216,15 +1213,14 @@ impl JsFunction { /// The caller must wrap in a `Handle` with an appropriate lifetime. unsafe fn clone(&self) -> Self { Self { - marker: PhantomData, raw: self.raw, } } } -impl Value for JsFunction {} +impl Value for JsFunction {} -unsafe impl TransparentNoCopyWrapper for JsFunction { +unsafe impl TransparentNoCopyWrapper for JsFunction { type Inner = raw::Local; fn into_inner(self) -> Self::Inner { @@ -1232,7 +1228,7 @@ unsafe impl TransparentNoCopyWrapper for JsFunction { } } -impl ValueInternal for JsFunction { +impl ValueInternal for JsFunction { fn name() -> String { "function".to_string() } @@ -1248,7 +1244,6 @@ impl ValueInternal for JsFunction { unsafe fn from_local(_env: Env, h: raw::Local) -> Self { JsFunction { raw: h, - marker: PhantomData, } } } From b3768267693469d78efc972abf7fd759720651f4 Mon Sep 17 00:00:00 2001 From: David Herman Date: Sun, 28 May 2023 09:37:23 -0700 Subject: [PATCH 2/5] add unit tests to demonstrate safety with non-objects and exotic objects --- test/napi/lib/functions.js | 22 ++++++++++++++++++++++ test/napi/src/js/functions.rs | 20 ++++++++++++++++++++ test/napi/src/lib.rs | 1 + 3 files changed, 43 insertions(+) diff --git a/test/napi/lib/functions.js b/test/napi/lib/functions.js index 2f1ce976b..a57b75cb8 100644 --- a/test/napi/lib/functions.js +++ b/test/napi/lib/functions.js @@ -222,6 +222,28 @@ describe("JsFunction", function () { assert.strictEqual(addon.get_number_or_default(), 0); }); + it("always provides an object for the this-binding", function() { + var meta1 = addon.assume_this_is_an_object.call(null); + assert.strictEqual(meta1.prototype, Object.getPrototypeOf(global)); + assert.strictEqual(meta1.hasOwn, false); + assert.strictEqual(meta1.property, Object.getPrototypeOf(global).toString); + + var meta2 = addon.assume_this_is_an_object.call(42); + assert.strictEqual(meta2.prototype, Number.prototype); + assert.strictEqual(meta2.hasOwn, false); + assert.strictEqual(meta2.property, Number.prototype.toString); + + var meta3 = addon.assume_this_is_an_object.call(Object.create(null)); + assert.strictEqual(meta3.prototype, null); + assert.strictEqual(meta3.hasOwn, false); + assert.strictEqual(meta3.property, undefined); + + var meta4 = addon.assume_this_is_an_object.call({toString: 17}); + assert.strictEqual(meta4.prototype, Object.prototype); + assert.strictEqual(meta4.hasOwn, true); + assert.strictEqual(meta4.property, 17); + }); + it("distinguishes calls from constructs", function () { assert.equal(addon.is_construct.call({}).wasConstructed, false); assert.equal(new addon.is_construct().wasConstructed, true); diff --git a/test/napi/src/js/functions.rs b/test/napi/src/js/functions.rs index 0a099774a..c5d919514 100644 --- a/test/napi/src/js/functions.rs +++ b/test/napi/src/js/functions.rs @@ -237,6 +237,26 @@ pub fn get_number_or_default(mut cx: FunctionContext) -> JsResult { Ok(cx.number(n)) } +pub fn assume_this_is_an_object(mut cx: FunctionContext) -> JsResult { + let this: Handle = cx.this()?; + let result: Handle = cx.empty_object(); + let global: Handle = cx.global(); + let object_class: Handle = global.get(&mut cx, "Object")?; + let get_prototype_of: Handle = object_class.get(&mut cx, "getPrototypeOf")?; + let object_prototype: Handle = object_class.get(&mut cx, "prototype")?; + let has_own_property: Handle = object_prototype.get(&mut cx, "hasOwnProperty")?; + let proto: Result, Handle> = cx.try_catch(|cx| { + get_prototype_of.call_with(cx).arg(this).apply(cx) + }); + let proto: Handle = proto.unwrap_or_else(|_| cx.undefined().upcast()); + let has_own: Handle = has_own_property.call_with(&cx).this(this).arg(cx.string("toString")).apply(&mut cx)?; + let prop: Handle = this.get(&mut cx, "toString")?; + result.set(&mut cx, "prototype", proto)?; + result.set(&mut cx, "hasOwn", has_own)?; + result.set(&mut cx, "property", prop)?; + Ok(result) +} + pub fn is_construct(mut cx: FunctionContext) -> JsResult { let this = cx.this::()?; let construct = matches!(cx.kind(), CallKind::Construct); diff --git a/test/napi/src/lib.rs b/test/napi/src/lib.rs index 90ccb7e7f..4bdbe1402 100644 --- a/test/napi/src/lib.rs +++ b/test/napi/src/lib.rs @@ -319,6 +319,7 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> { cx.export_function("throw_and_catch", throw_and_catch)?; cx.export_function("call_and_catch", call_and_catch)?; cx.export_function("get_number_or_default", get_number_or_default)?; + cx.export_function("assume_this_is_an_object", assume_this_is_an_object)?; cx.export_function("is_construct", is_construct)?; cx.export_function("caller_with_drop_callback", caller_with_drop_callback)?; From 929d6e51c546e133f22860b67d2feece611c5cb5 Mon Sep 17 00:00:00 2001 From: David Herman Date: Thu, 1 Jun 2023 19:57:29 -0700 Subject: [PATCH 3/5] rustfmt --- crates/neon/src/types_impl/mod.rs | 18 ++++++++---------- test/napi/src/js/functions.rs | 14 ++++++++------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/crates/neon/src/types_impl/mod.rs b/crates/neon/src/types_impl/mod.rs index 19ab71526..327df09d0 100644 --- a/crates/neon/src/types_impl/mod.rs +++ b/crates/neon/src/types_impl/mod.rs @@ -1116,9 +1116,7 @@ impl JsFunction { unsafe { if let Ok(raw) = sys::fun::new(cx.env().to_raw(), name, f) { - Ok(Handle::new_internal(JsFunction { - raw, - })) + Ok(Handle::new_internal(JsFunction { raw })) } else { Err(Throw::new()) } @@ -1167,7 +1165,11 @@ impl JsFunction { /// Calls this function as a constructor. /// /// **See also:** [`JsFunction::construct_with`]. - pub fn construct<'a, 'b, C: Context<'a>, AS>(&self, cx: &mut C, args: AS) -> JsResult<'a, JsObject> + pub fn construct<'a, 'b, C: Context<'a>, AS>( + &self, + cx: &mut C, + args: AS, + ) -> JsResult<'a, JsObject> where AS: AsRef<[Handle<'b, JsValue>]>, { @@ -1209,9 +1211,7 @@ impl JsFunction { /// # Safety /// The caller must wrap in a `Handle` with an appropriate lifetime. unsafe fn clone(&self) -> Self { - Self { - raw: self.raw, - } + Self { raw: self.raw } } } @@ -1239,9 +1239,7 @@ impl ValueInternal for JsFunction { } unsafe fn from_local(_env: Env, h: raw::Local) -> Self { - JsFunction { - raw: h, - } + JsFunction { raw: h } } } diff --git a/test/napi/src/js/functions.rs b/test/napi/src/js/functions.rs index 7634c3066..042d0de9b 100644 --- a/test/napi/src/js/functions.rs +++ b/test/napi/src/js/functions.rs @@ -238,16 +238,18 @@ pub fn get_number_or_default(mut cx: FunctionContext) -> JsResult { pub fn assume_this_is_an_object(mut cx: FunctionContext) -> JsResult { let this: Handle = cx.this()?; let result: Handle = cx.empty_object(); - let global: Handle = cx.global(); - let object_class: Handle = global.get(&mut cx, "Object")?; + let object_class: Handle = cx.global("Object")?; let get_prototype_of: Handle = object_class.get(&mut cx, "getPrototypeOf")?; let object_prototype: Handle = object_class.get(&mut cx, "prototype")?; let has_own_property: Handle = object_prototype.get(&mut cx, "hasOwnProperty")?; - let proto: Result, Handle> = cx.try_catch(|cx| { - get_prototype_of.call_with(cx).arg(this).apply(cx) - }); + let proto: Result, Handle> = + cx.try_catch(|cx| get_prototype_of.call_with(cx).arg(this).apply(cx)); let proto: Handle = proto.unwrap_or_else(|_| cx.undefined().upcast()); - let has_own: Handle = has_own_property.call_with(&cx).this(this).arg(cx.string("toString")).apply(&mut cx)?; + let has_own: Handle = has_own_property + .call_with(&cx) + .this(this) + .arg(cx.string("toString")) + .apply(&mut cx)?; let prop: Handle = this.get(&mut cx, "toString")?; result.set(&mut cx, "prototype", proto)?; result.set(&mut cx, "hasOwn", has_own)?; From 334c00026eeb82ac09135c31b18ae88a9b494b91 Mon Sep 17 00:00:00 2001 From: David Herman Date: Thu, 1 Jun 2023 20:01:38 -0700 Subject: [PATCH 4/5] clippy --- crates/neon/src/lifecycle.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/neon/src/lifecycle.rs b/crates/neon/src/lifecycle.rs index aacbe90e6..0a08ef922 100644 --- a/crates/neon/src/lifecycle.rs +++ b/crates/neon/src/lifecycle.rs @@ -74,7 +74,9 @@ pub(crate) struct LocalTable { pub(crate) type LocalCellValue = Box; +#[derive(Default)] pub(crate) enum LocalCell { + #[default] /// Uninitialized state. Uninit, /// Intermediate "dirty" state representing the middle of a `get_or_try_init` transaction. @@ -143,12 +145,6 @@ impl LocalCell { } } -impl Default for LocalCell { - fn default() -> Self { - LocalCell::Uninit - } -} - impl LocalTable { pub(crate) fn get(&mut self, index: usize) -> &mut LocalCell { if index >= self.cells.len() { From 9519abd1b6a6bf78c59800bb4f1cc893fe704c00 Mon Sep 17 00:00:00 2001 From: David Herman Date: Thu, 1 Jun 2023 20:08:43 -0700 Subject: [PATCH 5/5] prettier --- test/napi/lib/functions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/napi/lib/functions.js b/test/napi/lib/functions.js index a57b75cb8..bf5077c97 100644 --- a/test/napi/lib/functions.js +++ b/test/napi/lib/functions.js @@ -222,7 +222,7 @@ describe("JsFunction", function () { assert.strictEqual(addon.get_number_or_default(), 0); }); - it("always provides an object for the this-binding", function() { + it("always provides an object for the this-binding", function () { var meta1 = addon.assume_this_is_an_object.call(null); assert.strictEqual(meta1.prototype, Object.getPrototypeOf(global)); assert.strictEqual(meta1.hasOwn, false); @@ -238,7 +238,7 @@ describe("JsFunction", function () { assert.strictEqual(meta3.hasOwn, false); assert.strictEqual(meta3.property, undefined); - var meta4 = addon.assume_this_is_an_object.call({toString: 17}); + var meta4 = addon.assume_this_is_an_object.call({ toString: 17 }); assert.strictEqual(meta4.prototype, Object.prototype); assert.strictEqual(meta4.hasOwn, true); assert.strictEqual(meta4.property, 17);