From 3d8e19d1275a751b67e46322f3cc96017e182332 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 22 Dec 2025 14:37:43 -0500 Subject: [PATCH 1/9] support non-result return values in jsg rust --- src/rust/jsg-macros/README.md | 25 ++++ src/rust/jsg-macros/lib.rs | 29 ++++- src/rust/jsg-test/tests/resource_callback.rs | 128 +++++++++++++++++++ src/rust/jsg/ffi.c++ | 10 ++ src/rust/jsg/ffi.h | 2 + src/rust/jsg/lib.rs | 22 +--- src/rust/jsg/v8.rs | 16 ++- 7 files changed, 211 insertions(+), 21 deletions(-) diff --git a/src/rust/jsg-macros/README.md b/src/rust/jsg-macros/README.md index c72483ad3e1..eebaa3092ce 100644 --- a/src/rust/jsg-macros/README.md +++ b/src/rust/jsg-macros/README.md @@ -20,6 +20,31 @@ pub struct MyRecord { } ``` +## `#[jsg_method]` + +Generates FFI callback functions for JSG resource methods. The `name` parameter is optional and defaults to converting the method name from `snake_case` to `camelCase`. + +Return values are handled via the `jsg::Wrappable` trait. Any type implementing `Wrappable` can be returned. + +```rust +impl DnsUtil { + #[jsg_method(name = "parseCaaRecord")] + pub fn parse_caa_record(&self, record: &str) -> Result { + // Errors are thrown as JavaScript exceptions + } + + #[jsg_method] + pub fn get_name(&self) -> String { + self.name.clone() + } + + #[jsg_method] + pub fn reset(&self) { + // Void methods return undefined in JavaScript + } +} +``` + ## `#[jsg_resource]` Generates boilerplate for JSG resources. Applied to both struct definitions and impl blocks. Automatically implements `jsg::Type::class_name()` using the struct name, or a custom name if provided via the `name` parameter. diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 966dbc7d737..4af1074e835 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -160,6 +160,33 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { /// } /// ``` /// +/// # Return Types +/// +/// Return values are handled via the [`jsg::Wrappable`] trait. Any type implementing +/// `Wrappable` can be returned from a method. Built-in implementations include: +/// +/// - `()` - returns `undefined` in JavaScript +/// - `Result` where `T: Wrappable` - unwraps the value or throws a JavaScript exception +/// - `NonCoercible` - unwraps and returns the inner value +/// - `String`, `bool`, `f64` - primitive types +/// +/// ```ignore +/// #[jsg_method] +/// pub fn parse_record(&self, data: &str) -> Result { +/// // Errors are thrown as JavaScript exceptions +/// } +/// +/// #[jsg_method] +/// pub fn get_name(&self) -> String { +/// self.name.clone() +/// } +/// +/// #[jsg_method] +/// pub fn reset(&self) { +/// // Returns undefined +/// } +/// ``` +/// /// # Example /// /// ```ignore @@ -236,7 +263,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); let result = self_.#fn_name(#(#arg_refs),*); - unsafe { jsg::handle_result(&mut lock, &mut args, result) }; + jsg::Wrappable::wrap_return(result, &mut lock, &mut args); } }; diff --git a/src/rust/jsg-test/tests/resource_callback.rs b/src/rust/jsg-test/tests/resource_callback.rs index 9462ea0bbfb..808c1deb0b0 100644 --- a/src/rust/jsg-test/tests/resource_callback.rs +++ b/src/rust/jsg-test/tests/resource_callback.rs @@ -5,6 +5,9 @@ //! V8's internal field embedder data type tags are used correctly when getting //! aligned pointers from internal fields. +use std::cell::Cell; +use std::rc::Rc; + use jsg::Lock; use jsg::ResourceState; use jsg::ResourceTemplate; @@ -28,6 +31,41 @@ impl EchoResource { } } +#[jsg_resource] +struct DirectReturnResource { + _state: ResourceState, + name: String, + counter: Rc>, +} + +#[jsg_resource] +impl DirectReturnResource { + #[jsg_method] + pub fn get_name(&self) -> String { + self.name.clone() + } + + #[jsg_method] + pub fn is_valid(&self) -> bool { + !self.name.is_empty() + } + + #[jsg_method] + pub fn get_counter(&self) -> f64 { + f64::from(self.counter.get()) + } + + #[jsg_method] + pub fn increment_counter(&self) { + self.counter.set(self.counter.get() + 1); + } + + #[jsg_method] + pub fn maybe_name(&self) -> Option { + Some(self.name.clone()).filter(|s| !s.is_empty()) + } +} + /// Validates that resource methods can be called from JavaScript. /// This test ensures the embedder data type tag is correctly used when /// unwrapping resource pointers from V8 internal fields. @@ -91,3 +129,93 @@ fn resource_method_can_be_called_multiple_times() { ); }); } + +/// Validates that methods can return values directly without Result wrapper. +#[test] +fn resource_method_returns_non_result_values() { + let harness = crate::Harness::new(); + harness.run_in_context(|isolate, ctx| unsafe { + let mut lock = Lock::from_isolate_ptr(isolate); + let counter = Rc::new(Cell::new(42)); + let resource = jsg::Ref::new(DirectReturnResource { + _state: ResourceState::default(), + name: "TestResource".to_owned(), + counter: counter.clone(), + }); + let mut template = DirectReturnResourceTemplate::new(&mut lock); + let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); + ctx.set_global_safe("resource", wrapped.into_ffi()); + + assert_eq!( + ctx.eval_safe("resource.getName()"), + EvalResult { + success: true, + result_type: "string".to_owned(), + result_value: "TestResource".to_owned(), + } + ); + + assert_eq!( + ctx.eval_safe("resource.isValid()"), + EvalResult { + success: true, + result_type: "boolean".to_owned(), + result_value: "true".to_owned(), + } + ); + + assert_eq!( + ctx.eval_safe("resource.getCounter()"), + EvalResult { + success: true, + result_type: "number".to_owned(), + result_value: "42".to_owned(), + } + ); + + assert_eq!( + ctx.eval_safe("resource.incrementCounter()"), + EvalResult { + success: true, + result_type: "undefined".to_owned(), + result_value: "undefined".to_owned(), + } + ); + assert_eq!(counter.get(), 43); + + assert_eq!( + ctx.eval_safe("resource.maybeName()"), + EvalResult { + success: true, + result_type: "string".to_owned(), + result_value: "TestResource".to_owned(), + } + ); + }); +} + +/// Validates that Option returns null for None. +#[test] +fn resource_method_returns_null_for_none() { + let harness = crate::Harness::new(); + harness.run_in_context(|isolate, ctx| unsafe { + let mut lock = Lock::from_isolate_ptr(isolate); + let resource = jsg::Ref::new(DirectReturnResource { + _state: ResourceState::default(), + name: String::new(), + counter: Rc::new(Cell::new(0)), + }); + let mut template = DirectReturnResourceTemplate::new(&mut lock); + let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); + ctx.set_global_safe("resource", wrapped.into_ffi()); + + assert_eq!( + ctx.eval_safe("resource.maybeName()"), + EvalResult { + success: true, + result_type: "object".to_owned(), + result_value: "null".to_owned(), + } + ); + }); +} diff --git a/src/rust/jsg/ffi.c++ b/src/rust/jsg/ffi.c++ index 0d699d9c75f..eb610f08b97 100644 --- a/src/rust/jsg/ffi.c++ +++ b/src/rust/jsg/ffi.c++ @@ -48,6 +48,16 @@ Local local_new_object(Isolate* isolate) { return to_ffi(kj::mv(object)); } +Local local_new_null(Isolate* isolate) { + v8::Local null = v8::Null(isolate); + return to_ffi(kj::mv(null)); +} + +Local local_new_undefined(Isolate* isolate) { + v8::Local undefined = v8::Undefined(isolate); + return to_ffi(kj::mv(undefined)); +} + bool local_eq(const Local& lhs, const Local& rhs) { return local_as_ref_from_ffi(lhs) == local_as_ref_from_ffi(rhs); } diff --git a/src/rust/jsg/ffi.h b/src/rust/jsg/ffi.h index 05769480b1e..02be16bec44 100644 --- a/src/rust/jsg/ffi.h +++ b/src/rust/jsg/ffi.h @@ -34,6 +34,8 @@ Local local_new_number(Isolate* isolate, double value); Local local_new_string(Isolate* isolate, ::rust::Str value); Local local_new_boolean(Isolate* isolate, bool value); Local local_new_object(Isolate* isolate); +Local local_new_null(Isolate* isolate); +Local local_new_undefined(Isolate* isolate); bool local_eq(const Local& lhs, const Local& rhs); bool local_has_value(const Local& val); bool local_is_string(const Local& val); diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index 31eae0f5935..ef601552979 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -11,7 +11,10 @@ use kj_rs::KjMaybe; pub mod modules; pub mod v8; +mod wrappable; + pub use v8::ffi::ExceptionType; +pub use wrappable::Wrappable; use crate::v8::ToLocalValue; @@ -612,25 +615,6 @@ unsafe fn realm_create(isolate: *mut v8::ffi::Isolate) -> Box { unsafe { Box::new(Realm::from_isolate(v8::IsolatePtr::from_ffi(isolate))) } } -/// Handles a result by setting the return value or throwing an error. -/// -/// # Safety -/// The caller must ensure V8 operations are performed within the correct isolate/context. -pub unsafe fn handle_result, E: std::fmt::Display>( - lock: &mut Lock, - args: &mut v8::FunctionCallbackInfo, - result: Result, -) { - match result { - Ok(result) => args.set_return_value(T::wrap(result, lock)), - Err(err) => { - // TODO(soon): Make sure to use jsg::Error trait here and dynamically call proper method to throw the error. - let description = err.to_string(); - unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &description) }; - } - } -} - impl Type for String { type This = Self; diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 39c1d95c996..a13ce3a353b 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -52,6 +52,8 @@ pub mod ffi { pub unsafe fn local_new_string(isolate: *mut Isolate, value: &str) -> Local; pub unsafe fn local_new_boolean(isolate: *mut Isolate, value: bool) -> Local; pub unsafe fn local_new_object(isolate: *mut Isolate) -> Local; + pub unsafe fn local_new_null(isolate: *mut Isolate) -> Local; + pub unsafe fn local_new_undefined(isolate: *mut Isolate) -> Local; pub unsafe fn local_eq(lhs: &Local, rhs: &Local) -> bool; pub unsafe fn local_has_value(value: &Local) -> bool; pub unsafe fn local_is_string(value: &Local) -> bool; @@ -210,7 +212,6 @@ impl Drop for Local<'_, T> { } // Common implementations for all Local<'a, T> -#[expect(clippy::elidable_lifetime_names)] impl<'a, T> Local<'a, T> { /// Creates a `Local` from an FFI handle. /// @@ -245,6 +246,19 @@ impl<'a, T> Local<'a, T> { &self.handle } + pub fn null(lock: &mut crate::Lock) -> Local<'a, Value> { + unsafe { Local::from_ffi(lock.isolate(), ffi::local_new_null(lock.isolate().as_ffi())) } + } + + pub fn undefined(lock: &mut crate::Lock) -> Local<'a, Value> { + unsafe { + Local::from_ffi( + lock.isolate(), + ffi::local_new_undefined(lock.isolate().as_ffi()), + ) + } + } + pub fn has_value(&self) -> bool { unsafe { ffi::local_has_value(&self.handle) } } From 3f0bce640555ec457b54e8439b0316a5af3f7dc4 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 23 Dec 2025 10:51:18 -0500 Subject: [PATCH 2/9] implement jsg::Wrappable trait --- src/rust/jsg-macros/README.md | 2 +- src/rust/jsg-macros/lib.rs | 86 +++----------- src/rust/jsg/lib.rs | 71 ------------ src/rust/jsg/wrappable.rs | 203 ++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 141 deletions(-) create mode 100644 src/rust/jsg/wrappable.rs diff --git a/src/rust/jsg-macros/README.md b/src/rust/jsg-macros/README.md index eebaa3092ce..3acfe2ee93c 100644 --- a/src/rust/jsg-macros/README.md +++ b/src/rust/jsg-macros/README.md @@ -24,7 +24,7 @@ pub struct MyRecord { Generates FFI callback functions for JSG resource methods. The `name` parameter is optional and defaults to converting the method name from `snake_case` to `camelCase`. -Return values are handled via the `jsg::Wrappable` trait. Any type implementing `Wrappable` can be returned. +Parameters and return values are handled via the `jsg::Wrappable` trait. Any type implementing `Wrappable` can be used as a parameter or return value. Use `NonCoercible` to reject values that would require JavaScript coercion. ```rust impl DnsUtil { diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 4af1074e835..d9f48d0207e 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -130,48 +130,16 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { /// Creates a `{method_name}_callback` extern "C" function that bridges JavaScript and Rust. /// If no name is provided, automatically converts `snake_case` to `camelCase`. /// -/// # Supported Parameter Types +/// Parameters and return values are handled via the [`jsg::Wrappable`] trait. +/// See `jsg/wrappable.rs` for supported types. /// -/// | Type | `T` | `NonCoercible` | -/// |------------|---------------|-------------------| -/// | `&str` | ✅ Supported | ❌ | -/// | `T: Type` | ❌ | ✅ Supported | +/// Use `NonCoercible` to reject values that would require JavaScript coercion. +/// Use `&str` for string parameters (internally unwrapped as `String` and borrowed). /// -/// Any type implementing [`jsg::Type`] can be used with `NonCoercible`. -/// Built-in types include `String`, `bool`, and `f64`. -/// -/// # `NonCoercible` Parameters -/// -/// Use `NonCoercible` when you want to accept a value only if it's already the expected -/// type, without JavaScript's automatic type coercion: -/// -/// ```ignore -/// #[jsg_method] -/// pub fn strict_string(&self, param: NonCoercible) -> Result { -/// // Only accepts actual strings - passing null/undefined/numbers will throw -/// // Access via Deref: *param, or via AsRef: param.as_ref() -/// Ok(param.as_ref().clone()) -/// } -/// -/// #[jsg_method] -/// pub fn strict_bool(&self, param: NonCoercible) -> Result { -/// // Only accepts actual booleans -/// Ok(*param) -/// } -/// ``` -/// -/// # Return Types -/// -/// Return values are handled via the [`jsg::Wrappable`] trait. Any type implementing -/// `Wrappable` can be returned from a method. Built-in implementations include: -/// -/// - `()` - returns `undefined` in JavaScript -/// - `Result` where `T: Wrappable` - unwraps the value or throws a JavaScript exception -/// - `NonCoercible` - unwraps and returns the inner value -/// - `String`, `bool`, `f64` - primitive types +/// # Example /// /// ```ignore -/// #[jsg_method] +/// #[jsg_method(name = "parseRecord")] /// pub fn parse_record(&self, data: &str) -> Result { /// // Errors are thrown as JavaScript exceptions /// } @@ -180,27 +148,6 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { /// pub fn get_name(&self) -> String { /// self.name.clone() /// } -/// -/// #[jsg_method] -/// pub fn reset(&self) { -/// // Returns undefined -/// } -/// ``` -/// -/// # Example -/// -/// ```ignore -/// // With explicit name -/// #[jsg_method(name = "parseRecord")] -/// pub fn parse_record(&self, data: &str) -> Result { -/// // implementation -/// } -/// -/// // Without name - automatically becomes "parseRecord" -/// #[jsg_method] -/// pub fn parse_record(&self, data: &str) -> Result { -/// // implementation -/// } /// ``` #[proc_macro_attribute] pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { @@ -296,7 +243,8 @@ fn generate_unwrap_code( ty: &Type, index: usize, ) -> quote::__private::TokenStream { - // Check for NonCoercible types + // Check for NonCoercible types - these use their inherent unwrap method + // which validates the type and returns Option if is_non_coercible(ty) { return quote! { let Some(#arg_name) = (unsafe { @@ -307,16 +255,16 @@ fn generate_unwrap_code( }; } + // For &str references, unwrap as String and borrow if is_str_reference(ty) { - quote! { - let #arg_name = unsafe { - jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), args.get(#index).into_ffi()) - }; - } - } else { - quote! { - compile_error!("Unsupported parameter type for jsg::method. Currently only &str and NonCoercible are supported."); - } + return quote! { + let #arg_name = ::unwrap(lock.isolate(), args.get(#index)); + }; + } + + // For all other types, use Wrappable::unwrap + quote! { + let #arg_name = <#ty as jsg::Wrappable>::unwrap(lock.isolate(), args.get(#index)); } } diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index ef601552979..438735155d7 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -16,8 +16,6 @@ mod wrappable; pub use v8::ffi::ExceptionType; pub use wrappable::Wrappable; -use crate::v8::ToLocalValue; - #[cxx::bridge(namespace = "workerd::rust::jsg")] mod ffi { extern "Rust" { @@ -614,72 +612,3 @@ impl Drop for Realm { unsafe fn realm_create(isolate: *mut v8::ffi::Isolate) -> Box { unsafe { Box::new(Realm::from_isolate(v8::IsolatePtr::from_ffi(isolate))) } } - -impl Type for String { - type This = Self; - - fn class_name() -> &'static str { - "string" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_string() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_string(isolate.as_ffi(), value.into_ffi()) } - } -} - -impl Type for bool { - type This = Self; - - fn class_name() -> &'static str { - "boolean" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_boolean() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_boolean(isolate.as_ffi(), value.into_ffi()) } - } -} - -impl Type for f64 { - type This = Self; - - fn class_name() -> &'static str { - "number" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_number() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_number(isolate.as_ffi(), value.into_ffi()) } - } -} diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs new file mode 100644 index 00000000000..495e8e3a280 --- /dev/null +++ b/src/rust/jsg/wrappable.rs @@ -0,0 +1,203 @@ +// Copyright (c) 2017-2022 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +//! Trait for converting between Rust and JavaScript values. +//! +//! # Supported Types +//! +//! | Rust Type | JavaScript Type | +//! |-----------|-----------------| +//! | `()` | `undefined` | +//! | `String` | `string` | +//! | `bool` | `boolean` | +//! | `f64` | `number` | +//! | `Option` | `T` or `null` | +//! | `Result` | `T` or throws | +//! | `NonCoercible` | `T` | +//! | `T: Struct` | `object` | + +use std::fmt::Display; + +use crate::Lock; +use crate::NonCoercible; +use crate::Struct; +use crate::Type; +use crate::v8; +use crate::v8::ToLocalValue; + +impl Type for String { + type This = Self; + + fn class_name() -> &'static str { + "string" + } + + fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + this.to_local(lock) + } + + fn is_exact(value: &v8::Local) -> bool { + value.is_string() + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + unsafe { v8::ffi::unwrap_string(isolate.as_ffi(), value.into_ffi()) } + } +} + +impl Type for bool { + type This = Self; + + fn class_name() -> &'static str { + "boolean" + } + + fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + this.to_local(lock) + } + + fn is_exact(value: &v8::Local) -> bool { + value.is_boolean() + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + unsafe { v8::ffi::unwrap_boolean(isolate.as_ffi(), value.into_ffi()) } + } +} + +impl Type for f64 { + type This = Self; + + fn class_name() -> &'static str { + "number" + } + + fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + this.to_local(lock) + } + + fn is_exact(value: &v8::Local) -> bool { + value.is_number() + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + unsafe { v8::ffi::unwrap_number(isolate.as_ffi(), value.into_ffi()) } + } +} + +/// Trait for converting between Rust and JavaScript values. +/// +/// Provides bidirectional conversion: `wrap` converts Rust to JavaScript, +/// `unwrap` converts JavaScript to Rust. The `wrap_return` method is a +/// convenience for setting function return values. +pub trait Wrappable: Sized { + /// Converts this Rust value into a JavaScript value. + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value>; + + /// Converts a JavaScript value into this Rust type. + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self; + + /// Wraps this value and sets it as the function return value. + fn wrap_return(self, lock: &mut Lock, args: &mut v8::FunctionCallbackInfo) { + args.set_return_value(self.wrap(lock)); + } +} + +impl Wrappable for () { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + v8::Local::::undefined(lock) + } + + fn unwrap(_isolate: v8::IsolatePtr, _value: v8::Local) -> Self {} +} + +impl Wrappable for Result { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + match self { + Ok(value) => value.wrap(lock), + Err(_) => unreachable!("Cannot wrap Result::Err"), + } + } + + fn unwrap(_isolate: v8::IsolatePtr, _value: v8::Local) -> Self { + unreachable!("Cannot unwrap into Result") + } + + fn wrap_return(self, lock: &mut Lock, args: &mut v8::FunctionCallbackInfo) { + match self { + Ok(value) => value.wrap_return(lock, args), + Err(err) => { + // TODO(soon): Use jsg::Error trait to dynamically call proper method to throw the error. + let description = err.to_string(); + unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &description) }; + } + } + } +} + +impl Wrappable for Option { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + match self { + Some(value) => value.wrap(lock), + None => v8::Local::::null(lock), + } + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + if value.is_null_or_undefined() { + None + } else { + Some(T::unwrap(isolate, value)) + } + } +} + +impl> Wrappable for NonCoercible { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + T::wrap(self.value, lock) + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + Self::new(T::unwrap(isolate, value)) + } +} + +impl> Wrappable for T { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + ::wrap(self, lock) + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + ::unwrap(isolate, value) + } +} + +/// Implements `Wrappable` for types that already implement `Type`. +macro_rules! impl_wrappable_for_type { + ($($t:ty),*) => { + $( + impl Wrappable for $t { + fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + ::wrap(self, lock) + } + + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + ::unwrap(isolate, value) + } + } + )* + }; +} + +// When adding new primitive types that implement `Type`, add them here. +impl_wrappable_for_type!(String, bool, f64); From a310417ad49618913339091fc5770a6711cf0614 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 23 Dec 2025 11:00:28 -0500 Subject: [PATCH 3/9] refactor jsg-macros and wrappable --- src/rust/jsg-macros/BUILD.bazel | 1 + src/rust/jsg-macros/lib.rs | 423 +++++++++----------------------- src/rust/jsg-macros/types.rs | 5 + src/rust/jsg/lib.rs | 19 -- src/rust/jsg/wrappable.rs | 23 +- 5 files changed, 147 insertions(+), 324 deletions(-) create mode 100644 src/rust/jsg-macros/types.rs diff --git a/src/rust/jsg-macros/BUILD.bazel b/src/rust/jsg-macros/BUILD.bazel index 71318320dc7..d36c8763249 100644 --- a/src/rust/jsg-macros/BUILD.bazel +++ b/src/rust/jsg-macros/BUILD.bazel @@ -4,6 +4,7 @@ wd_rust_proc_macro( name = "jsg-macros", visibility = ["//visibility:public"], deps = [ + "@crates_vendor//:proc-macro2", "@crates_vendor//:quote", "@crates_vendor//:syn", ], diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index d9f48d0207e..3de612aa5f6 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -1,4 +1,7 @@ +mod types; + use proc_macro::TokenStream; +use proc_macro2::TokenStream as TokenStream2; use quote::ToTokens; use quote::quote; use syn::Data; @@ -10,96 +13,51 @@ use syn::ItemImpl; use syn::Type; use syn::parse_macro_input; use syn::spanned::Spanned; +use types::is_str_ref; /// Generates `jsg::Struct` and `jsg::Type` implementations for data structures. /// /// Only public fields are included in the generated JavaScript object. -/// Automatically implements `jsg::Type::class_name()` using the struct name, -/// or a custom name if provided via the `name` parameter. -/// -/// # Example -/// ```rust -/// #[jsg::struct] -/// pub struct CaaRecord { -/// pub critical: u8, -/// pub field: String, -/// } -/// -/// #[jsg::struct(name = "CustomName")] -/// pub struct MyRecord { -/// pub value: String, -/// } -/// ``` -/// -/// # Panics -/// Panics if applied to non-struct items or structs without named fields. +/// Use `name` parameter for custom JavaScript class name. #[proc_macro_attribute] pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { let input = parse_macro_input!(item as DeriveInput); let name = &input.ident; + let class_name = extract_name_attribute(&attr.to_string()).unwrap_or_else(|| name.to_string()); - let class_name = if attr.is_empty() { - name.to_string() - } else { - extract_name_attribute(&attr.to_string()).unwrap_or_else(|| name.to_string()) + let Data::Struct(data) = &input.data else { + return error(&input, "#[jsg_struct] can only be applied to structs"); }; - - let fields = match &input.data { - Data::Struct(data) => match &data.fields { - Fields::Named(fields) => &fields.named, - _ => { - return syn::Error::new_spanned( - &input, - "#[jsg::struct] only supports structs with named fields", - ) - .to_compile_error() - .into(); - } - }, - _ => { - return syn::Error::new_spanned( - &input, - "#[jsg::struct] can only be applied to structs", - ) - .to_compile_error() - .into(); - } + let Fields::Named(fields) = &data.fields else { + return error( + &input, + "#[jsg_struct] only supports structs with named fields", + ); }; - let field_assignments = fields.iter().filter_map(|field| { - let is_public = matches!(field.vis, syn::Visibility::Public(_)); - if is_public { - let field_name = &field.ident; - let field_name_str = field_name - .as_ref() - .expect("Named fields must have identifiers") - .to_string(); - Some(quote! { - let #field_name = jsg::v8::ToLocalValue::to_local(&this.#field_name, lock); - obj.set(lock, #field_name_str, #field_name); - }) - } else { - None + let field_assignments = fields.named.iter().filter_map(|field| { + if !matches!(field.vis, syn::Visibility::Public(_)) { + return None; } + let field_name = field.ident.as_ref()?; + let field_name_str = field_name.to_string(); + Some(quote! { + let #field_name = jsg::v8::ToLocalValue::to_local(&this.#field_name, lock); + obj.set(lock, #field_name_str, #field_name); + }) }); - let expanded = quote! { + quote! { #input impl jsg::Type for #name { type This = Self; - fn class_name() -> &'static str { - #class_name - } + fn class_name() -> &'static str { #class_name } fn wrap<'a, 'b>(this: Self::This, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> - where - 'b: 'a, + where 'b: 'a, { - // TODO(soon): Use a precached ObjectTemplate instance to create the object, - // similar to how C++ JSG optimizes object creation. This would avoid recreating - // the object shape on every wrap() call and improve performance. unsafe { let mut obj = lock.new_object(); #(#field_assignments)* @@ -112,43 +70,19 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { } fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { - // TODO(soon): Implement proper unwrapping for struct types unimplemented!("Struct unwrap is not yet supported") } } - impl jsg::Struct for #name { - - } - }; - - TokenStream::from(expanded) + impl jsg::Struct for #name {} + } + .into() } /// Generates FFI callback for JSG methods. /// -/// Creates a `{method_name}_callback` extern "C" function that bridges JavaScript and Rust. -/// If no name is provided, automatically converts `snake_case` to `camelCase`. -/// -/// Parameters and return values are handled via the [`jsg::Wrappable`] trait. +/// Parameters and return values are handled via `jsg::Wrappable`. /// See `jsg/wrappable.rs` for supported types. -/// -/// Use `NonCoercible` to reject values that would require JavaScript coercion. -/// Use `&str` for string parameters (internally unwrapped as `String` and borrowed). -/// -/// # Example -/// -/// ```ignore -/// #[jsg_method(name = "parseRecord")] -/// pub fn parse_record(&self, data: &str) -> Result { -/// // Errors are thrown as JavaScript exceptions -/// } -/// -/// #[jsg_method] -/// pub fn get_name(&self) -> String { -/// self.name.clone() -/// } -/// ``` #[proc_macro_attribute] pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let input_fn = parse_macro_input!(item as ItemFn); @@ -156,193 +90,93 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let fn_vis = &input_fn.vis; let fn_sig = &input_fn.sig; let fn_block = &input_fn.block; - let callback_name = syn::Ident::new(&format!("{fn_name}_callback"), fn_name.span()); let params: Vec<_> = fn_sig .inputs .iter() - .filter_map(|arg| { - if let FnArg::Typed(pat_type) = arg { - Some((&pat_type.pat, &pat_type.ty)) - } else { - None - } + .filter_map(|arg| match arg { + FnArg::Typed(pat_type) => Some(&pat_type.ty), + FnArg::Receiver(_) => None, }) .collect(); - let arg_unwraps: Vec<_> = params + let (unwraps, arg_refs): (Vec<_>, Vec<_>) = params .iter() .enumerate() - .map(|(i, (_pat, ty))| { - let arg_name = syn::Ident::new(&format!("arg{i}"), fn_name.span()); - let unwrap_code = generate_unwrap_code(&arg_name, ty, i); - (arg_name, unwrap_code) + .map(|(i, ty)| { + let arg = syn::Ident::new(&format!("arg{i}"), fn_name.span()); + let (unwrap, arg_ref) = generate_unwrap(&arg, ty, i); + (unwrap, arg_ref) }) - .collect(); - - let unwrap_statements = arg_unwraps - .iter() - .map(|(_arg_name, unwrap_code)| unwrap_code); - - let arg_refs: Vec<_> = params - .iter() - .zip(arg_unwraps.iter()) - .map(|((_, ty), (arg_name, _))| { - if is_str_reference(ty) { - quote! { &#arg_name } - } else { - quote! { #arg_name } - } - }) - .collect(); + .unzip(); - let expanded = quote! { - #fn_vis #fn_sig { - #fn_block - } + quote! { + #fn_vis #fn_sig { #fn_block } #[automatically_derived] extern "C" fn #callback_name(args: *mut jsg::v8::ffi::FunctionCallbackInfo) { let mut lock = unsafe { jsg::Lock::from_args(args) }; let mut args = unsafe { jsg::v8::FunctionCallbackInfo::from_ffi(args) }; - #(#unwrap_statements)* + #(#unwraps)* let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); let result = self_.#fn_name(#(#arg_refs),*); jsg::Wrappable::wrap_return(result, &mut lock, &mut args); } - }; - - TokenStream::from(expanded) -} - -fn is_str_reference(ty: &Type) -> bool { - match ty { - Type::Reference(type_ref) => { - matches!(&*type_ref.elem, Type::Path(type_path) if type_path.path.is_ident("str")) - } - _ => false, } + .into() } -/// Returns true if the type is `NonCoercible`. -fn is_non_coercible(ty: &Type) -> bool { - let Type::Path(type_path) = ty else { - return false; - }; - type_path - .path - .segments - .last() - .is_some_and(|seg| seg.ident == "NonCoercible") -} - -fn generate_unwrap_code( - arg_name: &syn::Ident, - ty: &Type, - index: usize, -) -> quote::__private::TokenStream { - // Check for NonCoercible types - these use their inherent unwrap method - // which validates the type and returns Option - if is_non_coercible(ty) { - return quote! { - let Some(#arg_name) = (unsafe { - <#ty>::unwrap(&mut lock, args.get(#index)) - }) else { - return; - }; +fn generate_unwrap(arg: &syn::Ident, ty: &Type, index: usize) -> (TokenStream2, TokenStream2) { + // &str: unwrap as String and borrow + if is_str_ref(ty) { + let unwrap = quote! { + let Some(#arg) = ::try_unwrap(&mut lock, args.get(#index)) else { return; }; }; + return (unwrap, quote! { &#arg }); } - // For &str references, unwrap as String and borrow - if is_str_reference(ty) { - return quote! { - let #arg_name = ::unwrap(lock.isolate(), args.get(#index)); - }; - } - - // For all other types, use Wrappable::unwrap - quote! { - let #arg_name = <#ty as jsg::Wrappable>::unwrap(lock.isolate(), args.get(#index)); - } + // All types use Wrappable::try_unwrap + let unwrap = quote! { + let Some(#arg) = <#ty as jsg::Wrappable>::try_unwrap(&mut lock, args.get(#index)) else { return; }; + }; + (unwrap, quote! { #arg }) } -/// Generates boilerplate code for JSG resources. -/// -/// Works in two contexts: -/// 1. On a struct - generates `jsg::Type`, Wrapper, and `ResourceTemplate` implementations -/// 2. On an impl block - scans for `#[jsg::method]` and generates `Resource` trait implementation -/// -/// Automatically implements `jsg::Type::class_name()` using the struct name, -/// or a custom name if provided via the `name` parameter. -/// -/// # Example -/// ```rust -/// #[jsg::resource] -/// pub struct DnsUtil { -/// pub _private: u8, -/// } -/// -/// #[jsg::resource(name = "CustomUtil")] -/// pub struct MyUtil { -/// pub _private: u8, -/// } -/// -/// #[jsg::resource] -/// impl DnsUtil { -/// #[jsg::method(name = "parseRecord")] -/// pub fn parse_record(&self, data: &str) -> Result { -/// // implementation -/// } -/// } -/// ``` +/// Generates boilerplate for JSG resources. /// -/// # Panics -/// Panics if applied to items other than structs or impl blocks. +/// On structs: generates `jsg::Type` and `ResourceTemplate`. +/// On impl blocks: generates `Resource` trait with method registrations. #[proc_macro_attribute] pub fn jsg_resource(attr: TokenStream, item: TokenStream) -> TokenStream { - // Try to parse as an impl block first if let Ok(impl_block) = syn::parse::(item.clone()) { return generate_resource_impl(&impl_block); } - // Otherwise, parse as a struct let input = parse_macro_input!(item as DeriveInput); let name = &input.ident; - - let class_name = if attr.is_empty() { - name.to_string() - } else { - extract_name_attribute(&attr.to_string()).unwrap_or_else(|| name.to_string()) - }; - + let class_name = extract_name_attribute(&attr.to_string()).unwrap_or_else(|| name.to_string()); let template_name = syn::Ident::new(&format!("{name}Template"), name.span()); - // Ensure it's a struct if !matches!(&input.data, Data::Struct(_)) { - return syn::Error::new_spanned( + return error( &input, - "#[jsg::resource] can only be applied to structs or impl blocks", - ) - .to_compile_error() - .into(); + "#[jsg_resource] can only be applied to structs or impl blocks", + ); } - let expanded = quote! { + quote! { #input #[automatically_derived] impl jsg::Type for #name { type This = jsg::Ref; - fn class_name() -> &'static str { - #class_name - } + fn class_name() -> &'static str { #class_name } fn wrap<'a, 'b>(_this: Self::This, _lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> - where - 'b: 'a, + where 'b: 'a, { todo!("Implement wrap for jsg::Resource") } @@ -352,7 +186,6 @@ pub fn jsg_resource(attr: TokenStream, item: TokenStream) -> TokenStream { } fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { - // TODO(soon): Implement proper unwrapping for resource types unimplemented!("Resource unwrap is not yet supported") } } @@ -365,104 +198,91 @@ pub fn jsg_resource(attr: TokenStream, item: TokenStream) -> TokenStream { #[automatically_derived] impl jsg::ResourceTemplate for #template_name { fn new(lock: &mut jsg::Lock) -> Self { - Self { - constructor: jsg::create_resource_constructor::<#name>(lock), - } + Self { constructor: jsg::create_resource_constructor::<#name>(lock) } } fn get_constructor(&self) -> &jsg::v8::Global { &self.constructor } } - }; - - TokenStream::from(expanded) + } + .into() } fn generate_resource_impl(impl_block: &ItemImpl) -> TokenStream { let self_ty = &impl_block.self_ty; - let mut method_registrations = vec![]; - - for item in &impl_block.items { - if let syn::ImplItem::Fn(method) = item { - for attr in &method.attrs { - // TODO: More reliable way to detect jsg_method attribute - if attr.path().is_ident("jsg") - || (attr.path().segments.len() == 1 - && attr.path().segments[0].ident == "jsg_method") - { - let rust_method_name = &method.sig.ident; - - let attr_str = attr.meta.to_token_stream().to_string(); - let js_name = extract_name_attribute(&attr_str) - .unwrap_or_else(|| snake_to_camel_case(&rust_method_name.to_string())); - - let callback_name = syn::Ident::new( - &format!("{rust_method_name}_callback"), - rust_method_name.span(), - ); - - method_registrations.push(quote! { - jsg::Member::Method { - name: #js_name.to_owned(), - callback: Self::#callback_name, - } - }); - } - } - } - } - // Create a unique drop callback function name based on the type + let method_registrations: Vec<_> = impl_block + .items + .iter() + .filter_map(|item| { + let syn::ImplItem::Fn(method) = item else { + return None; + }; + let attr = method.attrs.iter().find(|a| { + a.path().is_ident("jsg") + || a.path() + .segments + .last() + .is_some_and(|s| s.ident == "jsg_method") + })?; + + let rust_name = &method.sig.ident; + let js_name = extract_name_attribute(&attr.meta.to_token_stream().to_string()) + .unwrap_or_else(|| snake_to_camel(&rust_name.to_string())); + let callback = syn::Ident::new(&format!("{rust_name}_callback"), rust_name.span()); + + Some(quote! { + jsg::Member::Method { name: #js_name.to_owned(), callback: Self::#callback } + }) + }) + .collect(); + let type_name = match &**self_ty { - syn::Type::Path(type_path) => type_path + syn::Type::Path(p) => p .path .segments .last() - .map_or_else(|| "Unknown".to_owned(), |seg| seg.ident.to_string()), - _ => "Unknown".to_owned(), + .map_or("Unknown", |s| s.ident.to_string().leak()), + _ => "Unknown", }; - let drop_callback_name = - syn::Ident::new(&format!("drop_{type_name}"), impl_block.self_ty.span()); + let drop_fn = syn::Ident::new(&format!("drop_{type_name}"), self_ty.span()); - let expanded = quote! { + quote! { #impl_block #[allow(non_snake_case)] #[automatically_derived] - unsafe extern "C" fn #drop_callback_name(isolate: *mut jsg::v8::ffi::Isolate, this: *mut std::os::raw::c_void) { + unsafe extern "C" fn #drop_fn(isolate: *mut jsg::v8::ffi::Isolate, this: *mut std::os::raw::c_void) { jsg::drop_resource::<#self_ty>(isolate, this); } #[automatically_derived] impl jsg::Resource for #self_ty { - fn members() -> Vec - where - Self: Sized, - { - vec![ - #(#method_registrations,)* - ] + fn members() -> Vec where Self: Sized { + vec![#(#method_registrations,)*] } fn get_drop_fn(&self) -> unsafe extern "C" fn(*mut jsg::v8::ffi::Isolate, *mut std::os::raw::c_void) { - #drop_callback_name + #drop_fn } fn get_state(&mut self) -> &mut jsg::ResourceState { &mut self._state } } - }; + } + .into() +} - TokenStream::from(expanded) +fn error(tokens: &impl ToTokens, msg: &str) -> TokenStream { + syn::Error::new_spanned(tokens, msg) + .to_compile_error() + .into() } fn extract_name_attribute(attr_str: &str) -> Option { - if !attr_str.contains("name") { - return None; - } - + attr_str.find("name")?.checked_add(0)?; attr_str .split('=') .nth(1)? @@ -473,22 +293,19 @@ fn extract_name_attribute(attr_str: &str) -> Option { .map(str::to_owned) } -fn snake_to_camel_case(s: &str) -> String { +fn snake_to_camel(s: &str) -> String { let mut result = String::new(); - let mut capitalize_next = false; - - for (i, ch) in s.chars().enumerate() { - if ch == '_' { - capitalize_next = true; - } else if i == 0 { - result.push(ch); - } else if capitalize_next { - result.push(ch.to_ascii_uppercase()); - capitalize_next = false; - } else { - result.push(ch); + let mut cap_next = false; + for (i, c) in s.chars().enumerate() { + match c { + '_' => cap_next = true, + _ if i == 0 => result.push(c), + _ if cap_next => { + result.push(c.to_ascii_uppercase()); + cap_next = false; + } + _ => result.push(c), } } - result } diff --git a/src/rust/jsg-macros/types.rs b/src/rust/jsg-macros/types.rs new file mode 100644 index 00000000000..e4e4cbe7668 --- /dev/null +++ b/src/rust/jsg-macros/types.rs @@ -0,0 +1,5 @@ +use syn::Type; + +pub fn is_str_ref(ty: &Type) -> bool { + matches!(ty, Type::Reference(r) if matches!(&*r.elem, Type::Path(p) if p.path.is_ident("str"))) +} diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index 438735155d7..8743bc458fa 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -230,25 +230,6 @@ impl NonCoercible { pub fn new(value: T) -> Self { Self { value } } - - /// Unwraps a V8 value into `NonCoercible`, throwing a JavaScript error if the value - /// is not exactly the expected type. - /// - /// Returns `Some(NonCoercible)` if the value is the exact type, or `None` if an error - /// was thrown (in which case the caller should return early). - /// - /// # Safety - /// The caller must ensure `lock` is valid and `value` is a valid V8 local handle. - pub unsafe fn unwrap(lock: &mut Lock, value: v8::Local) -> Option { - if !T::is_exact(&value) { - let type_name = T::class_name(); - let error_msg = format!("Expected a {} value but got {}", type_name, value.type_of()); - unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &error_msg) }; - return None; - } - let inner = T::unwrap(lock.isolate(), value); - Some(Self::new(inner)) - } } impl From for NonCoercible { diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 495e8e3a280..00a8438f5d1 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -98,8 +98,8 @@ impl Type for f64 { /// Trait for converting between Rust and JavaScript values. /// /// Provides bidirectional conversion: `wrap` converts Rust to JavaScript, -/// `unwrap` converts JavaScript to Rust. The `wrap_return` method is a -/// convenience for setting function return values. +/// `unwrap` converts JavaScript to Rust. The `try_unwrap` method is used +/// by macros to unwrap parameters with proper error handling. pub trait Wrappable: Sized { /// Converts this Rust value into a JavaScript value. fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value>; @@ -107,6 +107,12 @@ pub trait Wrappable: Sized { /// Converts a JavaScript value into this Rust type. fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self; + /// Attempts to unwrap a JavaScript value, returning None and throwing a JS error on failure. + /// Most types simply delegate to `unwrap`, but `NonCoercible` validates the type first. + fn try_unwrap(lock: &mut Lock, value: v8::Local) -> Option { + Some(Self::unwrap(lock.isolate(), value)) + } + /// Wraps this value and sets it as the function return value. fn wrap_return(self, lock: &mut Lock, args: &mut v8::FunctionCallbackInfo) { args.set_return_value(self.wrap(lock)); @@ -170,6 +176,19 @@ impl> Wrappable for NonCoercible { fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { Self::new(T::unwrap(isolate, value)) } + + fn try_unwrap(lock: &mut Lock, value: v8::Local) -> Option { + if !T::is_exact(&value) { + let error_msg = format!( + "Expected a {} value but got {}", + T::class_name(), + value.type_of() + ); + unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &error_msg) }; + return None; + } + Some(Self::new(T::unwrap(lock.isolate(), value))) + } } impl> Wrappable for T { From 9aaf6ade0934a39e708792d40015f42d462493eb Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 23 Dec 2025 11:05:03 -0500 Subject: [PATCH 4/9] add todo --- src/rust/jsg-macros/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 3de612aa5f6..0f8736808b9 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -58,6 +58,9 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { fn wrap<'a, 'b>(this: Self::This, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> where 'b: 'a, { + // TODO(soon): Use a precached ObjectTemplate instance to create the object, + // similar to how C++ JSG optimizes object creation. This would avoid recreating + // the object shape on every wrap() call and improve performance. unsafe { let mut obj = lock.new_object(); #(#field_assignments)* From 7a279363776918ed202bad085ba93486e4ec8a59 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 2 Jan 2026 15:29:38 -0500 Subject: [PATCH 5/9] remove &str special handling --- src/rust/api/dns.rs | 16 +- src/rust/jsg-macros/BUILD.bazel | 1 - src/rust/jsg-macros/README.md | 6 +- src/rust/jsg-macros/lib.rs | 74 +++--- src/rust/jsg-macros/types.rs | 5 - src/rust/jsg-test/tests/jsg_struct.rs | 8 +- src/rust/jsg-test/tests/resource_callback.rs | 2 +- src/rust/jsg/lib.rs | 26 ++- src/rust/jsg/wrappable.rs | 226 ++++++++----------- 9 files changed, 158 insertions(+), 206 deletions(-) delete mode 100644 src/rust/jsg-macros/types.rs diff --git a/src/rust/api/dns.rs b/src/rust/api/dns.rs index 5897ca09daa..2005654bb44 100644 --- a/src/rust/api/dns.rs +++ b/src/rust/api/dns.rs @@ -136,7 +136,7 @@ impl DnsUtil { /// `DnsParserError::InvalidHexString` /// `DnsParserError::ParseIntError` #[jsg_method] - pub fn parse_caa_record(&self, record: &str) -> Result { + pub fn parse_caa_record(&self, record: String) -> Result { // Let's remove "\\#" and the length of data from the beginning of the record let data = record.split_ascii_whitespace().collect::>()[2..].to_vec(); let critical = data[0].parse::()?; @@ -191,7 +191,7 @@ impl DnsUtil { /// `DnsParserError::InvalidHexString` /// `DnsParserError::ParseIntError` #[jsg_method] - pub fn parse_naptr_record(&self, record: &str) -> jsg::Result { + pub fn parse_naptr_record(&self, record: String) -> jsg::Result { let data = record.split_ascii_whitespace().collect::>()[1..].to_vec(); let order_str = data[1..3].to_vec(); @@ -262,7 +262,7 @@ mod tests { _state: ResourceState::default(), }; let record = dns_util - .parse_caa_record("\\# 15 00 05 69 73 73 75 65 70 6b 69 2e 67 6f 6f 67") + .parse_caa_record("\\# 15 00 05 69 73 73 75 65 70 6b 69 2e 67 6f 6f 67".to_owned()) .unwrap(); assert_eq!(record.critical, 0); @@ -277,7 +277,8 @@ mod tests { }; let record = dns_util .parse_caa_record( - "\\# 21 00 09 69 73 73 75 65 77 69 6c 64 6c 65 74 73 65 6e 63 72 79 70 74", + "\\# 21 00 09 69 73 73 75 65 77 69 6c 64 6c 65 74 73 65 6e 63 72 79 70 74" + .to_owned(), ) .unwrap(); @@ -291,8 +292,9 @@ mod tests { let dns_util = DnsUtil { _state: ResourceState::default(), }; - let result = - dns_util.parse_caa_record("\\# 15 00 05 69 6e 76 61 6c 69 64 70 6b 69 2e 67 6f 6f 67"); + let result = dns_util.parse_caa_record( + "\\# 15 00 05 69 6e 76 61 6c 69 64 70 6b 69 2e 67 6f 6f 67".to_owned(), + ); assert!(result.is_err()); } @@ -303,7 +305,7 @@ mod tests { _state: ResourceState::default(), }; let record = dns_util - .parse_naptr_record("\\# 37 15 b3 08 ae 01 73 0a 6d 79 2d 73 65 72 76 69 63 65 06 72 65 67 65 78 70 0b 72 65 70 6c 61 63 65 6d 65 6e 74 00") + .parse_naptr_record("\\# 37 15 b3 08 ae 01 73 0a 6d 79 2d 73 65 72 76 69 63 65 06 72 65 67 65 78 70 0b 72 65 70 6c 61 63 65 6d 65 6e 74 00".to_owned()) .unwrap(); assert_eq!(record.flags, "s"); diff --git a/src/rust/jsg-macros/BUILD.bazel b/src/rust/jsg-macros/BUILD.bazel index d36c8763249..71318320dc7 100644 --- a/src/rust/jsg-macros/BUILD.bazel +++ b/src/rust/jsg-macros/BUILD.bazel @@ -4,7 +4,6 @@ wd_rust_proc_macro( name = "jsg-macros", visibility = ["//visibility:public"], deps = [ - "@crates_vendor//:proc-macro2", "@crates_vendor//:quote", "@crates_vendor//:syn", ], diff --git a/src/rust/jsg-macros/README.md b/src/rust/jsg-macros/README.md index 3acfe2ee93c..c4b1d980513 100644 --- a/src/rust/jsg-macros/README.md +++ b/src/rust/jsg-macros/README.md @@ -9,7 +9,7 @@ Generates the `jsg::Struct` and `jsg::Type` implementations for data structures. ```rust #[jsg_struct] pub struct CaaRecord { - pub critical: u8, + pub critical: f64, pub field: String, pub value: String, } @@ -29,7 +29,7 @@ Parameters and return values are handled via the `jsg::Wrappable` trait. Any typ ```rust impl DnsUtil { #[jsg_method(name = "parseCaaRecord")] - pub fn parse_caa_record(&self, record: &str) -> Result { + pub fn parse_caa_record(&self, record: String) -> Result { // Errors are thrown as JavaScript exceptions } @@ -65,7 +65,7 @@ pub struct MyUtil { #[jsg_resource] impl DnsUtil { #[jsg_method] - pub fn parse_caa_record(&self, record: &str) -> Result { + pub fn parse_caa_record(&self, record: String) -> Result { // implementation } } diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 0f8736808b9..4525cc6cd05 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -1,7 +1,4 @@ -mod types; - use proc_macro::TokenStream; -use proc_macro2::TokenStream as TokenStream2; use quote::ToTokens; use quote::quote; use syn::Data; @@ -10,10 +7,8 @@ use syn::Fields; use syn::FnArg; use syn::ItemFn; use syn::ItemImpl; -use syn::Type; use syn::parse_macro_input; use syn::spanned::Spanned; -use types::is_str_ref; /// Generates `jsg::Struct` and `jsg::Type` implementations for data structures. /// @@ -51,27 +46,31 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { #input impl jsg::Type for #name { - type This = Self; - fn class_name() -> &'static str { #class_name } - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> - where 'b: 'a, + fn is_exact(value: &jsg::v8::Local) -> bool { + value.is_object() + } + } + + impl jsg::Wrappable for #name { + fn wrap<'a, 'b>(self, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> + where + 'b: 'a, { // TODO(soon): Use a precached ObjectTemplate instance to create the object, // similar to how C++ JSG optimizes object creation. This would avoid recreating // the object shape on every wrap() call and improve performance. unsafe { + let this = self; let mut obj = lock.new_object(); #(#field_assignments)* obj.into() } } + } - fn is_exact(value: &jsg::v8::Local) -> bool { - value.is_object() - } - + impl jsg::Unwrappable for #name { fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { unimplemented!("Struct unwrap is not yet supported") } @@ -104,13 +103,15 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { }) .collect(); - let (unwraps, arg_refs): (Vec<_>, Vec<_>) = params + let (unwraps, arg_names): (Vec<_>, Vec<_>) = params .iter() .enumerate() .map(|(i, ty)| { let arg = syn::Ident::new(&format!("arg{i}"), fn_name.span()); - let (unwrap, arg_ref) = generate_unwrap(&arg, ty, i); - (unwrap, arg_ref) + let unwrap = quote! { + let Some(#arg) = <#ty as jsg::Unwrappable>::try_unwrap(&mut lock, args.get(#i)) else { return; }; + }; + (unwrap, arg) }) .unzip(); @@ -124,29 +125,13 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { #(#unwraps)* let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); - let result = self_.#fn_name(#(#arg_refs),*); - jsg::Wrappable::wrap_return(result, &mut lock, &mut args); + let result = self_.#fn_name(#(#arg_names),*); + args.set_return_value(jsg::Wrappable::wrap(result, &mut lock)); } } .into() } -fn generate_unwrap(arg: &syn::Ident, ty: &Type, index: usize) -> (TokenStream2, TokenStream2) { - // &str: unwrap as String and borrow - if is_str_ref(ty) { - let unwrap = quote! { - let Some(#arg) = ::try_unwrap(&mut lock, args.get(#index)) else { return; }; - }; - return (unwrap, quote! { &#arg }); - } - - // All types use Wrappable::try_unwrap - let unwrap = quote! { - let Some(#arg) = <#ty as jsg::Wrappable>::try_unwrap(&mut lock, args.get(#index)) else { return; }; - }; - (unwrap, quote! { #arg }) -} - /// Generates boilerplate for JSG resources. /// /// On structs: generates `jsg::Type` and `ResourceTemplate`. @@ -174,20 +159,25 @@ pub fn jsg_resource(attr: TokenStream, item: TokenStream) -> TokenStream { #[automatically_derived] impl jsg::Type for #name { - type This = jsg::Ref; - fn class_name() -> &'static str { #class_name } - fn wrap<'a, 'b>(_this: Self::This, _lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> - where 'b: 'a, - { - todo!("Implement wrap for jsg::Resource") - } - fn is_exact(value: &jsg::v8::Local) -> bool { value.is_object() } + } + #[automatically_derived] + impl jsg::Wrappable for #name { + fn wrap<'a, 'b>(self, _lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> + where + 'b: 'a, + { + unimplemented!("Resource wrap is not yet supported") + } + } + + #[automatically_derived] + impl jsg::Unwrappable for #name { fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { unimplemented!("Resource unwrap is not yet supported") } diff --git a/src/rust/jsg-macros/types.rs b/src/rust/jsg-macros/types.rs deleted file mode 100644 index e4e4cbe7668..00000000000 --- a/src/rust/jsg-macros/types.rs +++ /dev/null @@ -1,5 +0,0 @@ -use syn::Type; - -pub fn is_str_ref(ty: &Type) -> bool { - matches!(ty, Type::Reference(r) if matches!(&*r.elem, Type::Path(p) if p.path.is_ident("str"))) -} diff --git a/src/rust/jsg-test/tests/jsg_struct.rs b/src/rust/jsg-test/tests/jsg_struct.rs index 5c3d95c0d37..6f8945044d3 100644 --- a/src/rust/jsg-test/tests/jsg_struct.rs +++ b/src/rust/jsg-test/tests/jsg_struct.rs @@ -1,7 +1,7 @@ use jsg::Error; use jsg::ExceptionType; use jsg::Lock; -use jsg::Type; +use jsg::Wrappable; use jsg::v8; use jsg::v8::ToLocalValue; use jsg_macros::jsg_struct; @@ -31,7 +31,7 @@ fn objects_can_be_wrapped_and_unwrapped() { let instance = TestStruct { str: "test".to_owned(), }; - let wrapped = TestStruct::wrap(instance, &mut lock); + let wrapped = instance.wrap(&mut lock); let mut obj: v8::Local<'_, v8::Object> = wrapped.into(); assert!(obj.has(&mut lock, "str")); let str_value = obj.get(&mut lock, "str"); @@ -54,7 +54,7 @@ fn struct_with_multiple_properties() { age: 30, active: "true".to_owned(), }; - let wrapped = MultiPropertyStruct::wrap(instance, &mut lock); + let wrapped = instance.wrap(&mut lock); let obj: v8::Local<'_, v8::Object> = wrapped.into(); assert!(obj.has(&mut lock, "name")); @@ -142,7 +142,7 @@ fn nested_object_properties() { let inner_instance = NestedStruct { inner: "nested value".to_owned(), }; - let inner_wrapped = NestedStruct::wrap(inner_instance, &mut lock); + let inner_wrapped = inner_instance.wrap(&mut lock); outer.set(&mut lock, "nested", inner_wrapped); assert!(outer.has(&mut lock, "nested")); diff --git a/src/rust/jsg-test/tests/resource_callback.rs b/src/rust/jsg-test/tests/resource_callback.rs index 808c1deb0b0..4cbc4b000cb 100644 --- a/src/rust/jsg-test/tests/resource_callback.rs +++ b/src/rust/jsg-test/tests/resource_callback.rs @@ -26,7 +26,7 @@ struct EchoResource { #[expect(clippy::unnecessary_wraps)] impl EchoResource { #[jsg_method] - pub fn echo(&self, message: &str) -> Result { + pub fn echo(&self, message: String) -> Result { Ok(format!("{}{}", self.prefix, message)) } } diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index 8743bc458fa..838b014c279 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -14,6 +14,7 @@ pub mod v8; mod wrappable; pub use v8::ffi::ExceptionType; +pub use wrappable::Unwrappable; pub use wrappable::Wrappable; #[cxx::bridge(namespace = "workerd::rust::jsg")] @@ -230,6 +231,11 @@ impl NonCoercible { pub fn new(value: T) -> Self { Self { value } } + + /// Consumes the wrapper and returns the inner value. + pub fn into_inner(self) -> T { + self.value + } } impl From for NonCoercible { @@ -366,32 +372,30 @@ impl Clone for Ref { } } +/// Provides metadata about Rust types exposed to JavaScript. +/// +/// This trait provides type information used for error messages, memory tracking, +/// and type validation (for `NonCoercible`). The actual conversion logic is in +/// `Wrappable` (Rust → JS) and `Unwrappable` (JS → Rust). +/// /// TODO: Implement `memory_info(jsg::MemoryTracker)` pub trait Type: Sized { - /// The input type for [`wrap()`](Self::wrap). For primitive types this is typically `Self`, - /// but resource types may use `Ref` or other wrapper types. - type This; - + /// The JavaScript class name for this type (used in error messages). fn class_name() -> &'static str; + /// Same as jsgGetMemoryName fn memory_name() -> &'static str { std::any::type_name::() } + /// Same as jsgGetMemorySelfSize fn memory_self_size() -> usize { std::mem::size_of::() } - /// Wraps this struct as a JavaScript value by deep-copying its fields. - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a; /// Returns true if the V8 value is exactly this type (no coercion). /// Used by `NonCoercible` to reject values that would require coercion. fn is_exact(value: &v8::Local) -> bool; - - /// Unwraps a V8 value into this type without coercion. - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self; } pub enum Member { diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 00a8438f5d1..cfda81ee0ab 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -2,7 +2,7 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -//! Trait for converting between Rust and JavaScript values. +//! Traits for converting between Rust and JavaScript values. //! //! # Supported Types //! @@ -12,98 +12,42 @@ //! | `String` | `string` | //! | `bool` | `boolean` | //! | `f64` | `number` | -//! | `Option` | `T` or `null` | +//! | `Option` | `T` or `null`/`undefined` | //! | `Result` | `T` or throws | -//! | `NonCoercible` | `T` | +//! | `NonCoercible` | `T` (strict type checking) | //! | `T: Struct` | `object` | use std::fmt::Display; use crate::Lock; use crate::NonCoercible; -use crate::Struct; use crate::Type; use crate::v8; use crate::v8::ToLocalValue; -impl Type for String { - type This = Self; +// ============================================================================= +// Wrappable trait (Rust → JavaScript) +// ============================================================================= - fn class_name() -> &'static str { - "string" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> +/// Trait for converting Rust values to JavaScript. +/// +/// Provides Rust → JavaScript conversion. +pub trait Wrappable: Sized { + /// Converts this Rust value into a JavaScript value. + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_string() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_string(isolate.as_ffi(), value.into_ffi()) } - } + 'b: 'a; } -impl Type for bool { - type This = Self; +// ============================================================================= +// Unwrappable trait (JavaScript → Rust) +// ============================================================================= - fn class_name() -> &'static str { - "boolean" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_boolean() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_boolean(isolate.as_ffi(), value.into_ffi()) } - } -} - -impl Type for f64 { - type This = Self; - - fn class_name() -> &'static str { - "number" - } - - fn wrap<'a, 'b>(this: Self::This, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - this.to_local(lock) - } - - fn is_exact(value: &v8::Local) -> bool { - value.is_number() - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::unwrap_number(isolate.as_ffi(), value.into_ffi()) } - } -} - -/// Trait for converting between Rust and JavaScript values. +/// Trait for converting JavaScript values to Rust. /// -/// Provides bidirectional conversion: `wrap` converts Rust to JavaScript, -/// `unwrap` converts JavaScript to Rust. The `try_unwrap` method is used -/// by macros to unwrap parameters with proper error handling. -pub trait Wrappable: Sized { - /// Converts this Rust value into a JavaScript value. - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value>; - +/// Provides JS → Rust conversion. The `try_unwrap` method is used by macros +/// to unwrap function parameters with proper error handling. +pub trait Unwrappable: Sized { /// Converts a JavaScript value into this Rust type. fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self; @@ -112,55 +56,107 @@ pub trait Wrappable: Sized { fn try_unwrap(lock: &mut Lock, value: v8::Local) -> Option { Some(Self::unwrap(lock.isolate(), value)) } +} - /// Wraps this value and sets it as the function return value. - fn wrap_return(self, lock: &mut Lock, args: &mut v8::FunctionCallbackInfo) { - args.set_return_value(self.wrap(lock)); - } +// ============================================================================= +// Primitive type implementations +// ============================================================================= + +/// Implements `Type`, `Wrappable`, and `Unwrappable` for primitive types. +macro_rules! impl_primitive { + { $type:ty, $class_name:literal, $is_exact:ident, $unwrap_fn:ident } => { + impl Type for $type { + fn class_name() -> &'static str { + $class_name + } + + fn is_exact(value: &v8::Local) -> bool { + value.$is_exact() + } + } + + impl Wrappable for $type { + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + self.to_local(lock) + } + } + + impl Unwrappable for $type { + fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { + unsafe { v8::ffi::$unwrap_fn(isolate.as_ffi(), value.into_ffi()) } + } + } + }; } +impl_primitive!(String, "string", is_string, unwrap_string); +impl_primitive!(bool, "boolean", is_boolean, unwrap_boolean); +impl_primitive!(f64, "number", is_number, unwrap_number); + +// ============================================================================= +// Wrapper type implementations +// ============================================================================= + impl Wrappable for () { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { v8::Local::::undefined(lock) } - - fn unwrap(_isolate: v8::IsolatePtr, _value: v8::Local) -> Self {} } impl Wrappable for Result { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { match self { Ok(value) => value.wrap(lock), - Err(_) => unreachable!("Cannot wrap Result::Err"), - } - } - - fn unwrap(_isolate: v8::IsolatePtr, _value: v8::Local) -> Self { - unreachable!("Cannot unwrap into Result") - } - - fn wrap_return(self, lock: &mut Lock, args: &mut v8::FunctionCallbackInfo) { - match self { - Ok(value) => value.wrap_return(lock, args), Err(err) => { // TODO(soon): Use jsg::Error trait to dynamically call proper method to throw the error. let description = err.to_string(); unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &description) }; + v8::Local::::undefined(lock) } } } } impl Wrappable for Option { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { match self { Some(value) => value.wrap(lock), None => v8::Local::::null(lock), } } +} + +impl Wrappable for NonCoercible { + fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + self.into_inner().wrap(lock) + } +} +impl Unwrappable for Option { fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - if value.is_null_or_undefined() { + if value.is_null() { + None + } else if value.is_undefined() { + let error_msg = format!( + "Expected a null or {} value but got undefined", + T::class_name() + ); + unsafe { v8::ffi::isolate_throw_error(isolate.as_ffi(), &error_msg) }; None } else { Some(T::unwrap(isolate, value)) @@ -168,11 +164,7 @@ impl Wrappable for Option { } } -impl> Wrappable for NonCoercible { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { - T::wrap(self.value, lock) - } - +impl Unwrappable for NonCoercible { fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { Self::new(T::unwrap(isolate, value)) } @@ -190,33 +182,3 @@ impl> Wrappable for NonCoercible { Some(Self::new(T::unwrap(lock.isolate(), value))) } } - -impl> Wrappable for T { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { - ::wrap(self, lock) - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - ::unwrap(isolate, value) - } -} - -/// Implements `Wrappable` for types that already implement `Type`. -macro_rules! impl_wrappable_for_type { - ($($t:ty),*) => { - $( - impl Wrappable for $t { - fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { - ::wrap(self, lock) - } - - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - ::unwrap(isolate, value) - } - } - )* - }; -} - -// When adding new primitive types that implement `Type`, add them here. -impl_wrappable_for_type!(String, bool, f64); From 043d7ab51163647d38c84e360a4c2a1c00a6d0b8 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 6 Jan 2026 09:47:30 -0500 Subject: [PATCH 6/9] address pr reviews --- src/rust/api/dns.rs | 8 +- src/rust/api/lib.rs | 10 +- src/rust/jsg-macros/README.md | 6 +- src/rust/jsg-macros/lib.rs | 32 ++- src/rust/jsg-test/ffi.c++ | 41 +-- src/rust/jsg-test/ffi.h | 7 +- src/rust/jsg-test/lib.rs | 97 ++++++- src/rust/jsg-test/tests/eval.rs | 266 +++++-------------- src/rust/jsg-test/tests/jsg_struct.rs | 118 ++++---- src/rust/jsg-test/tests/non_coercible.rs | 67 ++--- src/rust/jsg-test/tests/resource_callback.rs | 147 ++++------ src/rust/jsg-test/tests/unwrap.rs | 89 +++---- src/rust/jsg/ffi.c++ | 4 + src/rust/jsg/ffi.h | 1 + src/rust/jsg/lib.rs | 166 ++++++++++-- src/rust/jsg/v8.rs | 7 + src/rust/jsg/wrappable.rs | 141 ++++++---- 17 files changed, 624 insertions(+), 583 deletions(-) diff --git a/src/rust/api/dns.rs b/src/rust/api/dns.rs index 2005654bb44..1a33ba3d0e5 100644 --- a/src/rust/api/dns.rs +++ b/src/rust/api/dns.rs @@ -18,7 +18,13 @@ pub enum DnsParserError { impl From for jsg::Error { fn from(val: DnsParserError) -> Self { - Self::new(jsg::ExceptionType::Error, val.to_string()) + match val { + DnsParserError::InvalidHexString(msg) | DnsParserError::InvalidDnsResponse(msg) => { + Self::new("Error", msg) + } + DnsParserError::ParseIntError(msg) => Self::new("RangeError", msg.to_string()), + DnsParserError::Unknown => Self::new("Error", "Unknown dns parser error".to_owned()), + } } } diff --git a/src/rust/api/lib.rs b/src/rust/api/lib.rs index 95e729e5e86..86e7e61f5c0 100644 --- a/src/rust/api/lib.rs +++ b/src/rust/api/lib.rs @@ -48,17 +48,17 @@ mod tests { #[test] fn test_wrap_resource_equality() { let harness = Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = jsg::Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, _ctx| unsafe { let dns_util = jsg::Ref::new(DnsUtil { _state: ResourceState::default(), }); - let mut dns_util_template = DnsUtilTemplate::new(&mut lock); + let mut dns_util_template = DnsUtilTemplate::new(lock); - let lhs = jsg::wrap_resource(&mut lock, dns_util.clone(), &mut dns_util_template); - let rhs = jsg::wrap_resource(&mut lock, dns_util, &mut dns_util_template); + let lhs = jsg::wrap_resource(lock, dns_util.clone(), &mut dns_util_template); + let rhs = jsg::wrap_resource(lock, dns_util, &mut dns_util_template); assert_eq!(lhs, rhs); + Ok(()) }); } } diff --git a/src/rust/jsg-macros/README.md b/src/rust/jsg-macros/README.md index c4b1d980513..ba0cef92508 100644 --- a/src/rust/jsg-macros/README.md +++ b/src/rust/jsg-macros/README.md @@ -24,7 +24,11 @@ pub struct MyRecord { Generates FFI callback functions for JSG resource methods. The `name` parameter is optional and defaults to converting the method name from `snake_case` to `camelCase`. -Parameters and return values are handled via the `jsg::Wrappable` trait. Any type implementing `Wrappable` can be used as a parameter or return value. Use `NonCoercible` to reject values that would require JavaScript coercion. +Parameters and return values are handled via the `jsg::Wrappable` trait. Any type implementing `Wrappable` can be used as a parameter or return value: + +- `Option` - accepts `T` or `undefined`, rejects `null` +- `Nullable` - accepts `T`, `null`, or `undefined` +- `NonCoercible` - rejects values that would require JavaScript coercion ```rust impl DnsUtil { diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 4525cc6cd05..89801940760 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -53,8 +53,8 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { } } - impl jsg::Wrappable for #name { - fn wrap<'a, 'b>(self, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> + impl jsg::ToJS for #name { + fn to_js<'a, 'b>(self, lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> where 'b: 'a, { @@ -70,9 +70,11 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { } } - impl jsg::Unwrappable for #name { - fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { - unimplemented!("Struct unwrap is not yet supported") + impl jsg::FromJS for #name { + type ResultType = Self; + + fn from_js(_lock: &mut jsg::Lock, _value: jsg::v8::Local) -> Result { + todo!("Struct from_js is not yet supported") } } @@ -83,7 +85,7 @@ pub fn jsg_struct(attr: TokenStream, item: TokenStream) -> TokenStream { /// Generates FFI callback for JSG methods. /// -/// Parameters and return values are handled via `jsg::Wrappable`. +/// Parameters and return values are handled via `jsg::FromJS`. /// See `jsg/wrappable.rs` for supported types. #[proc_macro_attribute] pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { @@ -109,7 +111,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { .map(|(i, ty)| { let arg = syn::Ident::new(&format!("arg{i}"), fn_name.span()); let unwrap = quote! { - let Some(#arg) = <#ty as jsg::Unwrappable>::try_unwrap(&mut lock, args.get(#i)) else { return; }; + let Ok(#arg) = <#ty as jsg::FromJS>::from_js(&mut lock, args.get(#i)) else { return; }; }; (unwrap, arg) }) @@ -126,7 +128,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); let result = self_.#fn_name(#(#arg_names),*); - args.set_return_value(jsg::Wrappable::wrap(result, &mut lock)); + args.set_return_value(jsg::ToJS::to_js(result, &mut lock)); } } .into() @@ -167,19 +169,21 @@ pub fn jsg_resource(attr: TokenStream, item: TokenStream) -> TokenStream { } #[automatically_derived] - impl jsg::Wrappable for #name { - fn wrap<'a, 'b>(self, _lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> + impl jsg::ToJS for #name { + fn to_js<'a, 'b>(self, _lock: &'a mut jsg::Lock) -> jsg::v8::Local<'b, jsg::v8::Value> where 'b: 'a, { - unimplemented!("Resource wrap is not yet supported") + todo!("Resource to_js is not yet supported") } } #[automatically_derived] - impl jsg::Unwrappable for #name { - fn unwrap(_isolate: jsg::v8::IsolatePtr, _value: jsg::v8::Local) -> Self { - unimplemented!("Resource unwrap is not yet supported") + impl jsg::FromJS for #name { + type ResultType = jsg::Ref; + + fn from_js(_lock: &mut jsg::Lock, _value: jsg::v8::Local) -> Result { + todo!("Resource from_js is not yet supported") } } diff --git a/src/rust/jsg-test/ffi.c++ b/src/rust/jsg-test/ffi.c++ index 3ebd9053527..a65016fcfe1 100644 --- a/src/rust/jsg-test/ffi.c++ +++ b/src/rust/jsg-test/ffi.c++ @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -68,59 +69,33 @@ EvalResult EvalContext::eval(::rust::Str code) const { v8Isolate, code.data(), v8::NewStringType::kNormal, static_cast(code.size()))); v8::Local script; - if (!v8::Script::Compile(ctx, source).ToLocal(&script)) { - result.success = false; - result.result_type = "CompileError"; - result.result_value = "Failed to compile script"; - return result; - } - + KJ_ASSERT(v8::Script::Compile(ctx, source).ToLocal(&script), "Failed to compile script"); v8::TryCatch catcher(v8Isolate); v8::Local value; if (script->Run(ctx).ToLocal(&value)) { - v8::String::Utf8Value type(v8Isolate, value->TypeOf(v8Isolate)); - v8::String::Utf8Value valueStr(v8Isolate, value); - result.success = true; - result.result_type = *type; - result.result_value = *valueStr; + result.value = ::workerd::rust::jsg::to_ffi(kj::mv(value)); } else if (catcher.HasCaught()) { - v8::String::Utf8Value message(v8Isolate, catcher.Exception()); - result.success = false; - result.result_type = "throws"; - result.result_value = *message ? *message : "Unknown error"; + auto exception = catcher.Exception(); + result.value = ::workerd::rust::jsg::to_ffi(kj::mv(exception)); } else { result.success = false; - result.result_type = "error"; - result.result_value = "Returned empty handle but didn't throw exception"; } return result; } -void TestHarness::run_in_context(::rust::Fn callback) const { +void TestHarness::run_in_context( + size_t data, ::rust::Fn callback) const { isolate->runInLockScope([&](TestIsolate::Lock& lock) { auto context = lock.newContext(); v8::Local v8Context = context.getHandle(lock.v8Isolate); v8::Context::Scope contextScope(v8Context); EvalContext evalContext(lock.v8Isolate, v8Context); - callback(lock.v8Isolate, evalContext); - }); -} - -void TestHarness::set_global(::rust::Str name, ::workerd::rust::jsg::Local value) const { - isolate->runInLockScope([&](TestIsolate::Lock& lock) { - auto context = lock.newContext(); - v8::Local v8Context = context.getHandle(lock.v8Isolate); - v8::Context::Scope contextScope(v8Context); - - v8::Local key = ::workerd::jsg::check(v8::String::NewFromUtf8( - lock.v8Isolate, name.data(), v8::NewStringType::kNormal, static_cast(name.size()))); - v8::Local v8Value = ::workerd::rust::jsg::local_from_ffi(kj::mv(value)); - ::workerd::jsg::check(v8Context->Global()->Set(v8Context, key, v8Value)); + callback(data, lock.v8Isolate, evalContext); }); } diff --git a/src/rust/jsg-test/ffi.h b/src/rust/jsg-test/ffi.h index a042892064f..d3b3c6cec78 100644 --- a/src/rust/jsg-test/ffi.h +++ b/src/rust/jsg-test/ffi.h @@ -38,11 +38,8 @@ class TestHarness { TestHarness(::workerd::jsg::V8StackScope& stackScope); // Runs a callback within a proper V8 context and stack scope - // The callback receives both the isolate and a context that can be used with eval - void run_in_context(::rust::Fn callback) const; - - // Sets a global variable on the context - void set_global(::rust::Str name, ::workerd::rust::jsg::Local value) const; + // The callback receives the data pointer, isolate and a context + void run_in_context(size_t data, ::rust::Fn callback) const; private: mutable kj::Own isolate; diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index 72fff923d10..e1251c4975f 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -16,11 +16,10 @@ mod ffi { type Local = jsg::v8::ffi::Local; } - #[derive(Debug, PartialEq, Eq)] + #[derive(Debug)] struct EvalResult { success: bool, - result_type: String, - result_value: String, + value: KjMaybe, } unsafe extern "C++" { @@ -32,29 +31,101 @@ mod ffi { pub unsafe fn create_test_harness() -> KjOwn; pub unsafe fn run_in_context( self: &TestHarness, - callback: unsafe fn(*mut Isolate, Pin<&mut EvalContext>), + data: usize, /* callback */ + callback: unsafe fn(usize /* callback */, *mut Isolate, Pin<&mut EvalContext>), ); - #[cxx_name = "eval"] - pub fn eval_safe(self: &EvalContext, code: &str) -> EvalResult; - - #[cxx_name = "set_global"] - pub fn set_global_safe(self: &EvalContext, name: &str, value: Local); + pub unsafe fn eval(self: &EvalContext, code: &str) -> EvalResult; + pub unsafe fn set_global(self: &EvalContext, name: &str, value: Local); } } pub struct Harness(KjOwn); -pub use ffi::EvalContext; -pub use ffi::EvalResult; +pub struct EvalContext<'a> { + inner: &'a ffi::EvalContext, + isolate: v8::IsolatePtr, +} + +impl EvalContext<'_> { + // TODO(soon): There is no way to distinguish a throw versus an error as a return value. + pub fn eval(&self, lock: &mut jsg::Lock, code: &str) -> Result + where + T: jsg::FromJS, + { + let result = unsafe { self.inner.eval(code) }; + let opt_local: Option = result.value.into(); + + if result.success { + match opt_local { + Some(local) => { + T::from_js(lock, unsafe { v8::Local::from_ffi(self.isolate, local) }) + } + None => Err(jsg::Error::new( + "Error", + "eval returned empty result".to_owned(), + )), + } + } else { + match opt_local { + Some(local) => { + let value = unsafe { v8::Local::from_ffi(self.isolate, local) }; + Err(jsg::Error::from_value(lock, value)) + } + None => Err(jsg::Error::new("Error", "eval failed".to_owned())), + } + } + } + + pub fn set_global(&self, name: &str, value: v8::Local) { + unsafe { self.inner.set_global(name, value.into_ffi()) } + } +} impl Harness { pub fn new() -> Self { Self(unsafe { ffi::create_test_harness() }) } - pub fn run_in_context(&self, callback: fn(*mut v8::ffi::Isolate, Pin<&mut EvalContext>)) { - unsafe { self.0.run_in_context(callback) } + /// Runs a callback within a V8 context. + /// + /// The callback is passed through C++ via a data pointer since CXX doesn't support + /// closures directly. The monomorphized trampoline function receives the pointer + /// and reconstructs the closure. + /// + /// The callback returns `Result<(), jsg::Error>` to allow use of the `?` operator. + /// If an error is returned, the test will panic. + pub fn run_in_context(&self, callback: F) + where + F: FnOnce(&mut jsg::Lock, &mut EvalContext) -> Result<(), jsg::Error>, + { + #[expect(clippy::needless_pass_by_value)] + fn trampoline( + data: usize, + isolate: *mut v8::ffi::Isolate, + context: Pin<&mut ffi::EvalContext>, + ) where + F: FnOnce(&mut jsg::Lock, &mut EvalContext) -> Result<(), jsg::Error>, + { + let cb = unsafe { &mut *(data as *mut Option) }; + if let Some(callback) = cb.take() { + let isolate_ptr = unsafe { v8::IsolatePtr::from_ffi(isolate) }; + let mut eval_context = EvalContext { + inner: &context, + isolate: isolate_ptr, + }; + let mut lock = unsafe { jsg::Lock::from_isolate_ptr(isolate) }; + if let Err(e) = callback(&mut lock, &mut eval_context) { + panic!("Test failed: {}: {}", e.name, e.message); + } + } + } + + let mut callback = Some(callback); + unsafe { + self.0 + .run_in_context(&raw mut callback as usize, trampoline::); + } } } diff --git a/src/rust/jsg-test/tests/eval.rs b/src/rust/jsg-test/tests/eval.rs index b4246e0eb4e..fbaae79131f 100644 --- a/src/rust/jsg-test/tests/eval.rs +++ b/src/rust/jsg-test/tests/eval.rs @@ -1,287 +1,165 @@ -use crate::EvalResult; - #[test] -fn eval_string_returns_string_type() { +fn eval_returns_correct_type() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("'Hello, World!'"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "Hello, World!".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "'Hello, World!'")?; + assert_eq!(result, "Hello, World!"); + Ok(()) }); } #[test] fn eval_string_concatenation() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("'Hello' + ', ' + 'World!'"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "Hello, World!".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "'Hello' + ', ' + 'World!'")?; + assert_eq!(result, "Hello, World!"); + Ok(()) }); } #[test] fn eval_number_returns_number_type() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("42"), - EvalResult { - success: true, - result_type: "number".to_owned(), - result_value: "42".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: f64 = ctx.eval(lock, "42")?; + assert!((result - 42.0).abs() < f64::EPSILON); + Ok(()) }); } #[test] fn eval_arithmetic_expression() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("1 + 2 + 3"), - EvalResult { - success: true, - result_type: "number".to_owned(), - result_value: "6".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: f64 = ctx.eval(lock, "1 + 2 + 3")?; + assert!((result - 6.0).abs() < f64::EPSILON); + Ok(()) }); } #[test] fn eval_boolean_true() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("true"), - EvalResult { - success: true, - result_type: "boolean".to_owned(), - result_value: "true".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: bool = ctx.eval(lock, "true")?; + assert!(result); + Ok(()) }); } #[test] fn eval_boolean_false() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("false"), - EvalResult { - success: true, - result_type: "boolean".to_owned(), - result_value: "false".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: bool = ctx.eval(lock, "false")?; + assert!(!result); + Ok(()) }); } #[test] fn eval_comparison_expression() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("5 > 3"), - EvalResult { - success: true, - result_type: "boolean".to_owned(), - result_value: "true".to_owned(), - } - ); - }); -} - -#[test] -fn eval_undefined() { - let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("undefined"), - EvalResult { - success: true, - result_type: "undefined".to_owned(), - result_value: "undefined".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: bool = ctx.eval(lock, "5 > 3")?; + assert!(result); + Ok(()) }); } #[test] fn eval_null() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("null"), - EvalResult { - success: true, - result_type: "object".to_owned(), - result_value: "null".to_owned(), - } - ); - }); -} - -#[test] -fn eval_object() { - let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("({})"), - EvalResult { - success: true, - result_type: "object".to_owned(), - result_value: "[object Object]".to_owned(), - } - ); - }); -} - -#[test] -fn eval_array() { - let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("[1, 2, 3]"), - EvalResult { - success: true, - result_type: "object".to_owned(), - result_value: "1,2,3".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: jsg::Nullable = ctx.eval(lock, "null")?; + assert!(result.is_null()); + Ok(()) }); } #[test] fn eval_throws_on_error() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("throw new Error('test error')"), - EvalResult { - success: false, - result_type: "throws".to_owned(), - result_value: "Error: test error".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: Result = ctx.eval(lock, "throw new Error('test error')"); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.name, "Error"); + assert_eq!(err.message, "test error"); + Ok(()) }); } #[test] -fn eval_throws_type_error() { +fn eval_throws_string_preserves_message() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("null.foo"), - EvalResult { - success: false, - result_type: "throws".to_owned(), - result_value: "TypeError: Cannot read properties of null (reading 'foo')" - .to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: Result = ctx.eval(lock, "throw 'custom string error'"); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.name, "Error"); + assert_eq!(err.message, "custom string error"); + Ok(()) }); } #[test] fn eval_function_call() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("(function() { return 'from function'; })()"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "from function".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "(function() { return 'from function'; })()")?; + assert_eq!(result, "from function"); + Ok(()) }); } #[test] fn eval_typeof_string() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("typeof 'hello'"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "string".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "typeof 'hello'")?; + assert_eq!(result, "string"); + Ok(()) }); } #[test] fn eval_typeof_number() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("typeof 42"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "number".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "typeof 42")?; + assert_eq!(result, "number"); + Ok(()) }); } #[test] fn eval_typeof_boolean() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("typeof true"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "boolean".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "typeof true")?; + assert_eq!(result, "boolean"); + Ok(()) }); } #[test] fn eval_unicode_string() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("'こんにちは'"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "こんにちは".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "'こんにちは'")?; + assert_eq!(result, "こんにちは"); + Ok(()) }); } #[test] fn eval_emoji_string() { let harness = crate::Harness::new(); - harness.run_in_context(|_isolate, ctx| { - assert_eq!( - ctx.eval_safe("'😀🎉'"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "😀🎉".to_owned(), - } - ); + harness.run_in_context(|lock, ctx| { + let result: String = ctx.eval(lock, "'😀🎉'")?; + assert_eq!(result, "😀🎉"); + Ok(()) }); } diff --git a/src/rust/jsg-test/tests/jsg_struct.rs b/src/rust/jsg-test/tests/jsg_struct.rs index 6f8945044d3..54313c0b64c 100644 --- a/src/rust/jsg-test/tests/jsg_struct.rs +++ b/src/rust/jsg-test/tests/jsg_struct.rs @@ -1,7 +1,6 @@ use jsg::Error; use jsg::ExceptionType; -use jsg::Lock; -use jsg::Wrappable; +use jsg::ToJS; use jsg::v8; use jsg::v8::ToLocalValue; use jsg_macros::jsg_struct; @@ -26,146 +25,144 @@ struct NestedStruct { #[test] fn objects_can_be_wrapped_and_unwrapped() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, _ctx| { let instance = TestStruct { str: "test".to_owned(), }; - let wrapped = instance.wrap(&mut lock); + let wrapped = instance.to_js(lock); let mut obj: v8::Local<'_, v8::Object> = wrapped.into(); - assert!(obj.has(&mut lock, "str")); - let str_value = obj.get(&mut lock, "str"); + assert!(obj.has(lock, "str")); + let str_value = obj.get(lock, "str"); assert!(str_value.unwrap().is_string()); - assert!(!obj.has(&mut lock, "test")); - let value = "value".to_local(&mut lock); + assert!(!obj.has(lock, "test")); + let value = "value".to_local(lock); assert!(value.is_string()); - obj.set(&mut lock, "test", value); - assert!(obj.has(&mut lock, "test")); + obj.set(lock, "test", value); + assert!(obj.has(lock, "test")); + Ok(()) }); } #[test] fn struct_with_multiple_properties() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, _ctx| { let instance = MultiPropertyStruct { name: "Alice".to_owned(), age: 30, active: "true".to_owned(), }; - let wrapped = instance.wrap(&mut lock); + let wrapped = instance.to_js(lock); let obj: v8::Local<'_, v8::Object> = wrapped.into(); - assert!(obj.has(&mut lock, "name")); - assert!(obj.has(&mut lock, "age")); - assert!(obj.has(&mut lock, "active")); + assert!(obj.has(lock, "name")); + assert!(obj.has(lock, "age")); + assert!(obj.has(lock, "active")); - let name_value = obj.get(&mut lock, "name"); + let name_value = obj.get(lock, "name"); assert!(name_value.is_some()); assert!(name_value.unwrap().is_string()); - let age_value = obj.get(&mut lock, "age"); + let age_value = obj.get(lock, "age"); assert!(age_value.is_some()); - let active_value = obj.get(&mut lock, "active"); + let active_value = obj.get(lock, "active"); assert!(active_value.is_some()); assert!(active_value.unwrap().is_string()); + Ok(()) }); } #[test] fn number_type_conversions() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - + harness.run_in_context(|lock, _ctx| { let byte_val: u8 = 42; - let byte_local = byte_val.to_local(&mut lock); + let byte_local = byte_val.to_local(lock); assert!(byte_local.has_value()); let int_val: u32 = 12345; - let int_local = int_val.to_local(&mut lock); + let int_local = int_val.to_local(lock); assert!(int_local.has_value()); + Ok(()) }); } #[test] fn empty_object_and_property_setting() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, _ctx| { let mut obj = lock.new_object(); - assert!(!obj.has(&mut lock, "nonexistent")); - assert!(obj.get(&mut lock, "nonexistent").is_none()); + assert!(!obj.has(lock, "nonexistent")); + assert!(obj.get(lock, "nonexistent").is_none()); - let str_value = "hello".to_local(&mut lock); - obj.set(&mut lock, "key1", str_value); - assert!(obj.has(&mut lock, "key1")); + let str_value = "hello".to_local(lock); + obj.set(lock, "key1", str_value); + assert!(obj.has(lock, "key1")); - let num_value = 100u32.to_local(&mut lock); - obj.set(&mut lock, "key2", num_value); - assert!(obj.has(&mut lock, "key2")); + let num_value = 100u32.to_local(lock); + obj.set(lock, "key2", num_value); + assert!(obj.has(lock, "key2")); - let val1 = obj.get(&mut lock, "key1"); + let val1 = obj.get(lock, "key1"); assert!(val1.is_some()); assert!(val1.unwrap().is_string()); - let val2 = obj.get(&mut lock, "key2"); + let val2 = obj.get(lock, "key2"); assert!(val2.is_some()); + Ok(()) }); } #[test] fn global_handle_conversion() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let local_str = "global test".to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let local_str = "global test".to_local(lock); assert!(local_str.has_value()); - let global_str = local_str.to_global(&mut lock); - let local_again = global_str.as_local(&mut lock); + let global_str = local_str.to_global(lock); + let local_again = global_str.as_local(lock); assert!(local_again.has_value()); + Ok(()) }); } #[test] fn nested_object_properties() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, _ctx| { let mut outer = lock.new_object(); let inner_instance = NestedStruct { inner: "nested value".to_owned(), }; - let inner_wrapped = inner_instance.wrap(&mut lock); - outer.set(&mut lock, "nested", inner_wrapped); + let inner_wrapped = inner_instance.to_js(lock); + outer.set(lock, "nested", inner_wrapped); - assert!(outer.has(&mut lock, "nested")); - let nested_val = outer.get(&mut lock, "nested"); + assert!(outer.has(lock, "nested")); + let nested_val = outer.get(lock, "nested"); assert!(nested_val.is_some()); let nested_obj: v8::Local<'_, v8::Object> = nested_val.unwrap().into(); - assert!(nested_obj.has(&mut lock, "inner")); + assert!(nested_obj.has(lock, "inner")); - let inner_val = nested_obj.get(&mut lock, "inner"); + let inner_val = nested_obj.get(lock, "inner"); assert!(inner_val.is_some()); assert!(inner_val.unwrap().is_string()); + Ok(()) }); } #[test] fn error_creation_and_display() { let error = Error::default(); - assert_eq!(error.name.to_string(), "Error"); + assert_eq!(error.name, "Error"); assert_eq!(error.message, "An unknown error occurred"); - let type_error = Error::new(ExceptionType::TypeError, "Invalid type".to_owned()); - assert_eq!(type_error.name.to_string(), "TypeError"); + let type_error = Error::new("TypeError", "Invalid type".to_owned()); + assert_eq!(type_error.name, "TypeError"); assert_eq!(type_error.message, "Invalid type"); // Test Display for all exception types @@ -181,27 +178,25 @@ fn error_creation_and_display() { fn error_from_parse_int_error() { let parse_result: Result = "not_a_number".parse(); let error: Error = parse_result.unwrap_err().into(); - assert_eq!(error.name.to_string(), "TypeError"); + assert_eq!(error.name, "TypeError"); assert!(error.message.contains("Failed to parse integer")); } #[test] fn type_of_returns_correct_js_types() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let str_val = "hello".to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let str_val = "hello".to_local(lock); assert_eq!(str_val.type_of(), "string"); assert!(str_val.is_string()); assert!(!str_val.is_null_or_undefined()); - let num_val = 42u32.to_local(&mut lock); + let num_val = 42u32.to_local(lock); assert_eq!(num_val.type_of(), "number"); assert!(num_val.is_number()); assert!(!num_val.is_null_or_undefined()); - let bool_val = true.to_local(&mut lock); + let bool_val = true.to_local(lock); assert_eq!(bool_val.type_of(), "boolean"); assert!(bool_val.is_boolean()); assert!(!bool_val.is_null_or_undefined()); @@ -209,5 +204,6 @@ fn type_of_returns_correct_js_types() { let obj_val = lock.new_object(); assert_eq!(obj_val.type_of(), "object"); assert!(!obj_val.is_null_or_undefined()); + Ok(()) }); } diff --git a/src/rust/jsg-test/tests/non_coercible.rs b/src/rust/jsg-test/tests/non_coercible.rs index 1734f092245..d0fac73a921 100644 --- a/src/rust/jsg-test/tests/non_coercible.rs +++ b/src/rust/jsg-test/tests/non_coercible.rs @@ -1,12 +1,9 @@ -use jsg::Lock; use jsg::NonCoercible; use jsg::ResourceState; use jsg::ResourceTemplate; use jsg_macros::jsg_method; use jsg_macros::jsg_resource; -use crate::EvalResult; - #[jsg_resource] struct MyResource { _state: ResourceState, @@ -126,58 +123,46 @@ fn non_coercible_debug() { #[test] fn non_coercible_methods_accept_correct_types_and_reject_incorrect_types() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, ctx| { let resource = jsg::Ref::new(MyResource { _state: ResourceState::default(), }); - let mut template = MyResourceTemplate::new(&mut lock); - let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); - ctx.set_global_safe("resource", wrapped.into_ffi()); + let mut template = MyResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("resource", wrapped); // String method accepts string - assert_eq!( - ctx.eval_safe("resource.string('hello')"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "hello".to_owned(), - } - ); + let result: String = ctx.eval(lock, "resource.string('hello')")?; + assert_eq!(result, "hello"); // String method rejects number - let result = ctx.eval_safe("resource.string(123)"); - assert!(!result.success); - assert!(result.result_value.contains("string")); + let result: Result = ctx.eval(lock, "resource.string(123)"); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.name, "TypeError"); + assert!(err.message.contains("string")); // Boolean method accepts boolean - assert_eq!( - ctx.eval_safe("resource.boolean(true)"), - EvalResult { - success: true, - result_type: "boolean".to_owned(), - result_value: "true".to_owned(), - } - ); + let result: bool = ctx.eval(lock, "resource.boolean(true)")?; + assert!(result); // Boolean method rejects string - let result = ctx.eval_safe("resource.boolean('true')"); - assert!(!result.success); - assert!(result.result_value.contains("boolean")); + let result: Result = ctx.eval(lock, "resource.boolean('true')"); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.name, "TypeError"); + assert!(err.message.contains("boolean")); // Number method accepts number - assert_eq!( - ctx.eval_safe("resource.number(42.5)"), - EvalResult { - success: true, - result_type: "number".to_owned(), - result_value: "42.5".to_owned(), - } - ); + let result: f64 = ctx.eval(lock, "resource.number(42.5)")?; + assert!((result - 42.5).abs() < f64::EPSILON); // Number method rejects string - let result = ctx.eval_safe("resource.number('42')"); - assert!(!result.success); - assert!(result.result_value.contains("number")); + let result: Result = ctx.eval(lock, "resource.number('42')"); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.name, "TypeError"); + assert!(err.message.contains("number")); + Ok(()) }); } diff --git a/src/rust/jsg-test/tests/resource_callback.rs b/src/rust/jsg-test/tests/resource_callback.rs index 4cbc4b000cb..8529c1e86db 100644 --- a/src/rust/jsg-test/tests/resource_callback.rs +++ b/src/rust/jsg-test/tests/resource_callback.rs @@ -8,14 +8,11 @@ use std::cell::Cell; use std::rc::Rc; -use jsg::Lock; use jsg::ResourceState; use jsg::ResourceTemplate; use jsg_macros::jsg_method; use jsg_macros::jsg_resource; -use crate::EvalResult; - #[jsg_resource] struct EchoResource { _state: ResourceState, @@ -72,25 +69,19 @@ impl DirectReturnResource { #[test] fn resource_method_callback_receives_correct_self() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, ctx| { let resource = jsg::Ref::new(EchoResource { _state: ResourceState::default(), prefix: "Hello, ".to_owned(), }); - let mut template = EchoResourceTemplate::new(&mut lock); - let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); - ctx.set_global_safe("echoResource", wrapped.into_ffi()); + let mut template = EchoResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("echoResource", wrapped); // Call the method from JavaScript - assert_eq!( - ctx.eval_safe("echoResource.echo('World!')"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "Hello, World!".to_owned(), - } - ); + let result: String = ctx.eval(lock, "echoResource.echo('World!')")?; + assert_eq!(result, "Hello, World!"); + Ok(()) }); } @@ -98,35 +89,23 @@ fn resource_method_callback_receives_correct_self() { #[test] fn resource_method_can_be_called_multiple_times() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, ctx| { let resource = jsg::Ref::new(EchoResource { _state: ResourceState::default(), prefix: ">> ".to_owned(), }); - let mut template = EchoResourceTemplate::new(&mut lock); - let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); - ctx.set_global_safe("echo", wrapped.into_ffi()); + let mut template = EchoResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("echo", wrapped); // First call - assert_eq!( - ctx.eval_safe("echo.echo('first')"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: ">> first".to_owned(), - } - ); + let result: String = ctx.eval(lock, "echo.echo('first')")?; + assert_eq!(result, ">> first"); // Second call - assert_eq!( - ctx.eval_safe("echo.echo('second')"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: ">> second".to_owned(), - } - ); + let result: String = ctx.eval(lock, "echo.echo('second')")?; + assert_eq!(result, ">> second"); + Ok(()) }); } @@ -134,63 +113,37 @@ fn resource_method_can_be_called_multiple_times() { #[test] fn resource_method_returns_non_result_values() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, ctx| { let counter = Rc::new(Cell::new(42)); let resource = jsg::Ref::new(DirectReturnResource { _state: ResourceState::default(), name: "TestResource".to_owned(), counter: counter.clone(), }); - let mut template = DirectReturnResourceTemplate::new(&mut lock); - let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); - ctx.set_global_safe("resource", wrapped.into_ffi()); - - assert_eq!( - ctx.eval_safe("resource.getName()"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "TestResource".to_owned(), - } - ); - - assert_eq!( - ctx.eval_safe("resource.isValid()"), - EvalResult { - success: true, - result_type: "boolean".to_owned(), - result_value: "true".to_owned(), - } - ); - - assert_eq!( - ctx.eval_safe("resource.getCounter()"), - EvalResult { - success: true, - result_type: "number".to_owned(), - result_value: "42".to_owned(), - } - ); - - assert_eq!( - ctx.eval_safe("resource.incrementCounter()"), - EvalResult { - success: true, - result_type: "undefined".to_owned(), - result_value: "undefined".to_owned(), - } - ); + let mut template = DirectReturnResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("resource", wrapped); + + // Test getString returns string + let result: String = ctx.eval(lock, "resource.getName()")?; + assert_eq!(result, "TestResource"); + + // Test isValid returns boolean + let result: bool = ctx.eval(lock, "resource.isValid()")?; + assert!(result); + + // Test getCounter returns number + let result: f64 = ctx.eval(lock, "resource.getCounter()")?; + assert!((result - 42.0).abs() < f64::EPSILON); + + // Test incrementCounter returns undefined (we just check it doesn't error) + let _: Option = ctx.eval(lock, "resource.incrementCounter()")?; assert_eq!(counter.get(), 43); - assert_eq!( - ctx.eval_safe("resource.maybeName()"), - EvalResult { - success: true, - result_type: "string".to_owned(), - result_value: "TestResource".to_owned(), - } - ); + // Test maybeName returns string for Some + let result: String = ctx.eval(lock, "resource.maybeName()")?; + assert_eq!(result, "TestResource"); + Ok(()) }); } @@ -198,24 +151,18 @@ fn resource_method_returns_non_result_values() { #[test] fn resource_method_returns_null_for_none() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); + harness.run_in_context(|lock, ctx| { let resource = jsg::Ref::new(DirectReturnResource { _state: ResourceState::default(), name: String::new(), counter: Rc::new(Cell::new(0)), }); - let mut template = DirectReturnResourceTemplate::new(&mut lock); - let wrapped = jsg::wrap_resource(&mut lock, resource, &mut template); - ctx.set_global_safe("resource", wrapped.into_ffi()); - - assert_eq!( - ctx.eval_safe("resource.maybeName()"), - EvalResult { - success: true, - result_type: "object".to_owned(), - result_value: "null".to_owned(), - } - ); + let mut template = DirectReturnResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("resource", wrapped); + + let result: Option = ctx.eval(lock, "resource.maybeName()")?; + assert!(result.is_none()); + Ok(()) }); } diff --git a/src/rust/jsg-test/tests/unwrap.rs b/src/rust/jsg-test/tests/unwrap.rs index f22c52fd4b3..af50db2f6a1 100644 --- a/src/rust/jsg-test/tests/unwrap.rs +++ b/src/rust/jsg-test/tests/unwrap.rs @@ -1,125 +1,120 @@ -use jsg::Lock; use jsg::v8::ToLocalValue; #[test] fn v8_is_string_returns_true_for_strings() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let str_val = "hello".to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let str_val = "hello".to_local(lock); assert!(str_val.is_string()); + Ok(()) }); } #[test] fn v8_is_boolean_returns_true_for_booleans() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let bool_true = true.to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let bool_true = true.to_local(lock); assert!(bool_true.is_boolean()); - let bool_false = false.to_local(&mut lock); + let bool_false = false.to_local(lock); assert!(bool_false.is_boolean()); + Ok(()) }); } #[test] fn v8_is_number_returns_true_for_numbers() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let float_val = 2.5f64.to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let float_val = 2.5f64.to_local(lock); assert!(float_val.is_number()); - let int_val = 42u32.to_local(&mut lock); + let int_val = 42u32.to_local(lock); assert!(int_val.is_number()); - let num_u8 = 255u8.to_local(&mut lock); + let num_u8 = 255u8.to_local(lock); assert!(num_u8.is_number()); + Ok(()) }); } #[test] fn v8_type_checks_are_mutually_exclusive() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - + harness.run_in_context(|lock, _ctx| { // String is only string - let s = "test".to_local(&mut lock); + let s = "test".to_local(lock); assert!(s.is_string() && !s.is_boolean() && !s.is_number()); // Boolean is only boolean - let b = true.to_local(&mut lock); + let b = true.to_local(lock); assert!(!b.is_string() && b.is_boolean() && !b.is_number()); // Number is only number - let n = 42.0f64.to_local(&mut lock); + let n = 42.0f64.to_local(lock); assert!(!n.is_string() && !n.is_boolean() && n.is_number()); + Ok(()) }); } #[test] fn v8_unwrap_boolean_returns_correct_values() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let bool_true = true.to_local(&mut lock); + harness.run_in_context(|lock, _ctx| { + let bool_true = true.to_local(lock); let unwrapped_true = - jsg::v8::ffi::unwrap_boolean(lock.isolate().as_ffi(), bool_true.into_ffi()); + unsafe { jsg::v8::ffi::unwrap_boolean(lock.isolate().as_ffi(), bool_true.into_ffi()) }; assert!(unwrapped_true); - let bool_false = false.to_local(&mut lock); + let bool_false = false.to_local(lock); let unwrapped_false = - jsg::v8::ffi::unwrap_boolean(lock.isolate().as_ffi(), bool_false.into_ffi()); + unsafe { jsg::v8::ffi::unwrap_boolean(lock.isolate().as_ffi(), bool_false.into_ffi()) }; assert!(!unwrapped_false); + Ok(()) }); } #[test] fn v8_unwrap_number_returns_correct_values() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let num = 2.5f64.to_local(&mut lock); - let unwrapped = jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), num.into_ffi()); + harness.run_in_context(|lock, _ctx| { + let num = 2.5f64.to_local(lock); + let unwrapped = + unsafe { jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), num.into_ffi()) }; assert!((unwrapped - 2.5).abs() < f64::EPSILON); - let zero = 0.0f64.to_local(&mut lock); - let unwrapped_zero = jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), zero.into_ffi()); + let zero = 0.0f64.to_local(lock); + let unwrapped_zero = + unsafe { jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), zero.into_ffi()) }; assert!(unwrapped_zero.abs() < f64::EPSILON); - let negative = (-42.5f64).to_local(&mut lock); + let negative = (-42.5f64).to_local(lock); let unwrapped_neg = - jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), negative.into_ffi()); + unsafe { jsg::v8::ffi::unwrap_number(lock.isolate().as_ffi(), negative.into_ffi()) }; assert!((unwrapped_neg - (-42.5)).abs() < f64::EPSILON); + Ok(()) }); } #[test] fn v8_unwrap_string_returns_correct_values() { let harness = crate::Harness::new(); - harness.run_in_context(|isolate, _ctx| unsafe { - let mut lock = Lock::from_isolate_ptr(isolate); - - let s = "hello world".to_local(&mut lock); - let unwrapped = jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), s.into_ffi()); + harness.run_in_context(|lock, _ctx| { + let s = "hello world".to_local(lock); + let unwrapped = + unsafe { jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), s.into_ffi()) }; assert_eq!(unwrapped.as_str(), "hello world"); - let empty = "".to_local(&mut lock); + let empty = "".to_local(lock); let unwrapped_empty = - jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), empty.into_ffi()); + unsafe { jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), empty.into_ffi()) }; assert_eq!(unwrapped_empty.as_str(), ""); - let unicode = "こんにちは".to_local(&mut lock); + let unicode = "こんにちは".to_local(lock); let unwrapped_unicode = - jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), unicode.into_ffi()); + unsafe { jsg::v8::ffi::unwrap_string(lock.isolate().as_ffi(), unicode.into_ffi()) }; assert_eq!(unwrapped_unicode.as_str(), "こんにちは"); + Ok(()) }); } diff --git a/src/rust/jsg/ffi.c++ b/src/rust/jsg/ffi.c++ index eb610f08b97..16e7995e824 100644 --- a/src/rust/jsg/ffi.c++ +++ b/src/rust/jsg/ffi.c++ @@ -94,6 +94,10 @@ bool local_is_object(const Local& val) { return local_as_ref_from_ffi(val)->IsObject(); } +bool local_is_native_error(const Local& val) { + return local_as_ref_from_ffi(val)->IsNativeError(); +} + ::rust::String local_type_of(Isolate* isolate, const Local& val) { auto v8Val = local_as_ref_from_ffi(val); v8::Local typeStr = v8Val->TypeOf(isolate); diff --git a/src/rust/jsg/ffi.h b/src/rust/jsg/ffi.h index 02be16bec44..aab36bfb710 100644 --- a/src/rust/jsg/ffi.h +++ b/src/rust/jsg/ffi.h @@ -45,6 +45,7 @@ bool local_is_null(const Local& val); bool local_is_undefined(const Local& val); bool local_is_null_or_undefined(const Local& val); bool local_is_object(const Local& val); +bool local_is_native_error(const Local& val); ::rust::String local_type_of(Isolate* isolate, const Local& val); // Local diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index 838b014c279..adee939027b 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -14,8 +14,8 @@ pub mod v8; mod wrappable; pub use v8::ffi::ExceptionType; -pub use wrappable::Unwrappable; -pub use wrappable::Wrappable; +pub use wrappable::FromJS; +pub use wrappable::ToJS; #[cxx::bridge(namespace = "workerd::rust::jsg")] mod ffi { @@ -142,22 +142,68 @@ pub fn unwrap_resource<'a, R: Resource>( unsafe { &mut *ptr } } +#[derive(Debug)] pub struct Error { - pub name: v8::ffi::ExceptionType, + pub name: String, pub message: String, } +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {}", self.name, self.message) + } +} + impl Error { - pub fn new(name: v8::ffi::ExceptionType, message: String) -> Self { - Self { name, message } + pub fn new(name: impl Into, message: String) -> Self { + Self { + name: name.into(), + message, + } + } + + /// Creates an Error from a V8 value (typically an exception). + /// + /// If the value is a native error, extracts the name and message properties. + /// Otherwise, converts the value to a string for the message. + pub fn from_value(lock: &mut Lock, value: v8::Local) -> Self { + if value.is_native_error() { + let obj: v8::Local = value.into(); + + let name = obj + .get(lock, "name") + .and_then(|v| String::from_js(lock, v).ok()) + .unwrap_or_else(|| "Error".to_owned()); + + let message = obj + .get(lock, "message") + .and_then(|v| String::from_js(lock, v).ok()) + .unwrap_or_else(|| "Unknown error".to_owned()); + + Self { name, message } + } else { + let message = + String::from_js(lock, value).unwrap_or_else(|_| "Unknown error".to_owned()); + Self { + name: "Error".to_owned(), + message, + } + } } /// Creates a V8 exception from this error. - pub fn as_exception<'a>(&self, isolate: v8::IsolatePtr) -> v8::Local<'a, v8::Value> { + pub fn to_local<'a>(&self, isolate: v8::IsolatePtr) -> v8::Local<'a, v8::Value> { + let exception_type = match self.name.as_str() { + "TypeError" => v8::ffi::ExceptionType::TypeError, + "RangeError" => v8::ffi::ExceptionType::RangeError, + "ReferenceError" => v8::ffi::ExceptionType::ReferenceError, + "SyntaxError" => v8::ffi::ExceptionType::SyntaxError, + _ => v8::ffi::ExceptionType::Error, + }; unsafe { v8::Local::from_ffi( isolate, - v8::ffi::exception_create(isolate.as_ffi(), self.name, &self.message), + v8::ffi::exception_create(isolate.as_ffi(), exception_type, &self.message), ) } } @@ -166,7 +212,7 @@ impl Error { impl Default for Error { fn default() -> Self { Self { - name: v8::ffi::ExceptionType::Error, + name: "Error".to_owned(), message: "An unknown error occurred".to_owned(), } } @@ -174,10 +220,7 @@ impl Default for Error { impl From for Error { fn from(err: ParseIntError) -> Self { - Self::new( - v8::ffi::ExceptionType::TypeError, - format!("Failed to parse integer: {err}"), - ) + Self::new("TypeError", format!("Failed to parse integer: {err}")) } } @@ -222,11 +265,11 @@ impl From for Error { /// input accordingly to avoid being a source of user confusion. Only use `NonCoercible` if /// you have a good reason to disable coercion. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct NonCoercible { +pub struct NonCoercible { value: T, } -impl NonCoercible { +impl NonCoercible { /// Creates a new `NonCoercible` wrapper around the given value. pub fn new(value: T) -> Self { Self { value } @@ -238,19 +281,19 @@ impl NonCoercible { } } -impl From for NonCoercible { +impl From for NonCoercible { fn from(value: T) -> Self { Self::new(value) } } -impl AsRef for NonCoercible { +impl AsRef for NonCoercible { fn as_ref(&self) -> &T { &self.value } } -impl Deref for NonCoercible { +impl Deref for NonCoercible { type Target = T; fn deref(&self) -> &Self::Target { @@ -258,6 +301,93 @@ impl Deref for NonCoercible { } } +/// A wrapper type that accepts `null`, `undefined`, or a value of type `T`. +/// +/// `Nullable` is similar to `Option` but also accepts `undefined` as a null-ish value. +/// This is useful for JavaScript APIs where both `null` and `undefined` represent +/// the absence of a value. +/// +/// # Behavior +/// +/// - `null` → `Nullable::Null` +/// - `undefined` → `Nullable::Undefined` +/// - `T` → `Nullable::Some(T)` +/// +/// # Example +/// +/// ```ignore +/// use jsg::Nullable; +/// +/// #[jsg_method] +/// pub fn process(&self, value: Nullable) -> Result<(), Error> { +/// match value { +/// Nullable::Some(s) => println!("Got value: {}", s), +/// Nullable::Null => println!("Got null"), +/// Nullable::Undefined => println!("Got undefined"), +/// } +/// Ok(()) +/// } +/// ``` +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Nullable { + Some(T), + Null, + Undefined, +} + +impl Nullable { + /// Returns `true` if the nullable contains a value. + pub fn is_some(&self) -> bool { + matches!(self, Self::Some(_)) + } + + /// Returns `true` if the nullable is `Null`. + pub fn is_null(&self) -> bool { + matches!(self, Self::Null) + } + + /// Returns `true` if the nullable is `Undefined`. + pub fn is_undefined(&self) -> bool { + matches!(self, Self::Undefined) + } + + /// Returns `true` if the nullable is `Null` or `Undefined`. + pub fn is_null_or_undefined(&self) -> bool { + matches!(self, Self::Null | Self::Undefined) + } + + /// Converts from `Nullable` to `Option`. + pub fn into_option(self) -> Option { + match self { + Self::Some(v) => Some(v), + Self::Null | Self::Undefined => None, + } + } + + /// Returns a reference to the contained value, or `None` if null or undefined. + pub fn as_ref(&self) -> Option<&T> { + match self { + Self::Some(v) => Some(v), + Self::Null | Self::Undefined => None, + } + } +} + +impl From> for Nullable { + fn from(opt: Option) -> Self { + match opt { + Some(v) => Self::Some(v), + None => Self::Null, + } + } +} + +impl From> for Option { + fn from(nullable: Nullable) -> Self { + nullable.into_option() + } +} + /// Provides access to V8 operations within an isolate lock. /// /// A Lock wraps a V8 isolate pointer and is passed to resource methods and callbacks to @@ -376,7 +506,7 @@ impl Clone for Ref { /// /// This trait provides type information used for error messages, memory tracking, /// and type validation (for `NonCoercible`). The actual conversion logic is in -/// `Wrappable` (Rust → JS) and `Unwrappable` (JS → Rust). +/// `ToJS` (Rust → JS) and `FromJS` (JS → Rust). /// /// TODO: Implement `memory_info(jsg::MemoryTracker)` pub trait Type: Sized { diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index a13ce3a353b..31e6b5dbeaa 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -17,6 +17,7 @@ pub mod ffi { ptr: usize, } + #[derive(Debug)] enum ExceptionType { RangeError, ReferenceError, @@ -63,6 +64,7 @@ pub mod ffi { pub unsafe fn local_is_undefined(value: &Local) -> bool; pub unsafe fn local_is_null_or_undefined(value: &Local) -> bool; pub unsafe fn local_is_object(value: &Local) -> bool; + pub unsafe fn local_is_native_error(value: &Local) -> bool; pub unsafe fn local_type_of(isolate: *mut Isolate, value: &Local) -> String; // Local @@ -296,6 +298,11 @@ impl<'a, T> Local<'a, T> { unsafe { ffi::local_is_object(&self.handle) } } + /// Returns true if the value is a native JavaScript error. + pub fn is_native_error(&self) -> bool { + unsafe { ffi::local_is_native_error(&self.handle) } + } + /// Returns the JavaScript type of the underlying value as a string. /// /// Uses V8's native `TypeOf` method which returns the same result as diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index cfda81ee0ab..66e941dbefa 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -12,57 +12,56 @@ //! | `String` | `string` | //! | `bool` | `boolean` | //! | `f64` | `number` | -//! | `Option` | `T` or `null`/`undefined` | +//! | `Option` | `T` or `undefined` | +//! | `Nullable` | `T`, `null`, or `undefined` | //! | `Result` | `T` or throws | //! | `NonCoercible` | `T` (strict type checking) | //! | `T: Struct` | `object` | use std::fmt::Display; +use crate::Error; use crate::Lock; use crate::NonCoercible; +use crate::Nullable; use crate::Type; use crate::v8; use crate::v8::ToLocalValue; // ============================================================================= -// Wrappable trait (Rust → JavaScript) +// ToJS trait (Rust → JavaScript) // ============================================================================= /// Trait for converting Rust values to JavaScript. /// /// Provides Rust → JavaScript conversion. -pub trait Wrappable: Sized { +pub trait ToJS: Sized { /// Converts this Rust value into a JavaScript value. - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a; } // ============================================================================= -// Unwrappable trait (JavaScript → Rust) +// FromJS trait (JavaScript → Rust) // ============================================================================= /// Trait for converting JavaScript values to Rust. /// /// Provides JS → Rust conversion. The `try_unwrap` method is used by macros /// to unwrap function parameters with proper error handling. -pub trait Unwrappable: Sized { - /// Converts a JavaScript value into this Rust type. - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self; +pub trait FromJS: Sized { + type ResultType; - /// Attempts to unwrap a JavaScript value, returning None and throwing a JS error on failure. - /// Most types simply delegate to `unwrap`, but `NonCoercible` validates the type first. - fn try_unwrap(lock: &mut Lock, value: v8::Local) -> Option { - Some(Self::unwrap(lock.isolate(), value)) - } + /// Converts a JavaScript value into this Rust type. + fn from_js(lock: &mut Lock, value: v8::Local) -> Result; } // ============================================================================= // Primitive type implementations // ============================================================================= -/// Implements `Type`, `Wrappable`, and `Unwrappable` for primitive types. +/// Implements `Type`, `ToJS`, and `FromJS` for primitive types. macro_rules! impl_primitive { { $type:ty, $class_name:literal, $is_exact:ident, $unwrap_fn:ident } => { impl Type for $type { @@ -75,8 +74,8 @@ macro_rules! impl_primitive { } } - impl Wrappable for $type { - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + impl ToJS for $type { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a, { @@ -84,9 +83,11 @@ macro_rules! impl_primitive { } } - impl Unwrappable for $type { - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - unsafe { v8::ffi::$unwrap_fn(isolate.as_ffi(), value.into_ffi()) } + impl FromJS for $type { + type ResultType = Self; + + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { + Ok(unsafe { v8::ffi::$unwrap_fn(lock.isolate().as_ffi(), value.into_ffi()) }) } } }; @@ -100,8 +101,8 @@ impl_primitive!(f64, "number", is_number, unwrap_number); // Wrapper type implementations // ============================================================================= -impl Wrappable for () { - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> +impl ToJS for () { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a, { @@ -109,15 +110,15 @@ impl Wrappable for () { } } -impl Wrappable for Result { - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> +impl ToJS for Result { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a, { match self { - Ok(value) => value.wrap(lock), + Ok(value) => value.to_js(lock), Err(err) => { - // TODO(soon): Use jsg::Error trait to dynamically call proper method to throw the error. + // TODO(soon): Use Error trait to dynamically call proper method to throw the error. let description = err.to_string(); unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &description) }; v8::Local::::undefined(lock) @@ -126,59 +127,99 @@ impl Wrappable for Result { } } -impl Wrappable for Option { - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> +impl ToJS for Option { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a, { match self { - Some(value) => value.wrap(lock), - None => v8::Local::::null(lock), + Some(value) => value.to_js(lock), + None => v8::Local::::undefined(lock), } } } -impl Wrappable for NonCoercible { - fn wrap<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> +impl ToJS for NonCoercible { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where 'b: 'a, { - self.into_inner().wrap(lock) + self.into_inner().to_js(lock) } } -impl Unwrappable for Option { - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { +impl ToJS for Nullable { + fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> + where + 'b: 'a, + { + match self { + Self::Some(value) => value.to_js(lock), + Self::Null => v8::Local::::null(lock), + Self::Undefined => v8::Local::::undefined(lock), + } + } +} + +impl FromJS for Option { + type ResultType = Option; + + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { if value.is_null() { - None + let error_msg = format!("Expected {} or undefined but got null", T::class_name()); + // TODO(soon): Throw errors in the correct place. This function should return + // a result and the caller should handle the error. + let err = Error::new("TypeError", error_msg); + unsafe { + v8::ffi::isolate_throw_exception( + lock.isolate().as_ffi(), + err.to_local(lock.isolate()).into_ffi(), + ); + } + Err(err) } else if value.is_undefined() { - let error_msg = format!( - "Expected a null or {} value but got undefined", - T::class_name() - ); - unsafe { v8::ffi::isolate_throw_error(isolate.as_ffi(), &error_msg) }; - None + Ok(None) } else { - Some(T::unwrap(isolate, value)) + Ok(Some(T::from_js(lock, value)?)) } } } -impl Unwrappable for NonCoercible { - fn unwrap(isolate: v8::IsolatePtr, value: v8::Local) -> Self { - Self::new(T::unwrap(isolate, value)) - } +impl FromJS for NonCoercible { + type ResultType = NonCoercible; - fn try_unwrap(lock: &mut Lock, value: v8::Local) -> Option { + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { if !T::is_exact(&value) { let error_msg = format!( "Expected a {} value but got {}", T::class_name(), value.type_of() ); - unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &error_msg) }; - return None; + // TODO(soon): Throw errors in the correct place. This function should return + // a result and the caller should handle the error. + let err = Error::new("TypeError", error_msg); + unsafe { + v8::ffi::isolate_throw_exception( + lock.isolate().as_ffi(), + err.to_local(lock.isolate()).into_ffi(), + ); + } + return Err(err); + } + Ok(::new(T::from_js(lock, value)?)) + } +} + +impl FromJS for Nullable { + type ResultType = Nullable; + + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { + if value.is_null() { + Ok(Nullable::Null) + } else if value.is_undefined() { + Ok(Nullable::Undefined) + } else { + Ok(Nullable::Some(T::from_js(lock, value)?)) } - Some(Self::new(T::unwrap(lock.isolate(), value))) } } From c6daeecddbc4c7d473fb283597837148a93557b8 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 6 Jan 2026 15:46:42 -0500 Subject: [PATCH 7/9] fix compile flags --- compile_flags.txt | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/compile_flags.txt b/compile_flags.txt index 0e3392aa935..d2c4ae4588d 100644 --- a/compile_flags.txt +++ b/compile_flags.txt @@ -2,17 +2,17 @@ -stdlib=libc++ -xc++ -nostdinc --Ibazel-bin/external/com_cloudflare_lol_html/_virtual_includes/lolhtml +-Ibazel-bin/external/+_repo_rules+com_cloudflare_lol_html/_virtual_includes/lolhtml -Ibazel-bin/external/perfetto+/ -Iexternal/ada-url+/ -Iexternal/abseil-cpp+/ -Iexternal/+http+simdutf/ --Iexternal/+nbytes+nbytes/include/ --Iexternal/codspeed/google_benchmark/include/ +-Iexternal/+http+nbytes/include/ +-Iexternal/google_benchmark++_repo_rules+codspeed/google_benchmark/include/ -Iexternal/perfetto+/include/ -Iexternal/perfetto+/include/perfetto/base/build_configs/bazel/ -Iexternal/boringssl+/include --Iexternal/+ncrypto+ncrypto/include +-Iexternal/+http+ncrypto/include -isystembazel-bin/external/sqlite3+ -Isrc -isystem/usr/lib/llvm-21/include/c++/v1 @@ -40,30 +40,27 @@ -isystembazel-bin/external/+http+capnp-cpp/src/kj/compat/_virtual_includes/kj-gzip -isystembazel-bin/external/+http+capnp-cpp/src/kj/compat/_virtual_includes/kj-http -isystembazel-bin/external/+http+capnp-cpp/src/kj/compat/_virtual_includes/kj-tls --isystembazel-bin/external/workerd-cxx/_virtual_includes/core/ --isystembazel-bin/external/workerd-cxx/kj-rs/_virtual_includes/kj-rs-lib/ --isystembazel-bin/external/+_repo_rules+v8 --isystembazel-bin/external/+_repo_rules+v8/icu +-isystembazel-bin/external/+http+workerd-cxx/_virtual_includes/core/ +-isystembazel-bin/external/+http+workerd-cxx/kj-rs/_virtual_includes/kj-rs-lib/ +-isystemexternal/+_repo_rules2+v8/include +-isystemexternal/+_repo_rules2+v8/include/cppgc -isystembazel-bin/src -isystemexternal/brotli+/c/include --isystemexternal/+_repo_rules2+com_googlesource_chromium_icu/source/common +-isystemexternal/+_repo_rules3+com_googlesource_chromium_icu/source/common -isystemexternal/zlib+ --isystembazel-bin/src/rust/cxx-integration/_virtual_includes/cxx-include/ --isystembazel-bin/src/rust/cxx-integration/_virtual_includes/cxx-integration@cxx --isystembazel-bin/src/rust/cxx-integration-test/_virtual_includes/cxx-integration-test@cxx +-isystembazel-bin/src/rust/cxx-integration/_virtual_includes/lib.rs@cxx -isystembazel-bin/src/rust/cxx-integration-test/_virtual_includes/lib.rs@cxx +-isystembazel-bin/src/rust/dns/_virtual_includes/lib.rs@cxx -isystembazel-bin/src/rust/kj/_virtual_includes/ffi -isystembazel-bin/src/rust/kj/_virtual_includes/http.rs@cxx -isystembazel-bin/src/rust/kj/_virtual_includes/io.rs@cxx -isystembazel-bin/src/rust/kj/tests/_virtual_includes/lib.rs@cxx --isystembazel-bin/src/rust/python-parser/_virtual_includes/python-parser@cxx --isystembazel-bin/src/rust/net/_virtual_includes/net@cxx --isystembazel-bin/src/rust/transpiler/_virtual_includes/transpiler@cxx +-isystembazel-bin/src/rust/python-parser/_virtual_includes/lib.rs@cxx +-isystembazel-bin/src/rust/net/_virtual_includes/lib.rs@cxx +-isystembazel-bin/src/rust/transpiler/_virtual_includes/lib.rs@cxx -isystembazel-bin/src/rust/api/_virtual_includes/lib.rs@cxx --isystembazel-bin/src/rust/jsg/_virtual_includes/bridge -isystembazel-bin/src/rust/jsg/_virtual_includes/ffi -isystembazel-bin/src/rust/jsg/_virtual_includes/lib.rs@cxx --isystembazel-bin/src/rust/jsg/_virtual_includes/modules.rs@cxx -isystembazel-bin/src/rust/jsg/_virtual_includes/v8.rs@cxx -isystembazel-bin/src/rust/jsg-test/_virtual_includes/ffi-hdrs -isystembazel-bin/src/rust/jsg-test/_virtual_includes/lib.rs@cxx From 9b96eed51af0f0e4927d1e88e8d905e7e05041f3 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 7 Jan 2026 10:55:57 -0500 Subject: [PATCH 8/9] add support for &str as jsg method parameter --- src/rust/jsg-macros/lib.rs | 14 +++++++++--- src/rust/jsg-test/tests/resource_callback.rs | 24 ++++++++++++++++++++ src/rust/jsg/wrappable.rs | 22 ++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index 89801940760..cd4d1220234 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -105,7 +105,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { }) .collect(); - let (unwraps, arg_names): (Vec<_>, Vec<_>) = params + let (unwraps, arg_exprs): (Vec<_>, Vec<_>) = params .iter() .enumerate() .map(|(i, ty)| { @@ -113,7 +113,15 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let unwrap = quote! { let Ok(#arg) = <#ty as jsg::FromJS>::from_js(&mut lock, args.get(#i)) else { return; }; }; - (unwrap, arg) + // For reference types (like &str), FromJS returns an owned type (String), + // so we need to borrow it when passing to the function. + let is_ref = matches!(ty.as_ref(), syn::Type::Reference(_)); + let arg_expr = if is_ref { + quote! { &#arg } + } else { + quote! { #arg } + }; + (unwrap, arg_expr) }) .unzip(); @@ -127,7 +135,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { #(#unwraps)* let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); - let result = self_.#fn_name(#(#arg_names),*); + let result = self_.#fn_name(#(#arg_exprs),*); args.set_return_value(jsg::ToJS::to_js(result, &mut lock)); } } diff --git a/src/rust/jsg-test/tests/resource_callback.rs b/src/rust/jsg-test/tests/resource_callback.rs index 8529c1e86db..4961f8b4401 100644 --- a/src/rust/jsg-test/tests/resource_callback.rs +++ b/src/rust/jsg-test/tests/resource_callback.rs @@ -26,6 +26,11 @@ impl EchoResource { pub fn echo(&self, message: String) -> Result { Ok(format!("{}{}", self.prefix, message)) } + + #[jsg_method] + pub fn greet(&self, name: &str) -> String { + format!("{}{}!", self.prefix, name) + } } #[jsg_resource] @@ -109,6 +114,25 @@ fn resource_method_can_be_called_multiple_times() { }); } +/// Validates that methods can accept &str parameters. +#[test] +fn resource_method_accepts_str_ref_parameter() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let resource = jsg::Ref::new(EchoResource { + _state: ResourceState::default(), + prefix: "Hello, ".to_owned(), + }); + let mut template = EchoResourceTemplate::new(lock); + let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; + ctx.set_global("echo", wrapped); + + let result: String = ctx.eval(lock, "echo.greet('World')")?; + assert_eq!(result, "Hello, World!"); + Ok(()) + }); +} + /// Validates that methods can return values directly without Result wrapper. #[test] fn resource_method_returns_non_result_values() { diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 66e941dbefa..6a776359ee6 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -10,6 +10,7 @@ //! |-----------|-----------------| //! | `()` | `undefined` | //! | `String` | `string` | +//! | `&str` | `string` | //! | `bool` | `boolean` | //! | `f64` | `number` | //! | `Option` | `T` or `undefined` | @@ -97,6 +98,27 @@ impl_primitive!(String, "string", is_string, unwrap_string); impl_primitive!(bool, "boolean", is_boolean, unwrap_boolean); impl_primitive!(f64, "number", is_number, unwrap_number); +// Special implementation for &str - allows functions to accept &str parameters +// by converting JavaScript strings to owned Strings, then borrowing. +// The macro handles passing &arg instead of arg for reference types.zs +impl Type for &str { + fn class_name() -> &'static str { + "string" + } + + fn is_exact(value: &v8::Local) -> bool { + value.is_string() + } +} + +impl FromJS for &str { + type ResultType = String; + + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { + Ok(unsafe { v8::ffi::unwrap_string(lock.isolate().as_ffi(), value.into_ffi()) }) + } +} + // ============================================================================= // Wrapper type implementations // ============================================================================= From 123ae7ebfb476decafbd5c14ec9399f112ad986c Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 7 Jan 2026 13:19:22 -0500 Subject: [PATCH 9/9] simplify result handling and errors --- src/rust/api/dns.rs | 6 +- src/rust/jsg-macros/lib.rs | 36 +++++- src/rust/jsg-test/lib.rs | 47 +++++-- src/rust/jsg-test/tests/eval.rs | 56 +++++---- src/rust/jsg-test/tests/jsg_struct.rs | 10 +- src/rust/jsg-test/tests/non_coercible.rs | 42 ++++--- src/rust/jsg-test/tests/resource_callback.rs | 22 ++-- src/rust/jsg/ffi.c++ | 6 +- src/rust/jsg/lib.rs | 126 +++++++++++++------ src/rust/jsg/v8.rs | 49 ++++++-- src/rust/jsg/wrappable.rs | 43 +------ 11 files changed, 281 insertions(+), 162 deletions(-) diff --git a/src/rust/api/dns.rs b/src/rust/api/dns.rs index 1a33ba3d0e5..8dd24f9e67f 100644 --- a/src/rust/api/dns.rs +++ b/src/rust/api/dns.rs @@ -20,10 +20,10 @@ impl From for jsg::Error { fn from(val: DnsParserError) -> Self { match val { DnsParserError::InvalidHexString(msg) | DnsParserError::InvalidDnsResponse(msg) => { - Self::new("Error", msg) + Self::new_error(&msg) } - DnsParserError::ParseIntError(msg) => Self::new("RangeError", msg.to_string()), - DnsParserError::Unknown => Self::new("Error", "Unknown dns parser error".to_owned()), + DnsParserError::ParseIntError(msg) => Self::new_range_error(msg.to_string()), + DnsParserError::Unknown => Self::new_error("Unknown dns parser error"), } } } diff --git a/src/rust/jsg-macros/lib.rs b/src/rust/jsg-macros/lib.rs index cd4d1220234..46b486d43bb 100644 --- a/src/rust/jsg-macros/lib.rs +++ b/src/rust/jsg-macros/lib.rs @@ -111,7 +111,13 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { .map(|(i, ty)| { let arg = syn::Ident::new(&format!("arg{i}"), fn_name.span()); let unwrap = quote! { - let Ok(#arg) = <#ty as jsg::FromJS>::from_js(&mut lock, args.get(#i)) else { return; }; + let #arg = match <#ty as jsg::FromJS>::from_js(&mut lock, args.get(#i)) { + Ok(v) => v, + Err(err) => { + lock.throw_exception(&err); + return; + } + }; }; // For reference types (like &str), FromJS returns an owned type (String), // so we need to borrow it when passing to the function. @@ -125,6 +131,22 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { }) .unzip(); + // Check if return type is Result + let is_result = matches!(&fn_sig.output, syn::ReturnType::Type(_, ty) if is_result_type(ty)); + + let result_handling = if is_result { + quote! { + match result { + Ok(value) => args.set_return_value(jsg::ToJS::to_js(value, &mut lock)), + Err(err) => lock.throw_exception(&err.into()), + } + } + } else { + quote! { + args.set_return_value(jsg::ToJS::to_js(result, &mut lock)); + } + }; + quote! { #fn_vis #fn_sig { #fn_block } @@ -136,7 +158,7 @@ pub fn jsg_method(_attr: TokenStream, item: TokenStream) -> TokenStream { let this = args.this(); let self_ = jsg::unwrap_resource::(&mut lock, this); let result = self_.#fn_name(#(#arg_exprs),*); - args.set_return_value(jsg::ToJS::to_js(result, &mut lock)); + #result_handling } } .into() @@ -314,3 +336,13 @@ fn snake_to_camel(s: &str) -> String { } result } + +/// Checks if a type is `Result`. +fn is_result_type(ty: &syn::Type) -> bool { + if let syn::Type::Path(type_path) = ty + && let Some(segment) = type_path.path.segments.last() + { + return segment.ident == "Result"; + } + false +} diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index e1251c4975f..4622a36d9e8 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -1,5 +1,6 @@ use std::pin::Pin; +use jsg::FromJS; use jsg::v8; use kj_rs::KjOwn; @@ -47,9 +48,33 @@ pub struct EvalContext<'a> { isolate: v8::IsolatePtr, } +#[derive(Debug)] +pub enum EvalError<'a> { + UncoercibleResult { + value: v8::Local<'a, v8::Value>, + message: String, + }, + Exception(v8::Local<'a, v8::Value>), + EvalFailed, +} + +impl EvalError<'_> { + /// Extracts a `jsg::Error` from an `EvalError::Exception` variant. + /// + /// # Panics + /// + /// Panics if `self` is not `EvalError::Exception`, or if the value cannot be converted to a + /// `jsg::Error`. + pub fn unwrap_jsg_err(&self, lock: &mut jsg::Lock) -> jsg::Error { + match self { + EvalError::Exception(value) => jsg::Error::from_js(lock, value.clone()) + .expect("Failed to convert exception to jsg::Error"), + _ => panic!("Unexpected error"), + } + } +} impl EvalContext<'_> { - // TODO(soon): There is no way to distinguish a throw versus an error as a return value. - pub fn eval(&self, lock: &mut jsg::Lock, code: &str) -> Result + pub fn eval(&self, lock: &mut jsg::Lock, code: &str) -> Result> where T: jsg::FromJS, { @@ -59,20 +84,24 @@ impl EvalContext<'_> { if result.success { match opt_local { Some(local) => { - T::from_js(lock, unsafe { v8::Local::from_ffi(self.isolate, local) }) + let local = unsafe { v8::Local::from_ffi(self.isolate, local) }; + match T::from_js(lock, local.clone()) { + Err(e) => Err(EvalError::UncoercibleResult { + value: local, + message: e.to_string(), + }), + Ok(value) => Ok(value), + } } - None => Err(jsg::Error::new( - "Error", - "eval returned empty result".to_owned(), - )), + None => unreachable!(), } } else { match opt_local { Some(local) => { let value = unsafe { v8::Local::from_ffi(self.isolate, local) }; - Err(jsg::Error::from_value(lock, value)) + Err(EvalError::Exception(value)) } - None => Err(jsg::Error::new("Error", "eval failed".to_owned())), + None => Err(EvalError::EvalFailed), } } } diff --git a/src/rust/jsg-test/tests/eval.rs b/src/rust/jsg-test/tests/eval.rs index fbaae79131f..0c9bea7e515 100644 --- a/src/rust/jsg-test/tests/eval.rs +++ b/src/rust/jsg-test/tests/eval.rs @@ -1,8 +1,10 @@ +use crate::EvalError; + #[test] fn eval_returns_correct_type() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "'Hello, World!'")?; + let result: String = ctx.eval(lock, "'Hello, World!'").unwrap(); assert_eq!(result, "Hello, World!"); Ok(()) }); @@ -12,7 +14,7 @@ fn eval_returns_correct_type() { fn eval_string_concatenation() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "'Hello' + ', ' + 'World!'")?; + let result: String = ctx.eval(lock, "'Hello' + ', ' + 'World!'").unwrap(); assert_eq!(result, "Hello, World!"); Ok(()) }); @@ -22,7 +24,7 @@ fn eval_string_concatenation() { fn eval_number_returns_number_type() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: f64 = ctx.eval(lock, "42")?; + let result: f64 = ctx.eval(lock, "42").unwrap(); assert!((result - 42.0).abs() < f64::EPSILON); Ok(()) }); @@ -32,7 +34,7 @@ fn eval_number_returns_number_type() { fn eval_arithmetic_expression() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: f64 = ctx.eval(lock, "1 + 2 + 3")?; + let result: f64 = ctx.eval(lock, "1 + 2 + 3").unwrap(); assert!((result - 6.0).abs() < f64::EPSILON); Ok(()) }); @@ -42,7 +44,7 @@ fn eval_arithmetic_expression() { fn eval_boolean_true() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: bool = ctx.eval(lock, "true")?; + let result: bool = ctx.eval(lock, "true").unwrap(); assert!(result); Ok(()) }); @@ -52,7 +54,7 @@ fn eval_boolean_true() { fn eval_boolean_false() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: bool = ctx.eval(lock, "false")?; + let result: bool = ctx.eval(lock, "false").unwrap(); assert!(!result); Ok(()) }); @@ -62,7 +64,7 @@ fn eval_boolean_false() { fn eval_comparison_expression() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: bool = ctx.eval(lock, "5 > 3")?; + let result: bool = ctx.eval(lock, "5 > 3").unwrap(); assert!(result); Ok(()) }); @@ -72,7 +74,7 @@ fn eval_comparison_expression() { fn eval_null() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: jsg::Nullable = ctx.eval(lock, "null")?; + let result: jsg::Nullable = ctx.eval(lock, "null").unwrap(); assert!(result.is_null()); Ok(()) }); @@ -82,11 +84,13 @@ fn eval_null() { fn eval_throws_on_error() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: Result = ctx.eval(lock, "throw new Error('test error')"); - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!(err.name, "Error"); - assert_eq!(err.message, "test error"); + let result = ctx.eval::(lock, "throw new Error('test error')"); + match result.unwrap_err() { + EvalError::Exception(value) => { + assert_eq!(value.to_string(), "Error: test error"); + } + _ => panic!("Unexpected error type"), + } Ok(()) }); } @@ -95,11 +99,13 @@ fn eval_throws_on_error() { fn eval_throws_string_preserves_message() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: Result = ctx.eval(lock, "throw 'custom string error'"); - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!(err.name, "Error"); - assert_eq!(err.message, "custom string error"); + let result = ctx.eval::(lock, "throw 'custom string error'"); + match result.unwrap_err() { + EvalError::Exception(value) => { + assert_eq!(value.to_string(), "custom string error"); + } + _ => panic!("Unexpected error type"), + } Ok(()) }); } @@ -108,7 +114,9 @@ fn eval_throws_string_preserves_message() { fn eval_function_call() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "(function() { return 'from function'; })()")?; + let result: String = ctx + .eval(lock, "(function() { return 'from function'; })()") + .unwrap(); assert_eq!(result, "from function"); Ok(()) }); @@ -118,7 +126,7 @@ fn eval_function_call() { fn eval_typeof_string() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "typeof 'hello'")?; + let result: String = ctx.eval(lock, "typeof 'hello'").unwrap(); assert_eq!(result, "string"); Ok(()) }); @@ -128,7 +136,7 @@ fn eval_typeof_string() { fn eval_typeof_number() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "typeof 42")?; + let result: String = ctx.eval(lock, "typeof 42").unwrap(); assert_eq!(result, "number"); Ok(()) }); @@ -138,7 +146,7 @@ fn eval_typeof_number() { fn eval_typeof_boolean() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "typeof true")?; + let result: String = ctx.eval(lock, "typeof true").unwrap(); assert_eq!(result, "boolean"); Ok(()) }); @@ -148,7 +156,7 @@ fn eval_typeof_boolean() { fn eval_unicode_string() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "'こんにちは'")?; + let result: String = ctx.eval(lock, "'こんにちは'").unwrap(); assert_eq!(result, "こんにちは"); Ok(()) }); @@ -158,7 +166,7 @@ fn eval_unicode_string() { fn eval_emoji_string() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: String = ctx.eval(lock, "'😀🎉'")?; + let result: String = ctx.eval(lock, "'😀🎉'").unwrap(); assert_eq!(result, "😀🎉"); Ok(()) }); diff --git a/src/rust/jsg-test/tests/jsg_struct.rs b/src/rust/jsg-test/tests/jsg_struct.rs index 54313c0b64c..211ffda451d 100644 --- a/src/rust/jsg-test/tests/jsg_struct.rs +++ b/src/rust/jsg-test/tests/jsg_struct.rs @@ -157,12 +157,8 @@ fn nested_object_properties() { #[test] fn error_creation_and_display() { - let error = Error::default(); - assert_eq!(error.name, "Error"); - assert_eq!(error.message, "An unknown error occurred"); - - let type_error = Error::new("TypeError", "Invalid type".to_owned()); - assert_eq!(type_error.name, "TypeError"); + let type_error = Error::new_type_error("Invalid type"); + assert_eq!(type_error.name, ExceptionType::TypeError); assert_eq!(type_error.message, "Invalid type"); // Test Display for all exception types @@ -178,7 +174,7 @@ fn error_creation_and_display() { fn error_from_parse_int_error() { let parse_result: Result = "not_a_number".parse(); let error: Error = parse_result.unwrap_err().into(); - assert_eq!(error.name, "TypeError"); + assert_eq!(error.name, ExceptionType::RangeError); assert!(error.message.contains("Failed to parse integer")); } diff --git a/src/rust/jsg-test/tests/non_coercible.rs b/src/rust/jsg-test/tests/non_coercible.rs index d0fac73a921..f64ad37c20e 100644 --- a/src/rust/jsg-test/tests/non_coercible.rs +++ b/src/rust/jsg-test/tests/non_coercible.rs @@ -1,3 +1,4 @@ +use jsg::ExceptionType; use jsg::NonCoercible; use jsg::ResourceState; use jsg::ResourceTemplate; @@ -13,17 +14,17 @@ struct MyResource { #[expect(clippy::unnecessary_wraps)] impl MyResource { #[jsg_method] - pub fn string(&self, nc: NonCoercible) -> Result { + pub fn string(&self, nc: NonCoercible) -> Result { Ok(nc.as_ref().clone()) } #[jsg_method] - pub fn boolean(&self, nc: NonCoercible) -> Result { + pub fn boolean(&self, nc: NonCoercible) -> Result { Ok(*nc.as_ref()) } #[jsg_method] - pub fn number(&self, nc: NonCoercible) -> Result { + pub fn number(&self, nc: NonCoercible) -> Result { Ok(*nc.as_ref()) } } @@ -132,36 +133,39 @@ fn non_coercible_methods_accept_correct_types_and_reject_incorrect_types() { ctx.set_global("resource", wrapped); // String method accepts string - let result: String = ctx.eval(lock, "resource.string('hello')")?; + let result: String = ctx.eval(lock, "resource.string('hello')").unwrap(); assert_eq!(result, "hello"); // String method rejects number - let result: Result = ctx.eval(lock, "resource.string(123)"); - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!(err.name, "TypeError"); + let err = ctx + .eval::(lock, "resource.string(123)") + .unwrap_err() + .unwrap_jsg_err(lock); + assert_eq!(err.name, ExceptionType::TypeError); assert!(err.message.contains("string")); // Boolean method accepts boolean - let result: bool = ctx.eval(lock, "resource.boolean(true)")?; + let result: bool = ctx.eval(lock, "resource.boolean(true)").unwrap(); assert!(result); // Boolean method rejects string - let result: Result = ctx.eval(lock, "resource.boolean('true')"); - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!(err.name, "TypeError"); - assert!(err.message.contains("boolean")); + let err = ctx + .eval::(lock, "resource.boolean('true')") + .unwrap_err() + .unwrap_jsg_err(lock); + assert_eq!(err.name, ExceptionType::TypeError); + assert!(err.message.contains("bool")); // Number method accepts number - let result: f64 = ctx.eval(lock, "resource.number(42.5)")?; + let result: f64 = ctx.eval(lock, "resource.number(42.5)").unwrap(); assert!((result - 42.5).abs() < f64::EPSILON); // Number method rejects string - let result: Result = ctx.eval(lock, "resource.number('42')"); - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!(err.name, "TypeError"); + let err = ctx + .eval::(lock, "resource.number('42')") + .unwrap_err() + .unwrap_jsg_err(lock); + assert_eq!(err.name, ExceptionType::TypeError); assert!(err.message.contains("number")); Ok(()) }); diff --git a/src/rust/jsg-test/tests/resource_callback.rs b/src/rust/jsg-test/tests/resource_callback.rs index 4961f8b4401..af99fa761b0 100644 --- a/src/rust/jsg-test/tests/resource_callback.rs +++ b/src/rust/jsg-test/tests/resource_callback.rs @@ -23,7 +23,7 @@ struct EchoResource { #[expect(clippy::unnecessary_wraps)] impl EchoResource { #[jsg_method] - pub fn echo(&self, message: String) -> Result { + pub fn echo(&self, message: String) -> Result { Ok(format!("{}{}", self.prefix, message)) } @@ -84,7 +84,7 @@ fn resource_method_callback_receives_correct_self() { ctx.set_global("echoResource", wrapped); // Call the method from JavaScript - let result: String = ctx.eval(lock, "echoResource.echo('World!')")?; + let result: String = ctx.eval(lock, "echoResource.echo('World!')").unwrap(); assert_eq!(result, "Hello, World!"); Ok(()) }); @@ -104,11 +104,11 @@ fn resource_method_can_be_called_multiple_times() { ctx.set_global("echo", wrapped); // First call - let result: String = ctx.eval(lock, "echo.echo('first')")?; + let result: String = ctx.eval(lock, "echo.echo('first')").unwrap(); assert_eq!(result, ">> first"); // Second call - let result: String = ctx.eval(lock, "echo.echo('second')")?; + let result: String = ctx.eval(lock, "echo.echo('second')").unwrap(); assert_eq!(result, ">> second"); Ok(()) }); @@ -127,7 +127,7 @@ fn resource_method_accepts_str_ref_parameter() { let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; ctx.set_global("echo", wrapped); - let result: String = ctx.eval(lock, "echo.greet('World')")?; + let result: String = ctx.eval(lock, "echo.greet('World')").unwrap(); assert_eq!(result, "Hello, World!"); Ok(()) }); @@ -149,23 +149,23 @@ fn resource_method_returns_non_result_values() { ctx.set_global("resource", wrapped); // Test getString returns string - let result: String = ctx.eval(lock, "resource.getName()")?; + let result: String = ctx.eval(lock, "resource.getName()").unwrap(); assert_eq!(result, "TestResource"); // Test isValid returns boolean - let result: bool = ctx.eval(lock, "resource.isValid()")?; + let result: bool = ctx.eval(lock, "resource.isValid()").unwrap(); assert!(result); // Test getCounter returns number - let result: f64 = ctx.eval(lock, "resource.getCounter()")?; + let result: f64 = ctx.eval(lock, "resource.getCounter()").unwrap(); assert!((result - 42.0).abs() < f64::EPSILON); // Test incrementCounter returns undefined (we just check it doesn't error) - let _: Option = ctx.eval(lock, "resource.incrementCounter()")?; + let _: Option = ctx.eval(lock, "resource.incrementCounter()").unwrap(); assert_eq!(counter.get(), 43); // Test maybeName returns string for Some - let result: String = ctx.eval(lock, "resource.maybeName()")?; + let result: String = ctx.eval(lock, "resource.maybeName()").unwrap(); assert_eq!(result, "TestResource"); Ok(()) }); @@ -185,7 +185,7 @@ fn resource_method_returns_null_for_none() { let wrapped = unsafe { jsg::wrap_resource(lock, resource, &mut template) }; ctx.set_global("resource", wrapped); - let result: Option = ctx.eval(lock, "resource.maybeName()")?; + let result: Option = ctx.eval(lock, "resource.maybeName()").unwrap(); assert!(result.is_none()); Ok(()) }); diff --git a/src/rust/jsg/ffi.c++ b/src/rust/jsg/ffi.c++ index 16e7995e824..817d9c21801 100644 --- a/src/rust/jsg/ffi.c++ +++ b/src/rust/jsg/ffi.c++ @@ -327,10 +327,10 @@ Local exception_create(Isolate* isolate, ExceptionType exception_type, ::rust::S return to_ffi(v8::Exception::SyntaxError(message)); case ExceptionType::TypeError: return to_ffi(v8::Exception::TypeError(message)); - case ExceptionType::Error: - return to_ffi(v8::Exception::Error(message)); default: - KJ_UNREACHABLE; + // DOM-style exceptions (OperationError, DataError, etc.) and Error fall back to Error. + // TODO(soon): Use js.domException() to create proper DOMException objects. + return to_ffi(v8::Exception::Error(message)); } } diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index adee939027b..0f8219ec2e3 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -142,9 +142,32 @@ pub fn unwrap_resource<'a, R: Resource>( unsafe { &mut *ptr } } -#[derive(Debug)] +impl From<&str> for ExceptionType { + fn from(value: &str) -> Self { + match value { + "OperationError" => Self::OperationError, + "DataError" => Self::DataError, + "DataCloneError" => Self::DataCloneError, + "InvalidAccessError" => Self::InvalidAccessError, + "InvalidStateError" => Self::InvalidStateError, + "InvalidCharacterError" => Self::InvalidCharacterError, + "NotSupportedError" => Self::NotSupportedError, + "SyntaxError" => Self::SyntaxError, + "TimeoutError" => Self::TimeoutError, + "TypeMismatchError" => Self::TypeMismatchError, + "AbortError" => Self::AbortError, + "NotFoundError" => Self::NotFoundError, + "TypeError" => Self::TypeError, + "RangeError" => Self::RangeError, + "ReferenceError" => Self::ReferenceError, + _ => Self::Error, + } + } +} + +#[derive(Debug, Clone)] pub struct Error { - pub name: String, + pub name: ExceptionType, pub message: String, } @@ -154,73 +177,94 @@ impl std::fmt::Display for Error { } } -impl Error { - pub fn new(name: impl Into, message: String) -> Self { - Self { - name: name.into(), - message, +/// Generates constructor methods for each `ExceptionType` variant. +/// e.g., `new_type_error("message")` creates an Error with `ExceptionType::TypeError` +macro_rules! impl_error_constructors { + ($($variant:ident => $fn_name:ident),* $(,)?) => { + impl Error { + $( + pub fn $fn_name(message: impl Into) -> Self { + Self { + name: ExceptionType::$variant, + message: message.into(), + } + } + )* } - } + }; +} + +impl_error_constructors! { + OperationError => new_operation_error, + DataError => new_data_error, + DataCloneError => new_data_clone_error, + InvalidAccessError => new_invalid_access_error, + InvalidStateError => new_invalid_state_error, + InvalidCharacterError => new_invalid_character_error, + NotSupportedError => new_not_supported_error, + SyntaxError => new_syntax_error, + TimeoutError => new_timeout_error, + TypeMismatchError => new_type_mismatch_error, + AbortError => new_abort_error, + NotFoundError => new_not_found_error, + TypeError => new_type_error, + Error => new_error, + RangeError => new_range_error, + ReferenceError => new_reference_error, +} + +impl FromJS for Error { + type ResultType = Self; /// Creates an Error from a V8 value (typically an exception). /// /// If the value is a native error, extracts the name and message properties. /// Otherwise, converts the value to a string for the message. - pub fn from_value(lock: &mut Lock, value: v8::Local) -> Self { + fn from_js(lock: &mut Lock, value: v8::Local) -> Result { if value.is_native_error() { let obj: v8::Local = value.into(); let name = obj .get(lock, "name") - .and_then(|v| String::from_js(lock, v).ok()) - .unwrap_or_else(|| "Error".to_owned()); + .and_then(|v| String::from_js(lock, v).ok()); let message = obj .get(lock, "message") .and_then(|v| String::from_js(lock, v).ok()) .unwrap_or_else(|| "Unknown error".to_owned()); - Self { name, message } - } else { - let message = - String::from_js(lock, value).unwrap_or_else(|_| "Unknown error".to_owned()); - Self { - name: "Error".to_owned(), + Ok(Self { + name: name.map_or(ExceptionType::Error, |n| ExceptionType::from(n.as_str())), message, - } + }) + } else { + Err(Self::new_type_error("Unknown error")) + } + } +} + +impl Error { + pub fn new(name: &str, message: &str) -> Self { + Self { + name: ExceptionType::from(name), + message: message.to_owned(), } } /// Creates a V8 exception from this error. pub fn to_local<'a>(&self, isolate: v8::IsolatePtr) -> v8::Local<'a, v8::Value> { - let exception_type = match self.name.as_str() { - "TypeError" => v8::ffi::ExceptionType::TypeError, - "RangeError" => v8::ffi::ExceptionType::RangeError, - "ReferenceError" => v8::ffi::ExceptionType::ReferenceError, - "SyntaxError" => v8::ffi::ExceptionType::SyntaxError, - _ => v8::ffi::ExceptionType::Error, - }; unsafe { v8::Local::from_ffi( isolate, - v8::ffi::exception_create(isolate.as_ffi(), exception_type, &self.message), + v8::ffi::exception_create(isolate.as_ffi(), self.name, &self.message), ) } } } -impl Default for Error { - fn default() -> Self { - Self { - name: "Error".to_owned(), - message: "An unknown error occurred".to_owned(), - } - } -} - impl From for Error { fn from(err: ParseIntError) -> Self { - Self::new("TypeError", format!("Failed to parse integer: {err}")) + Self::new_range_error(format!("Failed to parse integer: {err}")) } } @@ -439,6 +483,16 @@ impl Lock { fn realm(&mut self) -> &mut Realm { unsafe { &mut *crate::ffi::realm_from_isolate(self.isolate().as_ffi()) } } + + /// Throws an error as a V8 exception. + pub fn throw_exception(&mut self, err: &Error) { + unsafe { + v8::ffi::isolate_throw_exception( + self.isolate().as_ffi(), + err.to_local(self.isolate()).into_ffi(), + ); + } + } } /// This is analogous to `jsg::Ref` in C++ JSG. diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 31e6b5dbeaa..94c4c40446d 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -1,7 +1,9 @@ use core::ffi::c_void; +use std::fmt::Display; use std::marker::PhantomData; use std::ptr::NonNull; +use crate::FromJS; use crate::Lock; #[expect(clippy::missing_safety_doc)] @@ -17,13 +19,24 @@ pub mod ffi { ptr: usize, } - #[derive(Debug)] - enum ExceptionType { - RangeError, - ReferenceError, + #[derive(Debug, PartialEq, Eq, Copy, Clone)] + pub enum ExceptionType { + OperationError, + DataError, + DataCloneError, + InvalidAccessError, + InvalidStateError, + InvalidCharacterError, + NotSupportedError, SyntaxError, + TimeoutError, + TypeMismatchError, + AbortError, + NotFoundError, TypeError, Error, + RangeError, + ReferenceError, } /// Module visibility level, corresponds to `workerd::jsg::ModuleType` from modules.capnp. @@ -177,12 +190,22 @@ pub mod ffi { impl std::fmt::Display for ffi::ExceptionType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let name = match *self { - Self::RangeError => "RangeError", - Self::ReferenceError => "ReferenceError", + Self::OperationError => "OperationError", + Self::DataError => "DataError", + Self::DataCloneError => "DataCloneError", + Self::InvalidAccessError => "InvalidAccessError", + Self::InvalidStateError => "InvalidStateError", + Self::InvalidCharacterError => "InvalidCharacterError", + Self::NotSupportedError => "NotSupportedError", Self::SyntaxError => "SyntaxError", + Self::TimeoutError => "TimeoutError", + Self::TypeMismatchError => "TypeMismatchError", + Self::AbortError => "AbortError", + Self::NotFoundError => "NotFoundError", Self::TypeError => "TypeError", - Self::Error => "Error", - _ => unreachable!(), + Self::RangeError => "RangeError", + Self::ReferenceError => "ReferenceError", + _ => "Error", }; write!(f, "{name}") } @@ -191,6 +214,16 @@ impl std::fmt::Display for ffi::ExceptionType { // Marker types for Local #[derive(Debug)] pub struct Value; + +impl Display for Local<'_, Value> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut lock = unsafe { Lock::from_isolate_ptr(self.isolate.as_ffi()) }; + match String::from_js(&mut lock, self.clone()) { + Ok(value) => write!(f, "{value}"), + Err(e) => write!(f, "{e:?}"), + } + } +} #[derive(Debug)] pub struct Object; pub struct FunctionTemplate; diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 6a776359ee6..c2e271e18b6 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -19,8 +19,6 @@ //! | `NonCoercible` | `T` (strict type checking) | //! | `T: Struct` | `object` | -use std::fmt::Display; - use crate::Error; use crate::Lock; use crate::NonCoercible; @@ -132,23 +130,6 @@ impl ToJS for () { } } -impl ToJS for Result { - fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> - where - 'b: 'a, - { - match self { - Ok(value) => value.to_js(lock), - Err(err) => { - // TODO(soon): Use Error trait to dynamically call proper method to throw the error. - let description = err.to_string(); - unsafe { v8::ffi::isolate_throw_error(lock.isolate().as_ffi(), &description) }; - v8::Local::::undefined(lock) - } - } - } -} - impl ToJS for Option { fn to_js<'a, 'b>(self, lock: &'a mut Lock) -> v8::Local<'b, v8::Value> where @@ -188,17 +169,8 @@ impl FromJS for Option { fn from_js(lock: &mut Lock, value: v8::Local) -> Result { if value.is_null() { - let error_msg = format!("Expected {} or undefined but got null", T::class_name()); - // TODO(soon): Throw errors in the correct place. This function should return - // a result and the caller should handle the error. - let err = Error::new("TypeError", error_msg); - unsafe { - v8::ffi::isolate_throw_exception( - lock.isolate().as_ffi(), - err.to_local(lock.isolate()).into_ffi(), - ); - } - Err(err) + let msg = format!("Expected {} or undefined but got null", T::class_name()); + Err(Error::new_type_error(msg)) } else if value.is_undefined() { Ok(None) } else { @@ -217,16 +189,7 @@ impl FromJS for NonCoercible { T::class_name(), value.type_of() ); - // TODO(soon): Throw errors in the correct place. This function should return - // a result and the caller should handle the error. - let err = Error::new("TypeError", error_msg); - unsafe { - v8::ffi::isolate_throw_exception( - lock.isolate().as_ffi(), - err.to_local(lock.isolate()).into_ffi(), - ); - } - return Err(err); + return Err(Error::new_type_error(error_msg)); } Ok(::new(T::from_js(lock, value)?)) }