From c8cba42d123cc014afdd4ecb659581374f6a7038 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 26 Oct 2026 12:50:09 -0700 Subject: [PATCH 1/2] Make Rust JSG Nullable mean T | null jsg::Nullable had three states (Some, Null, Undefined), so a single type stood for T | null | undefined. That has no counterpart in C++ JSG or in TypeScript, and it means RTTI for a Nullable has to describe both alternatives even when an API only ever uses one. Nullable is now T | null, with Some and Null variants. A value that can also be undefined is written Option>, composing with Option, which is T | undefined. The three states stay distinct: undefined is None, null is Some(Nullable::Null). Converting undefined to a Nullable is now a TypeError, where C++ kj::Maybe reads it as empty. A method taking Nullable therefore needs null or a T, which matches how an omitted Nullable argument was already rejected. For Web IDL's T?, which reads undefined as null, take Option> and call unwrap_or(Nullable::Null). Rejecting undefined is the stricter choice; accepting it later would not break existing callers, while the reverse would. API changes: - Remove Nullable::Undefined, is_undefined(), is_null_or_undefined(), not_undefined() and coerce_null(). - not_null() returns the contained value instead of Self. - Lenient converts to and from Option> rather than Nullable, and to_parts() returns an Option>. Nothing outside the jsg tests uses Nullable yet. --- docs/reference/rust-review-checklist.md | 3 +- src/rust/jsg-test/tests/eval.rs | 10 +- src/rust/jsg-test/tests/gc.rs | 28 --- src/rust/jsg-test/tests/mod.rs | 1 + src/rust/jsg-test/tests/nullable.rs | 251 ++++++++++++++++++++++++ src/rust/jsg/lenient.rs | 37 ++-- src/rust/jsg/nullable.rs | 160 +++++---------- src/rust/jsg/v8.rs | 1 - src/rust/jsg/wrappable.rs | 13 +- 9 files changed, 339 insertions(+), 165 deletions(-) create mode 100644 src/rust/jsg-test/tests/nullable.rs diff --git a/docs/reference/rust-review-checklist.md b/docs/reference/rust-review-checklist.md index fa4040ea0aa..0f30ca57830 100644 --- a/docs/reference/rust-review-checklist.md +++ b/docs/reference/rust-review-checklist.md @@ -98,7 +98,8 @@ Rust types exposed to JavaScript via the JSG bindings follow these patterns: - **`#[jsg_oneof]`** is for union/variant types (mapped from JS values by trying each variant). - **Type mappings**: `jsg::Number` wraps JS numbers (distinct from `f64`). `Vec` maps to `Uint8Array`, not a regular JS `Array`. `Option` maps to `T | undefined`; like - `jsg::Optional`, `null` converts as `T`. `Nullable` maps to `T | null | undefined`. + `jsg::Optional`, `null` converts as `T`. `Nullable` maps to `T | null`; + use `Option>` for `T | null | undefined`. `String`/`&str` map to JS strings. Integers and `#[jsg_struct]`s convert like C++ JSG's integers and dictionaries: out-of-range integers throw, and an absent struct field reads as `undefined`. diff --git a/src/rust/jsg-test/tests/eval.rs b/src/rust/jsg-test/tests/eval.rs index a9d52e4b93f..c94007af7b1 100644 --- a/src/rust/jsg-test/tests/eval.rs +++ b/src/rust/jsg-test/tests/eval.rs @@ -116,8 +116,10 @@ fn eval_null() { fn eval_undefined() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let result: jsg::Nullable = ctx.eval(lock, "undefined").unwrap(); - assert!(result.is_undefined()); + let result: Option> = ctx.eval(lock, "undefined").unwrap(); + assert!(result.is_none()); + let result = ctx.eval::>(lock, "undefined"); + assert!(matches!(result, Err(EvalError::UncoercibleResult { .. }))); Ok(()) }); } @@ -312,8 +314,8 @@ fn lenient_string_number() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let result: jsg::Lenient = ctx.eval(lock, "42").unwrap(); - let result: jsg::Nullable = result.into(); - assert_eq!(&result.unwrap(), "42"); + let result: Option> = result.into(); + assert_eq!(result, Some(jsg::Nullable::Some("42".to_owned()))); Ok(()) }); } diff --git a/src/rust/jsg-test/tests/gc.rs b/src/rust/jsg-test/tests/gc.rs index 729e513489b..e3d519e201e 100644 --- a/src/rust/jsg-test/tests/gc.rs +++ b/src/rust/jsg-test/tests/gc.rs @@ -896,34 +896,6 @@ fn nullable_ref_null_does_not_crash_during_gc() { }); } -/// Tests that `Nullable>` with `Nullable::Undefined` doesn't cause issues during GC. -#[test] -fn nullable_ref_undefined_does_not_crash_during_gc() { - NULLABLE_PARENT_DROPS.store(0, Ordering::SeqCst); - - let harness = crate::Harness::new(); - harness.run_in_context(|lock, ctx| { - let parent = jsg::Rc::new(NullableParent { - child: jsg::Nullable::Undefined, - }); - let wrapped = parent.clone().to_js(lock)?; - ctx.set_global(lock, "parent", wrapped); - - std::mem::drop(parent); - - // GC with Nullable::Undefined should not crash - crate::Harness::request_gc(lock); - assert_eq!(NULLABLE_PARENT_DROPS.load(Ordering::SeqCst), 0); - Ok(()) - }); - - harness.run_in_context(|lock, _ctx| { - crate::Harness::request_gc(lock); - assert_eq!(NULLABLE_PARENT_DROPS.load(Ordering::SeqCst), 1); - Ok(()) - }); -} - // --------------------------------------------------------------------------- // Rc shared ownership — native object outlives Rc but is // dropped when V8 GC collects the JS wrapper. diff --git a/src/rust/jsg-test/tests/mod.rs b/src/rust/jsg-test/tests/mod.rs index c1382ea9a46..246c9435542 100644 --- a/src/rust/jsg-test/tests/mod.rs +++ b/src/rust/jsg-test/tests/mod.rs @@ -26,6 +26,7 @@ mod local_value; mod name; mod nested_type; mod non_coercible; +mod nullable; mod option; mod promise; mod resource_callback; diff --git a/src/rust/jsg-test/tests/nullable.rs b/src/rust/jsg-test/tests/nullable.rs new file mode 100644 index 00000000000..8f5ca37c773 --- /dev/null +++ b/src/rust/jsg-test/tests/nullable.rs @@ -0,0 +1,251 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +//! `Nullable` is `T | null`: converting `undefined` to it is a `TypeError`. +//! `Option>` is `T | null | undefined`, and keeps the three apart. + +use jsg::ExceptionType; +use jsg::FromJS; +use jsg::Nullable; +use jsg::ToJS; +use jsg::v8; +use jsg_macros::jsg_method; +use jsg_macros::jsg_resource; +use jsg_macros::jsg_struct; + +fn convert<'s, T: FromJS<'s>>( + lock: &mut jsg::Lock<'s>, + ctx: &crate::EvalContext<'_>, + expr: &str, +) -> Result { + let value = ctx + .eval_raw(lock, expr) + .expect("expression should evaluate"); + T::from_js(lock, value) +} + +/// Names which of the three states `value` is in. +fn describe(value: Option>) -> String { + match value { + Some(Nullable::Some(s)) => format!("some {s}"), + Some(Nullable::Null) => "null".to_owned(), + None => "undefined".to_owned(), + } +} + +/// Evaluates `expr` and returns its string, or `"Name: message"` for what it throws. +fn call(lock: &mut jsg::Lock, ctx: &crate::EvalContext<'_>, expr: &str) -> String { + let code = format!("try {{ {expr} }} catch (e) {{ `${{e.name}}: ${{e.message}}` }}"); + ctx.eval(lock, &code).unwrap() +} + +/// Converts `expr` through a `Lenient` to an `Option>`. +fn lenient_to_optional_nullable( + lock: &mut jsg::Lock, + ctx: &crate::EvalContext<'_>, + expr: &str, +) -> Option> { + let lenient = convert::>>(lock, ctx, expr).unwrap(); + Option::>::from(lenient).map(|n| n.map(jsg::NonCoercible::into_inner)) +} + +#[jsg_resource] +struct Args {} + +#[jsg_resource] +impl Args { + #[jsg_method] + pub fn take_nullable(&self, value: Nullable) -> String { + describe(Some(value)) + } + + #[jsg_method] + pub fn take_optional_nullable(&self, value: Option>) -> String { + describe(value) + } + + #[jsg_method] + pub fn give_null(&self) -> Nullable { + Nullable::Null + } + + #[jsg_method] + pub fn give_undefined(&self) -> Option> { + None + } +} + +#[jsg_struct] +#[derive(Debug, PartialEq)] +struct Fields { + pub required: Nullable, + pub optional: Option>, +} + +#[test] +fn nullable_reads_null_and_values() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + assert_eq!( + convert::>(lock, ctx, "null")?, + Nullable::Null + ); + assert_eq!( + convert::>(lock, ctx, "'x'")?, + Nullable::Some("x".to_owned()) + ); + Ok(()) + }); +} + +#[test] +fn nullable_rejects_undefined() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let err = convert::>(lock, ctx, "undefined").unwrap_err(); + assert_eq!(err.name, ExceptionType::TypeError); + assert_eq!(err.message, "Expected string or null but got undefined"); + // For `try_from_js()`, `undefined` is a top-level mismatch, not an error. + let undefined = ctx + .eval_raw(lock, "undefined") + .expect("expression should evaluate"); + assert_eq!( + as FromJS>::try_from_js(lock, undefined)?, + None + ); + Ok(()) + }); +} + +#[test] +fn optional_nullable_keeps_null_and_undefined_apart() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + assert_eq!( + convert::>>(lock, ctx, "undefined")?, + None + ); + assert_eq!( + convert::>>(lock, ctx, "null")?, + Some(Nullable::Null) + ); + assert_eq!( + convert::>>(lock, ctx, "'x'")?, + Some(Nullable::Some("x".to_owned())) + ); + Ok(()) + }); +} + +#[test] +fn nullable_to_js() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, _ctx| { + assert!(Nullable::::Null.to_js(lock)?.is_null()); + assert!(Nullable::Some("x".to_owned()).to_js(lock)?.is_string()); + assert!(None::>.to_js(lock)?.is_undefined()); + assert!(Some(Nullable::::Null).to_js(lock)?.is_null()); + Ok(()) + }); +} + +#[test] +fn nullable_argument_requires_null_or_a_value() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let args = jsg::Rc::new(Args {}).to_js(lock)?; + ctx.set_global(lock, "args", args); + let not_of_type = "TypeError: Failed to execute 'takeNullable' on 'Args': parameter 1 \ + is not of type 'string'."; + assert_eq!(call(lock, ctx, "args.takeNullable()"), not_of_type); + assert_eq!(call(lock, ctx, "args.takeNullable(undefined)"), not_of_type); + assert_eq!(call(lock, ctx, "args.takeNullable(null)"), "null"); + assert_eq!(call(lock, ctx, "args.takeNullable('x')"), "some x"); + + assert_eq!(call(lock, ctx, "args.takeOptionalNullable()"), "undefined"); + assert_eq!( + call(lock, ctx, "args.takeOptionalNullable(undefined)"), + "undefined" + ); + assert_eq!(call(lock, ctx, "args.takeOptionalNullable(null)"), "null"); + assert_eq!(call(lock, ctx, "args.takeOptionalNullable('x')"), "some x"); + + assert!( + ctx.eval::( + lock, + "args.giveNull() === null && args.giveUndefined() === undefined" + ) + .unwrap() + ); + Ok(()) + }); +} + +#[test] +fn nullable_struct_fields() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + assert_eq!( + convert::(lock, ctx, "({ required: null })")?, + Fields { + required: Nullable::Null, + optional: None, + } + ); + assert_eq!( + convert::(lock, ctx, "({ required: 'x', optional: null })")?, + Fields { + required: Nullable::Some("x".to_owned()), + optional: Some(Nullable::Null), + } + ); + let err = convert::(lock, ctx, "({})").unwrap_err(); + assert_eq!(err.message, "Expected string or null but got undefined"); + + let value = Fields { + required: Nullable::Null, + optional: None, + } + .to_js(lock)?; + let obj: v8::Local<'_, v8::Object> = value.try_into().unwrap(); + assert!(obj.get(lock, "required")?.is_null()); + assert!(!obj.has(lock, "optional")?); + + let value = Fields { + required: Nullable::Null, + optional: Some(Nullable::Null), + } + .to_js(lock)?; + let obj: v8::Local<'_, v8::Object> = value.try_into().unwrap(); + assert!(obj.get(lock, "optional")?.is_null()); + Ok(()) + }); +} + +#[test] +fn lenient_converts_to_optional_nullable() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + assert_eq!(lenient_to_optional_nullable(lock, ctx, "undefined"), None); + assert_eq!( + lenient_to_optional_nullable(lock, ctx, "null"), + Some(Nullable::Null) + ); + assert_eq!( + lenient_to_optional_nullable(lock, ctx, "'x'"), + Some(Nullable::Some("x".to_owned())) + ); + // An unconvertable value reads as undefined. + assert_eq!(lenient_to_optional_nullable(lock, ctx, "1"), None); + + let lenient = convert::>>(lock, ctx, "1")?; + let (value, unconvertable) = lenient.to_parts(); + assert!(value.is_none()); + assert!(unconvertable.is_some()); + + assert!(jsg::Lenient::::from(None::>).is_undefined_strict()); + assert!(jsg::Lenient::::from(Some(Nullable::::Null)).is_null()); + Ok(()) + }); +} diff --git a/src/rust/jsg/lenient.rs b/src/rust/jsg/lenient.rs index 22954b0b558..ebff1d4a660 100644 --- a/src/rust/jsg/lenient.rs +++ b/src/rust/jsg/lenient.rs @@ -9,15 +9,15 @@ use crate::v8::Value; /// [`Lenient`] is a type for lenient conversion from JavaScript, akin to the /// C++ type `LenientOptional`. /// -/// It is an alternative to [`Nullable`] that treats type mismatches as `undefined` +/// It is an alternative to `Option>` that treats type mismatches as `undefined` /// instead of erroring. When `T::try_from_js` reports a shallow conversion mismatch, the /// original JS value is preserved as [`Lenient::Unconvertable`] rather than propagating the error. /// [`Lenient::Unconvertable`] is considered `undefined` when passed to JavaScript or when -/// converted into a [`Nullable`]. +/// converted into an `Option>`. /// /// [`Lenient`] is intentionally minimal — it is designed for crossing the Rust<->C++ FFI /// boundary, with the possibility of inspecting the JS value. For general-purpose Rust, -/// code, prefer converting to [`Nullable`]. +/// code, prefer converting to `Option>`. pub enum Lenient { Some(T), Unconvertable(TracedGlobal), @@ -56,25 +56,25 @@ impl Lenient { matches!(self, Self::Unconvertable(_)) } - /// Converts `self` into a [`crate::Nullable`] while also providing - /// the value passed to a failed conversion as an optional argument in - /// the case that it exists. - pub fn to_parts(self) -> (Nullable, Option>) { + /// Converts `self` into an `Option>`, where `None` is + /// `undefined`, while also providing the value passed to a failed + /// conversion as an optional argument in the case that it exists. + pub fn to_parts(self) -> (Option>, Option>) { match self { - Self::Some(v) => (Nullable::Some(v), None), - Self::Unconvertable(global) => (Nullable::Undefined, Some(global)), - Self::Null => (Nullable::Null, None), - Self::Undefined => (Nullable::Undefined, None), + Self::Some(v) => (Some(Nullable::Some(v)), None), + Self::Unconvertable(global) => (None, Some(global)), + Self::Null => (Some(Nullable::Null), None), + Self::Undefined => (None, None), } } } -impl From> for Nullable { +impl From> for Option> { fn from(value: Lenient) -> Self { match value { - Lenient::Some(v) => Self::Some(v), - Lenient::Null => Self::Null, - Lenient::Unconvertable(_) | Lenient::Undefined => Self::Undefined, + Lenient::Some(v) => Some(Nullable::Some(v)), + Lenient::Null => Some(Nullable::Null), + Lenient::Unconvertable(_) | Lenient::Undefined => None, } } } @@ -84,7 +84,12 @@ impl From> for Lenient { match value { Nullable::Some(v) => Self::Some(v), Nullable::Null => Self::Null, - Nullable::Undefined => Self::Undefined, } } } + +impl From>> for Lenient { + fn from(value: Option>) -> Self { + value.map_or(Self::Undefined, Self::from) + } +} diff --git a/src/rust/jsg/nullable.rs b/src/rust/jsg/nullable.rs index 049017bcdcd..7c17704597b 100644 --- a/src/rust/jsg/nullable.rs +++ b/src/rust/jsg/nullable.rs @@ -8,19 +8,22 @@ use std::ops::DerefMut; use crate::Error; use crate::Result; -/// A wrapper type that accepts `null`, `undefined`, or a value of type `T`. +/// A value of type `T` or `null`: the JavaScript type `T | null`. /// -/// Unlike `Option`, where only `undefined` is `None` and `null` goes to `T`, -/// `Nullable` tells `null` and `undefined` apart. This is useful for -/// writing flexible JavaScript APIs where we have full control over behavior -/// depending on the passed value, including easily throwing errors to make an -/// API conform to Web IDL nullable behavior. +/// `Nullable` does not accept `undefined`, so a parameter of this type must +/// be given `null` or a `T`. Unlike C++ `kj::Maybe`, which also reads +/// `undefined` as empty, converting `undefined` to a `Nullable` is a +/// `TypeError`. Where `undefined` is allowed too, use `Option>`, +/// which keeps all three apart, since [`Option`] is `T | undefined`: /// -/// # Behavior +/// | JavaScript | `Nullable` | `Option>` | +/// |-------------|---------------------|---------------------------| +/// | a `T` | `Nullable::Some(v)` | `Some(Nullable::Some(v))` | +/// | `null` | `Nullable::Null` | `Some(Nullable::Null)` | +/// | `undefined` | `TypeError` | `None` | /// -/// - `null` → `Nullable::Null` -/// - `undefined` → `Nullable::Undefined` -/// - `T` → `Nullable::Some(T)` +/// For Web IDL's `T?`, which reads `undefined` as `null`, take an +/// `Option>` and call `unwrap_or(Nullable::Null)` on it. /// /// # Examples /// @@ -28,63 +31,43 @@ use crate::Result; /// use jsg::Nullable; /// /// #[jsg_method] -/// pub fn process(&self, value: Nullable) -> Result<(), Error> { +/// pub fn process(&self, value: Nullable) { /// match value { -/// Nullable::Some(s) => println!("Got value: {}", s), +/// Nullable::Some(s) => println!("Got value: {s}"), /// Nullable::Null => println!("Got null"), -/// Nullable::Undefined => println!("Got undefined"), /// } -/// Ok(()) /// } -/// ``` -/// -/// Error Handling: -/// -/// -/// ```ignore -/// use jsg::Nullable; /// /// #[jsg_method] -/// pub fn process_some(&self, value: Nullable) -> Result<(), Error> { -/// let Nullable::Some(v) = value { -/// // Process... -/// } else { -/// return Err(Error::new_type_error("Must be some")) +/// pub fn process_optional(&self, value: Option>) { +/// match value { +/// Some(Nullable::Some(s)) => println!("Got value: {s}"), +/// Some(Nullable::Null) => println!("Got null"), +/// None => println!("Got undefined"), /// } /// } -/// ``` -/// -/// ```ignore -/// use jsg::Nullable; /// /// #[jsg_method] -/// pub fn process_some_or_none(&self, value: Nullable) -> Result<(), Error> { -/// // Returns an Error +/// pub fn process_some(&self, value: Nullable) -> Result<(), Error> { +/// // Throws a `TypeError` for `null`. /// let value = value.not_null()?; /// // Process... +/// Ok(()) /// } -/// ``` -/// -/// Returning: -/// -/// ```ignore -/// use jsg::Nullable; /// /// #[jsg_method] /// pub fn return_nullable(&self) -> Nullable { /// if self.condition { /// Nullable::Some(self.number) /// } else { -/// Nullable::Undefined +/// Nullable::Null /// } /// } /// ``` -/// #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Nullable { Some(T), Null, - Undefined, } impl Nullable { @@ -98,76 +81,35 @@ impl Nullable { 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 { - !self.is_some() - } - - /// Returns a reference to the contained value in `Some` if `Some`, and returns - /// the respective empty value if empty. + /// Converts from `&Nullable` to `Nullable<&T>`. pub fn as_ref(&self) -> Nullable<&T> { match self { Self::Some(v) => Nullable::Some(v), Self::Null => Nullable::Null, - Self::Undefined => Nullable::Undefined, } } - /// Returns an exclusive reference to the contained value in `Some` if `Some`, and - /// returns the respective empty value if empty. + /// Converts from `&mut Nullable` to `Nullable<&mut T>`. pub fn as_mut(&mut self) -> Nullable<&mut T> { match self { Self::Some(v) => Nullable::Some(v), Self::Null => Nullable::Null, - Self::Undefined => Nullable::Undefined, } } - /// Returns `Ok(self)` if `!self.is_null()`. Otherwise returns `TypeError` if - /// `self` is null. - pub fn not_null(self) -> Result { - if self.is_null() { - Err(Error::new_type_error("Value cannot be null")) - } else { - Ok(self) - } - } - - /// Returns `Some(self)` if `!self.is_undefined()`. Otherwise returns `None` if - /// `self` is undefined. - pub fn not_undefined(self) -> Result { - if self.is_undefined() { - Err(Error::new_type_error("Value cannot be undefined")) - } else { - Ok(self) - } - } - - /// Coerces `undefined` to `null`, matching the Web IDL specified behavior for T? - /// where T does not support `undefined`. - #[must_use] - pub fn coerce_null(self) -> Self { - if self.is_undefined() { - Self::Null - } else { - self - } + /// Returns the contained value, or a `TypeError` if `self` is `Null`. + pub fn not_null(self) -> Result { + self.ok_or_else(|| Error::new_type_error("Value cannot be null")) } /// Returns the contained [`Some`] value, consuming `self`. /// - /// This function panics on `!self.is_some()`, making it unsuitable - /// for general use. Outside of testing, prefer pattern matching, or a non-panicking - /// variant. + /// This function panics on `Null`, making it unsuitable for general use. + /// Outside of testing, prefer pattern matching, or a non-panicking variant. /// /// # Panics /// - /// Panics if the `self` value is [`Nullable::Null`] or [`Nullable::Undefined`] + /// Panics if the `self` value is [`Nullable::Null`]. /// /// [`Some`]: Nullable::Some #[track_caller] @@ -179,20 +121,18 @@ impl Nullable { match self { Self::Some(v) => v, Self::Null => panic!("called `Nullable::unwrap()` on a `Null` value"), - Self::Undefined => panic!("called `Nullable::unwrap()` on an `Undefined` value"), } } /// Returns the contained [`Some`] value, consuming `self`. /// - /// This function panics on `!self.is_some()`, making it unsuitable - /// for general use. Outside of testing, prefer pattern matching, or a non-panicking - /// variant. + /// This function panics on `Null`, making it unsuitable for general use. + /// Outside of testing, prefer pattern matching, or a non-panicking variant. /// /// # Panics /// - /// Panics if the `self` value is [`Nullable::Null`] or [`Nullable::Undefined`], using a custom - /// panic message passed to `msg`. + /// Panics if the `self` value is [`Nullable::Null`], using a custom panic + /// message passed to `msg`. /// /// [`Some`]: Nullable::Some #[track_caller] @@ -203,7 +143,7 @@ impl Nullable { pub fn expect(self, msg: &str) -> T { match self { Self::Some(v) => v, - Self::Null | Self::Undefined => panic!("{}", msg), + Self::Null => panic!("{}", msg), } } @@ -217,7 +157,7 @@ impl Nullable { pub fn unwrap_or(self, default: T) -> T { match self { Self::Some(v) => v, - Self::Null | Self::Undefined => default, + Self::Null => default, } } @@ -230,24 +170,24 @@ impl Nullable { { match self { Self::Some(v) => v, - Self::Null | Self::Undefined => f(), + Self::Null => f(), } } /// Transforms the `Nullable` into a `Result`, mapping `Some(v)` to `Ok(v)` - /// and `None` to `Err(err)`. + /// and `Null` to `Err(err)`. /// /// This function is eagerly evaluated. Use [`Nullable::ok_or_else`] for - /// a lazyily evaluated alternative. + /// a lazily evaluated alternative. pub fn ok_or(self, err: Error) -> Result { match self { Self::Some(v) => Ok(v), - Self::Null | Self::Undefined => Err(err), + Self::Null => Err(err), } } /// Transforms the `Nullable` into a `Result`, mapping `Some(v)` to `Ok(v)` - /// and `None` to `Err(err())`. + /// and `Null` to `Err(err())`. /// /// This function is lazily evaluated. pub fn ok_or_else(self, err: F) -> Result @@ -256,7 +196,7 @@ impl Nullable { { match self { Self::Some(v) => Ok(v), - Self::Null | Self::Undefined => Err(err()), + Self::Null => Err(err()), } } @@ -270,7 +210,7 @@ impl Nullable { } /// Converts from `Nullable` to `Nullable<&mut T::Target>`. Equivalent - /// to [`Option::as_deref`] for nullable. + /// to [`Option::as_deref_mut`] for nullable. pub fn as_deref_mut(&mut self) -> Nullable<&mut T::Target> where T: DerefMut, @@ -279,7 +219,7 @@ impl Nullable { } /// Applies the passed function if `Some`, and flattens the result. - /// Otherwise, returns `self`. + /// Otherwise, returns `Null`. pub fn and_then(self, f: F) -> Nullable where F: FnOnce(T) -> Nullable, @@ -287,12 +227,11 @@ impl Nullable { match self { Self::Some(v) => f(v), Self::Null => Nullable::Null, - Self::Undefined => Nullable::Undefined, } } - /// Maps a [`Nullable`] to a [`Nullable`] by applying the passed function if [`Some`], or - /// the respective empty type otherwise. + /// Maps a [`Nullable`] to a [`Nullable`] by applying the passed function if [`Some`], + /// or returns `Null` otherwise. /// /// [`Some`]: Nullable::Some pub fn map(self, f: F) -> Nullable @@ -302,7 +241,6 @@ impl Nullable { match self { Self::Some(t) => Nullable::Some(f(t)), Self::Null => Nullable::Null, - Self::Undefined => Nullable::Undefined, } } } @@ -320,7 +258,7 @@ impl From> for Option { fn from(nullable: Nullable) -> Self { match nullable { Nullable::Some(v) => Some(v), - Nullable::Null | Nullable::Undefined => None, + Nullable::Null => None, } } } diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 4090f97b3f2..695361ad6e6 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -3723,7 +3723,6 @@ impl ToLocalValue for Nullable { match self { Self::Some(v) => v.to_local(lock), Self::Null => lock.null(), - Self::Undefined => lock.undefined(), } } } diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 5cb377410c1..7f2ab1f8381 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -24,7 +24,8 @@ //! | `i64` | `bigint` or `number` (parameter only) | //! | `SystemTime` | `Date`; from a `Date` or `number` of milliseconds | //! | `Option` | `T` or `undefined` | -//! | `Nullable` | `T`, `null`, or `undefined` | +//! | `Nullable` | `T` or `null` (not `undefined`) | +//! | `Option>` | `T`, `null`, or `undefined` | //! | `Lenient` | `T`, `null`, or `undefined` (shallow type mismatches silently become `undefined`) | //! | `Result` | `T` or throws | //! | `NonCoercible` | `T` (strict type checking) | @@ -885,7 +886,6 @@ impl<'s, T: ToJS<'s>> ToJS<'s> for Nullable { match self { Self::Some(value) => value.to_js(lock), Self::Null => Ok(lock.null()), - Self::Undefined => Ok(lock.undefined()), } } } @@ -966,6 +966,8 @@ impl<'s, T: Type + FromJS<'s>> FromJS<'s> for NonCoercible { } } +// Unlike C++ `kj::Maybe`, only `null` is absent; `undefined` is a type mismatch. +// `Option>` accepts `undefined` too. impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { type ResultType = Nullable; @@ -980,7 +982,10 @@ impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { if value.is_null() { Ok(Nullable::Null) } else if value.is_undefined() { - Ok(Nullable::Undefined) + Err(Error::new_type_error(format!( + "Expected {} or null but got undefined", + T::type_name() + ))) } else { Ok(Nullable::Some(T::from_js(lock, value)?)) } @@ -993,7 +998,7 @@ impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { if value.is_null() { Ok(Some(Nullable::Null)) } else if value.is_undefined() { - Ok(Some(Nullable::Undefined)) + Ok(None) } else { Ok(T::try_from_js(lock, value)?.map(Nullable::Some)) } From f7b0802c6d93a51fde1b78ba93be48330771d47b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 26 Oct 2026 18:47:10 -0700 Subject: [PATCH 2/2] Coerce undefined to null for Rust JSG Nullable inputs Follow Web IDL nullable conversion and C++ kj::Maybe: both null and undefined become Nullable::Null, while output remains T or null. Nullable method arguments remain required, so an explicit undefined is accepted but an omitted argument still throws a TypeError. This lets C++ bindings migrate to Nullable without changing runtime acceptance, arity checks, or their T | null TypeScript representation. Option> remains optional and preserves undefined separately. Update conversion, method-argument, evaluation, and struct-field tests, and document the input coercion separately from the output type. --- docs/reference/rust-review-checklist.md | 6 +++-- src/rust/jsg-test/tests/eval.rs | 4 ++-- src/rust/jsg-test/tests/nullable.rs | 30 +++++++++++++++---------- src/rust/jsg/nullable.rs | 18 ++++++++------- src/rust/jsg/wrappable.rs | 17 +++++--------- 5 files changed, 39 insertions(+), 36 deletions(-) diff --git a/docs/reference/rust-review-checklist.md b/docs/reference/rust-review-checklist.md index 0f30ca57830..d7e58245929 100644 --- a/docs/reference/rust-review-checklist.md +++ b/docs/reference/rust-review-checklist.md @@ -98,8 +98,10 @@ Rust types exposed to JavaScript via the JSG bindings follow these patterns: - **`#[jsg_oneof]`** is for union/variant types (mapped from JS values by trying each variant). - **Type mappings**: `jsg::Number` wraps JS numbers (distinct from `f64`). `Vec` maps to `Uint8Array`, not a regular JS `Array`. `Option` maps to `T | undefined`; like - `jsg::Optional`, `null` converts as `T`. `Nullable` maps to `T | null`; - use `Option>` for `T | null | undefined`. + `jsg::Optional`, `null` converts as `T`. `Nullable` maps to `T | null` and, like + `kj::Maybe`, coerces `undefined` to `null` on input, but a method argument is still + required. Use `Option>` for an optional `T | null | undefined` that keeps + `undefined` distinct from `null`. `String`/`&str` map to JS strings. Integers and `#[jsg_struct]`s convert like C++ JSG's integers and dictionaries: out-of-range integers throw, and an absent struct field reads as `undefined`. diff --git a/src/rust/jsg-test/tests/eval.rs b/src/rust/jsg-test/tests/eval.rs index c94007af7b1..019bf73715b 100644 --- a/src/rust/jsg-test/tests/eval.rs +++ b/src/rust/jsg-test/tests/eval.rs @@ -118,8 +118,8 @@ fn eval_undefined() { harness.run_in_context(|lock, ctx| { let result: Option> = ctx.eval(lock, "undefined").unwrap(); assert!(result.is_none()); - let result = ctx.eval::>(lock, "undefined"); - assert!(matches!(result, Err(EvalError::UncoercibleResult { .. }))); + let result: jsg::Nullable = ctx.eval(lock, "undefined").unwrap(); + assert!(result.is_null()); Ok(()) }); } diff --git a/src/rust/jsg-test/tests/nullable.rs b/src/rust/jsg-test/tests/nullable.rs index 8f5ca37c773..154d7ef2460 100644 --- a/src/rust/jsg-test/tests/nullable.rs +++ b/src/rust/jsg-test/tests/nullable.rs @@ -2,10 +2,9 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -//! `Nullable` is `T | null`: converting `undefined` to it is a `TypeError`. +//! `Nullable` is `T | null`, with Web IDL's `undefined`-to-`null` coercion. //! `Option>` is `T | null | undefined`, and keeps the three apart. -use jsg::ExceptionType; use jsg::FromJS; use jsg::Nullable; use jsg::ToJS; @@ -100,19 +99,19 @@ fn nullable_reads_null_and_values() { } #[test] -fn nullable_rejects_undefined() { +fn nullable_coerces_undefined_to_null() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { - let err = convert::>(lock, ctx, "undefined").unwrap_err(); - assert_eq!(err.name, ExceptionType::TypeError); - assert_eq!(err.message, "Expected string or null but got undefined"); - // For `try_from_js()`, `undefined` is a top-level mismatch, not an error. + let value = convert::>(lock, ctx, "undefined")?; + assert_eq!(value, Nullable::Null); + assert!(value.to_js(lock)?.is_null()); + let undefined = ctx .eval_raw(lock, "undefined") .expect("expression should evaluate"); assert_eq!( as FromJS>::try_from_js(lock, undefined)?, - None + Some(Nullable::Null) ); Ok(()) }); @@ -151,7 +150,7 @@ fn nullable_to_js() { } #[test] -fn nullable_argument_requires_null_or_a_value() { +fn nullable_argument_is_required_but_accepts_undefined() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let args = jsg::Rc::new(Args {}).to_js(lock)?; @@ -159,7 +158,7 @@ fn nullable_argument_requires_null_or_a_value() { let not_of_type = "TypeError: Failed to execute 'takeNullable' on 'Args': parameter 1 \ is not of type 'string'."; assert_eq!(call(lock, ctx, "args.takeNullable()"), not_of_type); - assert_eq!(call(lock, ctx, "args.takeNullable(undefined)"), not_of_type); + assert_eq!(call(lock, ctx, "args.takeNullable(undefined)"), "null"); assert_eq!(call(lock, ctx, "args.takeNullable(null)"), "null"); assert_eq!(call(lock, ctx, "args.takeNullable('x')"), "some x"); @@ -200,8 +199,15 @@ fn nullable_struct_fields() { optional: Some(Nullable::Null), } ); - let err = convert::(lock, ctx, "({})").unwrap_err(); - assert_eq!(err.message, "Expected string or null but got undefined"); + for expr in ["({})", "({ required: undefined, optional: undefined })"] { + assert_eq!( + convert::(lock, ctx, expr)?, + Fields { + required: Nullable::Null, + optional: None, + } + ); + } let value = Fields { required: Nullable::Null, diff --git a/src/rust/jsg/nullable.rs b/src/rust/jsg/nullable.rs index 7c17704597b..e59bf596970 100644 --- a/src/rust/jsg/nullable.rs +++ b/src/rust/jsg/nullable.rs @@ -10,20 +10,22 @@ use crate::Result; /// A value of type `T` or `null`: the JavaScript type `T | null`. /// -/// `Nullable` does not accept `undefined`, so a parameter of this type must -/// be given `null` or a `T`. Unlike C++ `kj::Maybe`, which also reads -/// `undefined` as empty, converting `undefined` to a `Nullable` is a -/// `TypeError`. Where `undefined` is allowed too, use `Option>`, -/// which keeps all three apart, since [`Option`] is `T | undefined`: +/// Like C++ `kj::Maybe` and Web IDL's `T?`, input conversion coerces +/// `undefined` to `null`. A method parameter of this type is still required: +/// passing `undefined` produces `Nullable::Null`, but omitting the argument +/// is a `TypeError`. +/// +/// Use `Option>` for an optional nullable that keeps `undefined` +/// distinct from `null`, since [`Option`] is `T | undefined`: /// /// | JavaScript | `Nullable` | `Option>` | /// |-------------|---------------------|---------------------------| /// | a `T` | `Nullable::Some(v)` | `Some(Nullable::Some(v))` | /// | `null` | `Nullable::Null` | `Some(Nullable::Null)` | -/// | `undefined` | `TypeError` | `None` | +/// | `undefined` | `Nullable::Null` | `None` | /// -/// For Web IDL's `T?`, which reads `undefined` as `null`, take an -/// `Option>` and call `unwrap_or(Nullable::Null)` on it. +/// Output conversion produces only `T` or `null`. The TypeScript type is +/// `T | null`; `Option>` adds `undefined` and permits omission. /// /// # Examples /// diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 7f2ab1f8381..f972eed6df6 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -24,7 +24,7 @@ //! | `i64` | `bigint` or `number` (parameter only) | //! | `SystemTime` | `Date`; from a `Date` or `number` of milliseconds | //! | `Option` | `T` or `undefined` | -//! | `Nullable` | `T` or `null` (not `undefined`) | +//! | `Nullable` | `T` or `null` (`undefined` coerces to `null` on input) | //! | `Option>` | `T`, `null`, or `undefined` | //! | `Lenient` | `T`, `null`, or `undefined` (shallow type mismatches silently become `undefined`) | //! | `Result` | `T` or throws | @@ -966,8 +966,8 @@ impl<'s, T: Type + FromJS<'s>> FromJS<'s> for NonCoercible { } } -// Unlike C++ `kj::Maybe`, only `null` is absent; `undefined` is a type mismatch. -// `Option>` accepts `undefined` too. +// Like C++ `kj::Maybe`, Web IDL nullable conversion reads `undefined` as `null`. +// Nullable arguments remain required; only an outer `Option` permits omission. impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { type ResultType = Nullable; @@ -979,13 +979,8 @@ impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, ) -> Result { - if value.is_null() { + if value.is_null_or_undefined() { Ok(Nullable::Null) - } else if value.is_undefined() { - Err(Error::new_type_error(format!( - "Expected {} or null but got undefined", - T::type_name() - ))) } else { Ok(Nullable::Some(T::from_js(lock, value)?)) } @@ -995,10 +990,8 @@ impl<'s, T: FromJS<'s>> FromJS<'s> for Nullable { lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, ) -> Result, Error> { - if value.is_null() { + if value.is_null_or_undefined() { Ok(Some(Nullable::Null)) - } else if value.is_undefined() { - Ok(None) } else { Ok(T::try_from_js(lock, value)?.map(Nullable::Some)) }