From 3721a248570551b99dfb525f6cbb420521e68532 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Mon, 7 Oct 2024 16:56:26 -0400 Subject: [PATCH 1/2] fix(neon): Require `TryFromJs::Error` to implement `TryIntoJs` Additionally: * Replace `TryFromJs::from_js` implementations with a default * Add a `json::Error` wrapper to hide implementation details --- crates/neon/src/types_impl/extract/boxed.rs | 6 +- crates/neon/src/types_impl/extract/error.rs | 50 ++++++++++++++- crates/neon/src/types_impl/extract/json.rs | 57 +++++++++++++++-- crates/neon/src/types_impl/extract/mod.rs | 64 ++++++------------- .../src/types_impl/extract/try_from_js.rs | 40 +----------- test/napi/src/js/objects.rs | 4 +- 6 files changed, 123 insertions(+), 98 deletions(-) diff --git a/crates/neon/src/types_impl/extract/boxed.rs b/crates/neon/src/types_impl/extract/boxed.rs index 187b2a6bb..068a0f311 100644 --- a/crates/neon/src/types_impl/extract/boxed.rs +++ b/crates/neon/src/types_impl/extract/boxed.rs @@ -1,7 +1,7 @@ use crate::{ context::{Context, Cx}, handle::Handle, - result::{JsResult, NeonResult, ResultExt}, + result::{JsResult, NeonResult}, types::{ extract::{private, TryFromJs, TryIntoJs, TypeExpected}, Finalize, JsBox, JsValue, @@ -59,10 +59,6 @@ where Err(_) => Ok(Err(TypeExpected::new())), } } - - fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult { - Self::try_from_js(cx, v)?.or_throw(cx) - } } impl<'cx, T> TryIntoJs<'cx> for Boxed diff --git a/crates/neon/src/types_impl/extract/error.rs b/crates/neon/src/types_impl/extract/error.rs index 609692a8a..96ba4cd72 100644 --- a/crates/neon/src/types_impl/extract/error.rs +++ b/crates/neon/src/types_impl/extract/error.rs @@ -1,13 +1,59 @@ -use std::{error, fmt}; +use std::{convert::Infallible, error, fmt, marker::PhantomData}; use crate::{ context::{Context, Cx}, result::JsResult, - types::{extract::TryIntoJs, JsError}, + types::{ + extract::{private, TryIntoJs}, + JsError, JsValue, Value, + }, }; type BoxError = Box; +/// Error returned when a JavaScript value is not the type expected +pub struct TypeExpected(PhantomData); + +impl TypeExpected { + pub(super) fn new() -> Self { + Self(PhantomData) + } +} + +impl fmt::Display for TypeExpected { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "expected {}", T::name()) + } +} + +impl fmt::Debug for TypeExpected { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.debug_tuple("TypeExpected").field(&T::name()).finish() + } +} + +impl error::Error for TypeExpected {} + +impl<'cx, T: Value> TryIntoJs<'cx> for TypeExpected { + type Value = JsError; + + fn try_into_js(self, cx: &mut Cx<'cx>) -> JsResult<'cx, Self::Value> { + JsError::type_error(cx, self.to_string()) + } +} + +impl private::Sealed for TypeExpected {} + +impl<'cx> TryIntoJs<'cx> for Infallible { + type Value = JsValue; + + fn try_into_js(self, _: &mut Cx<'cx>) -> JsResult<'cx, Self::Value> { + unreachable!() + } +} + +impl private::Sealed for Infallible {} + #[derive(Debug)] /// Error that implements [`TryIntoJs`] and can produce specific error types. /// diff --git a/crates/neon/src/types_impl/extract/json.rs b/crates/neon/src/types_impl/extract/json.rs index ee3659e9b..00b7b9438 100644 --- a/crates/neon/src/types_impl/extract/json.rs +++ b/crates/neon/src/types_impl/extract/json.rs @@ -1,3 +1,21 @@ +//! Extract JavaScript values with JSON serialization +//! +//! For complex objects that implement [`serde::Serialize`] and [`serde::Deserialize`], +//! it is more ergonomic--and often faster--to extract with JSON serialization. The [`Json`] +//! extractor automatically calls `JSON.stringify` and `JSON.parse` as necessary. +//! +//! ``` +//! use neon::types::extract::Json; +//! +//! #[neon::export] +//! fn sort(Json(mut strings): Json>) -> Json> { +//! strings.sort(); +//! Json(strings) +//! } +//! ``` + +use std::{error, fmt}; + use crate::{ context::{Context, Cx}, handle::Handle, @@ -5,7 +23,7 @@ use crate::{ result::{JsResult, NeonResult}, types::{ extract::{private, TryFromJs, TryIntoJs}, - JsFunction, JsObject, JsString, JsValue, + JsError, JsFunction, JsObject, JsString, JsValue, }, }; @@ -73,17 +91,15 @@ impl<'cx, T> TryFromJs<'cx> for Json where for<'de> T: serde::de::Deserialize<'de>, { - type Error = serde_json::Error; + type Error = Error; fn try_from_js( cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>, ) -> NeonResult> { - Ok(serde_json::from_str(&stringify(cx, v)?).map(Json)) - } - - fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult { - Self::try_from_js(cx, v)?.or_else(|err| cx.throw_error(err.to_string())) + Ok(serde_json::from_str(&stringify(cx, v)?) + .map(Json) + .map_err(Error)) } } @@ -101,3 +117,30 @@ where } impl private::Sealed for Json {} + +/// Error returned when a value is invalid JSON +pub struct Error(serde_json::Error); + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Display::fmt(&self.0, f) + } +} + +impl fmt::Debug for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Debug::fmt(&self.0, f) + } +} + +impl error::Error for Error {} + +impl<'cx> TryIntoJs<'cx> for Error { + type Value = JsError; + + fn try_into_js(self, cx: &mut Cx<'cx>) -> JsResult<'cx, Self::Value> { + JsError::error(cx, self.to_string()) + } +} + +impl private::Sealed for Error {} diff --git a/crates/neon/src/types_impl/extract/mod.rs b/crates/neon/src/types_impl/extract/mod.rs index 83b6ae58e..6735a15dc 100644 --- a/crates/neon/src/types_impl/extract/mod.rs +++ b/crates/neon/src/types_impl/extract/mod.rs @@ -99,25 +99,29 @@ //! Note well, in this example, type annotations are not required on the tuple because //! Rust is able to infer it from the type arguments on `add` and `concat`. -use std::{fmt, marker::PhantomData}; - use crate::{ context::{Context, Cx, FunctionContext}, handle::Handle, - result::{JsResult, NeonResult, ResultExt}, + result::{JsResult, NeonResult}, types::{JsValue, Value}, }; -pub use self::{boxed::Boxed, error::Error, with::With}; +pub use self::{ + boxed::Boxed, + error::{Error, TypeExpected}, + with::With, +}; #[cfg(feature = "serde")] #[cfg_attr(docsrs, doc(cfg(feature = "serde")))] pub use self::json::Json; +#[cfg(feature = "serde")] +#[cfg_attr(docsrs, doc(cfg(feature = "serde")))] +pub mod json; + mod boxed; mod error; -#[cfg(feature = "serde")] -mod json; mod private; mod try_from_js; mod try_into_js; @@ -128,10 +132,7 @@ pub trait TryFromJs<'cx> where Self: private::Sealed + Sized, { - /// Error indicating non-JavaScript exception failure when extracting - // Consider adding a trait bound prior to unsealing `TryFromjs` - // https://github.com/neon-bindings/neon/issues/1026 - type Error; + type Error: TryIntoJs<'cx>; /// Extract this Rust type from a JavaScript value fn try_from_js( @@ -140,7 +141,16 @@ where ) -> NeonResult>; /// Same as [`TryFromJs`], but all errors are converted to JavaScript exceptions - fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult; + fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult { + match Self::try_from_js(cx, v)? { + Ok(v) => Ok(v), + Err(err) => { + let err = err.try_into_js(cx)?; + + cx.throw(err) + } + } + } } /// Convert Rust data into a JavaScript value @@ -166,38 +176,6 @@ pub struct ArrayBuffer(pub Vec); /// Wrapper for converting between [`Vec`] and [`JsBuffer`](super::JsBuffer) pub struct Buffer(pub Vec); -/// Error returned when a JavaScript value is not the type expected -pub struct TypeExpected(PhantomData); - -impl TypeExpected { - fn new() -> Self { - Self(PhantomData) - } -} - -impl fmt::Display for TypeExpected { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "expected {}", T::name()) - } -} - -impl fmt::Debug for TypeExpected { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_tuple("TypeExpected").field(&T::name()).finish() - } -} - -impl std::error::Error for TypeExpected {} - -impl ResultExt for Result> { - fn or_throw<'a, C: Context<'a>>(self, cx: &mut C) -> NeonResult { - match self { - Ok(v) => Ok(v), - Err(_) => cx.throw_type_error(format!("expected {}", U::name())), - } - } -} - /// Trait specifying values that may be extracted from function arguments. /// /// **Note:** This trait is implemented for tuples of up to 32 values, but for diff --git a/crates/neon/src/types_impl/extract/try_from_js.rs b/crates/neon/src/types_impl/extract/try_from_js.rs index 90724794d..c16beb49a 100644 --- a/crates/neon/src/types_impl/extract/try_from_js.rs +++ b/crates/neon/src/types_impl/extract/try_from_js.rs @@ -9,7 +9,7 @@ use crate::{ context::{internal::ContextInternal, Cx}, handle::{Handle, Root}, object::Object, - result::{NeonResult, ResultExt, Throw}, + result::{NeonResult, Throw}, sys, types::{ buffer::{Binary, TypedArray}, @@ -22,14 +22,6 @@ use crate::{ #[cfg(feature = "napi-5")] use crate::types::JsDate; -macro_rules! from_js { - () => { - fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult { - Self::try_from_js(cx, v)?.or_throw(cx) - } - }; -} - impl<'cx, V> TryFromJs<'cx> for Handle<'cx, V> where V: Value, @@ -42,8 +34,6 @@ where ) -> NeonResult> { Ok(v.downcast(cx).map_err(|_| TypeExpected::new())) } - - from_js!(); } impl<'cx, O> TryFromJs<'cx> for Root @@ -61,8 +51,6 @@ where Err(_) => Err(TypeExpected::new()), }) } - - from_js!(); } impl<'cx, T> TryFromJs<'cx> for Option @@ -81,14 +69,6 @@ where T::try_from_js(cx, v).map(|v| v.map(Some)) } - - fn from_js(cx: &mut Cx<'cx>, v: Handle<'cx, JsValue>) -> NeonResult { - if is_null_or_undefined(cx, v)? { - return Ok(None); - } - - T::from_js(cx, v).map(Some) - } } impl<'cx> TryFromJs<'cx> for f64 { @@ -110,8 +90,6 @@ impl<'cx> TryFromJs<'cx> for f64 { Ok(Ok(n)) } - - from_js!(); } impl<'cx> TryFromJs<'cx> for bool { @@ -133,8 +111,6 @@ impl<'cx> TryFromJs<'cx> for bool { Ok(Ok(b)) } - - from_js!(); } impl<'cx> TryFromJs<'cx> for String { @@ -178,8 +154,6 @@ impl<'cx> TryFromJs<'cx> for String { Ok(Ok(String::from_utf8_unchecked(buf))) } } - - from_js!(); } #[cfg_attr(docsrs, doc(cfg(feature = "napi-5")))] @@ -203,8 +177,6 @@ impl<'cx> TryFromJs<'cx> for Date { Ok(Ok(Date(d))) } - - from_js!(); } // This implementation primarily exists for macro authors. It is infallible, rather @@ -226,10 +198,6 @@ impl<'cx> TryFromJs<'cx> for () { ) -> NeonResult> { Ok(Ok(())) } - - fn from_js(_cx: &mut Cx<'cx>, _v: Handle<'cx, JsValue>) -> NeonResult { - Ok(()) - } } impl<'cx, T> TryFromJs<'cx> for Vec @@ -250,8 +218,6 @@ where Ok(Ok(v.as_slice(cx).to_vec())) } - - from_js!(); } impl<'cx> TryFromJs<'cx> for Buffer { @@ -268,8 +234,6 @@ impl<'cx> TryFromJs<'cx> for Buffer { Ok(Ok(Buffer(v.as_slice(cx).to_vec()))) } - - from_js!(); } impl<'cx> TryFromJs<'cx> for ArrayBuffer { @@ -286,8 +250,6 @@ impl<'cx> TryFromJs<'cx> for ArrayBuffer { Ok(Ok(ArrayBuffer(v.as_slice(cx).to_vec()))) } - - from_js!(); } fn is_null_or_undefined(cx: &mut Cx, v: Handle) -> NeonResult diff --git a/test/napi/src/js/objects.rs b/test/napi/src/js/objects.rs index 32b1f1815..1f7d2d980 100644 --- a/test/napi/src/js/objects.rs +++ b/test/napi/src/js/objects.rs @@ -113,12 +113,12 @@ pub fn call_methods_with_prop(mut cx: FunctionContext) -> JsResult { obj.prop(&mut cx, "setName") .bind()? .arg("Wonder Woman")? - .call()?; + .exec()?; obj.prop(&mut cx, "toString").bind()?.call() } pub fn call_non_method_with_prop(mut cx: FunctionContext) -> JsResult { let obj: Handle = cx.argument::(0)?; - obj.prop(&mut cx, "number").bind()?.call()?; + obj.prop(&mut cx, "number").bind()?.exec()?; Ok(cx.undefined()) } From afac8e11108c5076f2d911c23d678964f55ab0a3 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Mon, 7 Oct 2024 17:11:30 -0400 Subject: [PATCH 2/2] fix(test-ui): Only run UI tests on stable --- Cargo.lock | 7 +++++++ test/ui/Cargo.toml | 3 +++ test/ui/src/lib.rs | 1 + 3 files changed, 11 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 8a31c5086..8df7b86df 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -680,6 +680,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rustversion" +version = "1.0.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "955d28af4278de8121b7ebeb796b6a45735dc01436d898801014aced2773a3d6" + [[package]] name = "ryu" version = "1.0.17" @@ -871,6 +877,7 @@ name = "ui-test" version = "0.1.0" dependencies = [ "neon", + "rustversion", "trybuild", ] diff --git a/test/ui/Cargo.toml b/test/ui/Cargo.toml index 83b074886..15acd124e 100644 --- a/test/ui/Cargo.toml +++ b/test/ui/Cargo.toml @@ -13,5 +13,8 @@ version = "1.0.0" path = "../../crates/neon" features = ["futures", "napi-experimental", "serde"] +[dependencies] +rustversion = "1.0.17" + [dev-dependencies] trybuild = "1" diff --git a/test/ui/src/lib.rs b/test/ui/src/lib.rs index f120f3f48..5137b6a0d 100644 --- a/test/ui/src/lib.rs +++ b/test/ui/src/lib.rs @@ -1,3 +1,4 @@ +#[rustversion::attr(not(stable), ignore)] #[test] fn ui() { let t = trybuild::TestCases::new();