From 19cb8a112e4414aa3eb0f4ac5a55f9cdcc2d62d0 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Mon, 6 May 2024 12:31:15 -0700 Subject: [PATCH 1/2] feat: Add `ToV8`/`FromV8` impls for bool and numeric types (#727) Adds two wrapper structs which implement two different modes of serializing/deserializing numeric types: ```rust pub struct Smi(pub T); // serializes like #[smi] (but without fast call support) #[op2] #[to_v8] fn op_returns_smi() -> Smi { Smi(1) } pub struct Number(pub T); // serializes like #[number] #[op2] #[to_v8] fn op_returns_number() -> Number { Number(2) } ``` Also adds an impl for `bool` itself, which is fairly uncontroversial I think. TODO: - [x] add tests --- core/convert.rs | 275 ++++++++++++++++++++++++++++++++++++++++++++ core/error.rs | 27 +++++ core/lib.rs | 6 +- core/runtime/ops.rs | 109 +++++++++++++++++- core/to_from_v8.rs | 19 --- 5 files changed, 413 insertions(+), 23 deletions(-) create mode 100644 core/convert.rs delete mode 100644 core/to_from_v8.rs diff --git a/core/convert.rs b/core/convert.rs new file mode 100644 index 000000000..5c04803e6 --- /dev/null +++ b/core/convert.rs @@ -0,0 +1,275 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +use crate::error::StdAnyError; +use crate::runtime::ops; +use std::convert::Infallible; + +/// A conversion from a rust value to a v8 value. +/// +/// When passing data from Rust into JS, either +/// via an op or by calling a JS function directly, +/// you need to serialize the data into a native +/// V8 value. When using the [`op2`][deno_core::op2] macro, the return +/// value is converted to a `v8::Local` automatically, +/// and the strategy for conversion is controlled by attributes +/// like `#[smi]`, `#[number]`, `#[string]`. For types with support +/// built-in to the op2 macro, like primitives, strings, and buffers, +/// these attributes are sufficient and you don't need to worry about this trait. +/// +/// If, however, you want to return a custom type from an op, or +/// simply want more control over the conversion process, +/// you can implement the `ToV8` trait. This allows you the +/// choose the best serialization strategy for your specific use case. +/// You can then use the `#[to_v8]` attribute to indicate +/// that the `#[op2]` macro should call your implementation for the conversion. +/// +/// # Example +/// +/// ```ignore +/// use deno_core::ToV8; +/// use deno_core::convert::Smi; +/// use deno_core::op2; +/// +/// struct Foo(i32); +/// +/// impl<'a> ToV8<'a> for Foo { +/// // This conversion can never fail, so we use `Infallible` as the error type. +/// // Any error type that implements `std::error::Error` can be used here. +/// type Error = std::convert::Infallible; +/// +/// fn to_v8(self, scope: &mut v8::HandleScope<'a>) -> Result, Self::Error> { +/// // For performance, pass this value as a `v8::Integer` (i.e. a `smi`). +/// // The `Smi` wrapper type implements this conversion for you. +/// Smi(self.0).to_v8(scope) +/// } +/// } +/// +/// // using the `#[to_v8]` attribute tells the `op2` macro to call this implementation. +/// #[op2] +/// #[to_v8] +/// fn op_foo() -> Foo { +/// Foo(42) +/// } +/// ``` +/// +/// # Performance Notes +/// ## Structs +/// The natural representation of a struct in JS is an object with fields +/// corresponding the struct. This, however, is a performance footgun and +/// you should avoid creating and passing objects to V8 whenever possible. +/// In general, if you need to pass a compound type to JS, it is more performant to serialize +/// to a tuple (a `v8::Array`) rather than an object. +/// Object keys are V8 strings, and strings are expensive to pass to V8 +/// and they have to be managed by the V8 garbage collector. +/// Tuples, on the other hand, are keyed by `smi`s, which are immediates +/// and don't require allocation or garbage collection. +pub trait ToV8<'a> { + type Error: std::error::Error + Send + Sync + 'static; + + /// Converts the value to a V8 value. + fn to_v8( + self, + scope: &mut v8::HandleScope<'a>, + ) -> Result, Self::Error>; +} + +/// A conversion from a v8 value to a rust value. +/// +/// When writing a op, or otherwise writing a function in Rust called +/// from JS, arguments passed from JS are represented as [`v8::Local>`][deno_core::v8::Value]. +/// To convert these values into custom Rust types, you can implement the [`FromV8`] trait. +/// +/// Once you've implemented this trait, you can use the `#[from_v8]` attribute +/// to tell the [`op2`][deno_core::op2] macro to use your implementation to convert the argument +/// to the desired type. +/// +/// # Example +/// +/// ```ignore +/// use deno_core::FromV8; +/// use deno_core::convert::Smi; +/// use deno_core::op2; +/// +/// struct Foo(i32); +/// +/// impl<'a> FromV8<'a> for Foo { +/// // This conversion can fail, so we use `deno_core::error::StdAnyError` as the error type. +/// // Any error type that implements `std::error::Error` can be used here. +/// type Error = deno_core::error::StdAnyError; +/// +/// fn from_v8(scope: &mut v8::HandleScope<'a>, value: v8::Local<'a, v8::Value>) -> Result { +/// /// We expect this value to be a `v8::Integer`, so we use the [`Smi`][deno_core::convert::Smi] wrapper type to convert it. +/// Smi::from_v8(scope, value).map(|Smi(v)| Foo(v)) +/// } +/// } +/// +/// // using the `#[from_v8]` attribute tells the `op2` macro to call this implementation. +/// #[op2] +/// fn op_foo(#[from_v8] foo: Foo) { +/// let Foo(_) = foo; +/// } +/// ``` +pub trait FromV8<'a>: Sized { + type Error: std::error::Error + Send + Sync + 'static; + + /// Converts a V8 value to a Rust value. + fn from_v8( + scope: &mut v8::HandleScope<'a>, + value: v8::Local<'a, v8::Value>, + ) -> Result; +} + +// impls + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +/// Marks a numeric type as being serialized as a v8 `smi` in a `v8::Integer`. +#[repr(transparent)] +pub struct Smi(pub T); + +/// A trait for types that can represent a JS `smi`. +pub trait SmallInt { + const NAME: &'static str; + + #[allow(clippy::wrong_self_convention)] + fn as_i32(self) -> i32; + fn from_i32(value: i32) -> Self; +} + +macro_rules! impl_smallint { + (for $($t:ty),*) => { + $( + impl SmallInt for $t { + const NAME: &'static str = stringify!($t); + #[allow(clippy::wrong_self_convention)] + #[inline(always)] + fn as_i32(self) -> i32 { + self as _ + } + + #[inline(always)] + fn from_i32(value: i32) -> Self { + value as _ + } + } + )* + }; +} + +impl_smallint!(for u8, u16, u32, u64, usize, i8, i16, i32, i64, isize); + +impl<'a, T: SmallInt> ToV8<'a> for Smi { + type Error = Infallible; + + #[inline] + fn to_v8( + self, + scope: &mut v8::HandleScope<'a>, + ) -> Result, Self::Error> { + Ok(v8::Integer::new(scope, self.0.as_i32()).into()) + } +} + +impl<'a, T: SmallInt> FromV8<'a> for Smi { + type Error = StdAnyError; + + #[inline] + fn from_v8( + _scope: &mut v8::HandleScope<'a>, + value: v8::Local<'a, v8::Value>, + ) -> Result { + let v = crate::runtime::ops::to_i32_option(&value).ok_or_else(|| { + crate::error::type_error(format!("Expected {}", T::NAME)) + })?; + Ok(Smi(T::from_i32(v))) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +/// Marks a numeric type as being serialized as a v8 `number` in a `v8::Number`. +#[repr(transparent)] +pub struct Number(pub T); + +/// A trait for types that can represent a JS `number`. +pub trait Numeric: Sized { + const NAME: &'static str; + #[allow(clippy::wrong_self_convention)] + fn as_f64(self) -> f64; + fn from_value(value: &v8::Value) -> Option; +} + +macro_rules! impl_numeric { + ($($t:ty : $from: path ),*) => { + $( + impl Numeric for $t { + const NAME: &'static str = stringify!($t); + #[inline(always)] + fn from_value(value: &v8::Value) -> Option { + $from(value).map(|v| v as _) + } + + #[allow(clippy::wrong_self_convention)] + #[inline(always)] + fn as_f64(self) -> f64 { + self as _ + } + } + )* + }; +} + +impl_numeric!( + f32 : ops::to_f32_option, + f64 : ops::to_f64_option, + u32 : ops::to_u32_option, + u64 : ops::to_u64_option, + usize : ops::to_u64_option, + i32 : ops::to_i32_option, + i64 : ops::to_i64_option, + isize : ops::to_i64_option +); + +impl<'a, T: Numeric> ToV8<'a> for Number { + type Error = Infallible; + #[inline] + fn to_v8( + self, + scope: &mut v8::HandleScope<'a>, + ) -> Result, Self::Error> { + Ok(v8::Number::new(scope, self.0.as_f64()).into()) + } +} + +impl<'a, T: Numeric> FromV8<'a> for Number { + type Error = StdAnyError; + #[inline] + fn from_v8( + _scope: &mut v8::HandleScope<'a>, + value: v8::Local<'a, v8::Value>, + ) -> Result { + T::from_value(&value).map(Number).ok_or_else(|| { + crate::error::type_error(format!("Expected {}", T::NAME)).into() + }) + } +} + +impl<'a> ToV8<'a> for bool { + type Error = Infallible; + #[inline] + fn to_v8( + self, + scope: &mut v8::HandleScope<'a>, + ) -> Result, Self::Error> { + Ok(v8::Boolean::new(scope, self).into()) + } +} + +impl<'a> FromV8<'a> for bool { + type Error = Infallible; + #[inline] + fn from_v8( + _scope: &mut v8::HandleScope<'a>, + value: v8::Local<'a, v8::Value>, + ) -> Result { + Ok(value.is_true()) + } +} diff --git a/core/error.rs b/core/error.rs index 5f14cf555..6d8a3aa00 100644 --- a/core/error.rs +++ b/core/error.rs @@ -96,6 +96,33 @@ pub fn get_custom_error_class(error: &Error) -> Option<&'static str> { error.downcast_ref::().map(|e| e.class) } +/// A wrapper around `anyhow::Error` that implements `std::error::Error` +#[repr(transparent)] +pub struct StdAnyError(pub Error); +impl std::fmt::Debug for StdAnyError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}", self.0) + } +} + +impl std::fmt::Display for StdAnyError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::error::Error for StdAnyError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.0.source() + } +} + +impl From for StdAnyError { + fn from(err: Error) -> Self { + Self(err) + } +} + pub fn to_v8_error<'a>( scope: &mut v8::HandleScope<'a>, get_class: GetErrorClassFn, diff --git a/core/lib.rs b/core/lib.rs index af2bbf399..88a61992c 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -7,6 +7,7 @@ pub mod arena; mod async_cancel; mod async_cell; +pub mod convert; pub mod cppgc; pub mod error; mod error_codes; @@ -31,7 +32,6 @@ mod path; mod runtime; mod source_map; mod tasks; -mod to_from_v8; mod web_timeout; // Re-exports @@ -67,6 +67,8 @@ pub use crate::async_cell::AsyncRefCell; pub use crate::async_cell::AsyncRefFuture; pub use crate::async_cell::RcLike; pub use crate::async_cell::RcRef; +pub use crate::convert::FromV8; +pub use crate::convert::ToV8; pub use crate::error::GetErrorClassFn; pub use crate::error::JsErrorCreateFn; pub use crate::extensions::Extension; @@ -156,8 +158,6 @@ pub use crate::source_map::SourceMapData; pub use crate::source_map::SourceMapGetter; pub use crate::tasks::V8CrossThreadTaskSpawner; pub use crate::tasks::V8TaskSpawner; -pub use crate::to_from_v8::FromV8; -pub use crate::to_from_v8::ToV8; // Ensure we can use op2 in deno_core without any hackery. extern crate self as deno_core; diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs index 794452422..6a1c1934c 100644 --- a/core/runtime/ops.rs +++ b/core/runtime/ops.rs @@ -94,7 +94,7 @@ pub fn opstate_borrow_mut(state: &mut OpState) -> &mut T { state.borrow_mut() } -pub fn to_u32_option(number: &v8::Value) -> Option { +pub fn to_u32_option(number: &v8::Value) -> Option { try_number_some!(number Integer is_uint32); try_number_some!(number Int32 is_int32); try_number_some!(number Number is_number); @@ -515,6 +515,8 @@ pub fn to_v8_slice_any( #[allow(clippy::print_stdout, clippy::print_stderr, clippy::unused_async)] #[cfg(all(test, not(miri)))] mod tests { + use crate::convert::Number; + use crate::convert::Smi; use crate::error::generic_error; use crate::error::AnyError; use crate::error::JsError; @@ -522,9 +524,11 @@ mod tests { use crate::external::ExternalPointer; use crate::op2; use crate::runtime::JsRuntimeState; + use crate::FromV8; use crate::JsRuntime; use crate::OpState; use crate::RuntimeOptions; + use crate::ToV8; use anyhow::bail; use anyhow::Error; use bytes::BytesMut; @@ -632,6 +636,10 @@ mod tests { op_async_buffer_impl, op_async_external, op_async_serde_option_v8, + + op_smi_to_from_v8, + op_number_to_from_v8, + op_bool_to_from_v8, ], state = |state| { state.put(1234u32); @@ -2317,4 +2325,103 @@ mod tests { .await?; Ok(()) } + + #[op2] + #[to_v8] + pub fn op_smi_to_from_v8(#[from_v8] value: Smi) -> Smi { + value + } + + #[tokio::test] + pub async fn test_op_smi_to_from_v8() -> Result<(), Box> + { + run_test2( + JIT_ITERATIONS, + "op_smi_to_from_v8", + r" + for (const n of [-Math.pow(2, 31), -1, 0, 1, Math.pow(2, 31) - 1]) { + assert(op_smi_to_from_v8(n) == n); + }", + )?; + Ok(()) + } + + #[op2] + #[to_v8] + pub fn op_number_to_from_v8(#[from_v8] value: Number) -> Number { + value + } + + #[tokio::test] + pub async fn test_op_number_to_from_v8( + ) -> Result<(), Box> { + run_test2( + JIT_ITERATIONS, + "op_number_to_from_v8", + r" + for ( + const n of [ + Number.MIN_VALUE, + Number.MIN_SAFE_INTEGER, + -1, + 0, + 1, + Number.MAX_SAFE_INTEGER, + Number.MAX_VALUE, + Number.POSITIVE_INFINITY, + Number.NEGATIVE_INFINITY, + ] + ) { + assert(op_number_to_from_v8(n) === n); + } + + assert(isNaN(op_number_to_from_v8(Number.NaN))); + ", + )?; + Ok(()) + } + + struct Bool(bool); + + impl<'a> ToV8<'a> for Bool { + type Error = std::convert::Infallible; + + fn to_v8( + self, + scope: &mut v8::HandleScope<'a>, + ) -> Result, Self::Error> { + self.0.to_v8(scope) + } + } + + impl<'a> FromV8<'a> for Bool { + type Error = std::convert::Infallible; + + fn from_v8( + scope: &mut v8::HandleScope<'a>, + value: v8::Local<'a, v8::Value>, + ) -> Result { + bool::from_v8(scope, value).map(Bool) + } + } + + #[op2] + #[to_v8] + fn op_bool_to_from_v8(#[from_v8] value: Bool) -> Bool { + value + } + + #[tokio::test] + pub async fn test_op_bool_to_from_v8( + ) -> Result<(), Box> { + run_test2( + JIT_ITERATIONS, + "op_bool_to_from_v8", + r" + for (const v of [true, false]) { + assert(op_bool_to_from_v8(v) == v); + }", + )?; + Ok(()) + } } diff --git a/core/to_from_v8.rs b/core/to_from_v8.rs deleted file mode 100644 index 763fbebe7..000000000 --- a/core/to_from_v8.rs +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. - -pub trait ToV8<'a> { - type Error: std::error::Error + Send + Sync + 'static; - - fn to_v8( - self, - scope: &mut v8::HandleScope<'a>, - ) -> Result, Self::Error>; -} - -pub trait FromV8<'a>: Sized { - type Error: std::error::Error + Send + Sync + 'static; - - fn from_v8( - scope: &mut v8::HandleScope<'a>, - value: v8::Local<'a, v8::Value>, - ) -> Result; -} From 1ac34fd8f24683f52518126b2db4553b3d53ea29 Mon Sep 17 00:00:00 2001 From: denobot <33910674+denobot@users.noreply.github.com> Date: Tue, 7 May 2024 08:49:39 +0900 Subject: [PATCH 2/2] 0.279.0 (#731) Bumped versions for 0.279.0 Please ensure: - [ ] Crate versions are bumped correctly To make edits to this PR: ```shell git fetch upstream release_0_279.0 && git checkout -b release_0_279.0 upstream/release_0_279.0 ``` cc @nathanwhit Co-authored-by: nathanwhit --- Cargo.lock | 6 +++--- Cargo.toml | 6 +++--- core/Cargo.toml | 2 +- ops/Cargo.toml | 2 +- serde_v8/Cargo.toml | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b990f4b4e..5793912da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -516,7 +516,7 @@ dependencies = [ [[package]] name = "deno_core" -version = "0.278.0" +version = "0.279.0" dependencies = [ "anyhow", "bencher", @@ -583,7 +583,7 @@ dependencies = [ [[package]] name = "deno_ops" -version = "0.154.0" +version = "0.155.0" dependencies = [ "pretty_assertions", "prettyplease", @@ -1756,7 +1756,7 @@ dependencies = [ [[package]] name = "serde_v8" -version = "0.187.0" +version = "0.188.0" dependencies = [ "bencher", "num-bigint", diff --git a/Cargo.toml b/Cargo.toml index 1c996bd32..60d7af6c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,9 +19,9 @@ repository = "https://github.com/denoland/deno_core" [workspace.dependencies] # Local dependencies -deno_core = { version = "0.278.0", path = "./core" } -deno_ops = { version = "0.154.0", path = "./ops" } -serde_v8 = { version = "0.187.0", path = "./serde_v8" } +deno_core = { version = "0.279.0", path = "./core" } +deno_ops = { version = "0.155.0", path = "./ops" } +serde_v8 = { version = "0.188.0", path = "./serde_v8" } deno_core_testing = { path = "./testing" } v8 = { version = "0.91.0", default-features = false } diff --git a/core/Cargo.toml b/core/Cargo.toml index 50ef27e35..5f49e9821 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "deno_core" -version = "0.278.0" +version = "0.279.0" authors.workspace = true edition.workspace = true license.workspace = true diff --git a/ops/Cargo.toml b/ops/Cargo.toml index 79216f15d..5d48620b2 100644 --- a/ops/Cargo.toml +++ b/ops/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "deno_ops" -version = "0.154.0" +version = "0.155.0" authors.workspace = true edition.workspace = true license.workspace = true diff --git a/serde_v8/Cargo.toml b/serde_v8/Cargo.toml index 972c45c4a..31514aad4 100644 --- a/serde_v8/Cargo.toml +++ b/serde_v8/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "serde_v8" -version = "0.187.0" +version = "0.188.0" authors.workspace = true edition.workspace = true license.workspace = true