V8: Ser/de finishing touches (#3153)

# Description of Changes

This does 2 things:
1. simplifies the deserialization of optional sums so that they are
treated as regular sums.
2. adds higher level v8 ser/de interfaces and privatizes what can be
private.

# API and ABI breaking changes

None

# Expected complexity level and risk

1

# Testing

Existing ser/de tests are adjusted to use the new interfaces.

Author: Centril

crates/core/src/host/v8/de.rs

@@ -1,27 +1,60 @@
 #![allow(dead_code)]
 
-use super::error::{exception_already_thrown, ExceptionThrown};
+use super::error::{exception_already_thrown, ExcResult, ExceptionThrown, ExceptionValue, Throwable, TypeError};
 use super::from_value::{cast, FromValue};
+use core::cell::RefCell;
 use core::fmt;
 use core::iter::{repeat_n, RepeatN};
+use core::marker::PhantomData;
 use core::mem::MaybeUninit;
 use derive_more::From;
-use spacetimedb_sats::de::{
-    self, ArrayVisitor, DeserializeSeed, NoneAccess, ProductVisitor, SliceVisitor, SomeAccess, SumVisitor,
-};
+use spacetimedb_sats::de::{self, ArrayVisitor, DeserializeSeed, ProductVisitor, SliceVisitor, SumVisitor};
 use spacetimedb_sats::{i256, u256};
 use std::borrow::{Borrow, Cow};
+use std::rc::Rc;
 use v8::{Array, Global, HandleScope, Local, Name, Object, Uint8Array, Value};
 
+/// Returns a `KeyCache` for the current `scope`.
+///
+/// Creates the cache in the scope if it doesn't exist yet.
+pub(super) fn get_or_create_key_cache(scope: &mut HandleScope<'_>) -> Rc<RefCell<KeyCache>> {
+    let context = scope.get_current_context();
+    context.get_slot::<RefCell<KeyCache>>().unwrap_or_else(|| {
+        let cache = Rc::default();
+        context.set_slot(Rc::clone(&cache));
+        cache
+    })
+}
+
+/// Deserializes a `T` from `val` in `scope`, using `seed` for any context needed.
+pub(super) fn deserialize_js_seed<'de, T: DeserializeSeed<'de>>(
+    scope: &mut HandleScope<'de>,
+    val: Local<'_, Value>,
+    seed: T,
+) -> ExcResult<T::Output> {
+    let key_cache = get_or_create_key_cache(scope);
+    let key_cache = &mut *key_cache.borrow_mut();
+    let de = Deserializer::new(scope, val, key_cache);
+    seed.deserialize(de).map_err(|e| e.throw(scope))
+}
+
+/// Deserializes a `T` from `val` in `scope`.
+pub(super) fn deserialize_js<'de, T: de::Deserialize<'de>>(
+    scope: &mut HandleScope<'de>,
+    val: Local<'_, Value>,
+) -> ExcResult<T> {
+    deserialize_js_seed(scope, val, PhantomData)
+}
+
 /// Deserializes from V8 values.
-pub(super) struct Deserializer<'this, 'scope> {
+struct Deserializer<'this, 'scope> {
     common: DeserializerCommon<'this, 'scope>,
     input: Local<'scope, Value>,
 }
 
 impl<'this, 'scope> Deserializer<'this, 'scope> {
     /// Creates a new deserializer from `input` in `scope`.
-    pub fn new(scope: &'this mut HandleScope<'scope>, input: Local<'_, Value>, key_cache: &'this mut KeyCache) -> Self {
+    fn new(scope: &'this mut HandleScope<'scope>, input: Local<'_, Value>, key_cache: &'this mut KeyCache) -> Self {
         let input = Local::new(scope, input);
         let common = DeserializerCommon { scope, key_cache };
         Deserializer { input, common }
@@ -49,12 +82,22 @@ impl<'scope> DeserializerCommon<'_, 'scope> {
 
 /// The possible errors that [`Deserializer`] can produce.
 #[derive(Debug, From)]
-pub(super) enum Error<'scope> {
-    Value(Local<'scope, Value>),
-    Exception(ExceptionThrown),
+enum Error<'scope> {
+    Unthrown(ExceptionValue<'scope>),
+    Thrown(ExceptionThrown),
     Custom(String),
 }
 
+impl<'scope> Throwable<'scope> for Error<'scope> {
+    fn throw(self, scope: &mut HandleScope<'scope>) -> ExceptionThrown {
+        match self {
+            Self::Unthrown(exception) => exception.throw(scope),
+            Self::Thrown(thrown) => thrown,
+            Self::Custom(msg) => TypeError(msg).throw(scope),
+        }
+    }
+}
+
 impl de::Error for Error<'_> {
     fn custom(msg: impl fmt::Display) -> Self {
         Self::Custom(msg.to_string())
@@ -104,7 +147,7 @@ impl KeyCache {
 }
 
 // Creates an interned [`v8::String`].
-pub(super) fn v8_interned_string<'scope>(scope: &mut HandleScope<'scope>, field: &str) -> Local<'scope, v8::String> {
+fn v8_interned_string<'scope>(scope: &mut HandleScope<'scope>, field: &str) -> Local<'scope, v8::String> {
     // Internalized v8 strings are significantly faster than "normal" v8 strings
     // since v8 deduplicates re-used strings minimizing new allocations
     // see: https://github.com/v8/v8/blob/14ac92e02cc3db38131a57e75e2392529f405f2f/include/v8.h#L3165-L3171
@@ -128,7 +171,7 @@ fn deref_local<'scope, T>(local: Local<'scope, T>) -> &'scope T {
 macro_rules! deserialize_primitive {
     ($dmethod:ident, $t:ty) => {
         fn $dmethod(self) -> Result<$t, Self::Error> {
-            FromValue::from_value(self.input, self.common.scope).map_err(Error::Value)
+            FromValue::from_value(self.input, self.common.scope).map_err(Error::Unthrown)
         }
     };
 }
@@ -175,40 +218,9 @@ impl<'de, 'this, 'scope: 'de> de::Deserializer<'de> for Deserializer<'this, 'sco
         let sum_name = visitor.sum_name().unwrap_or("<unknown>");
 
         // We expect a canonical representation of a sum value in JS to be
-        // `{ tag: "foo", value: a_value_for_foo }`
-        // with special convenience for optionals
-        // where we also accept `null`, `undefined` and an object without `tag`.
-        let (object, tag_field) = 'treat_as_regular_sum: {
-            // Optionals receive some special handling for added convenience in JS.
-            if visitor.is_option() {
-                // If we don't have an object at all,
-                // it's either `null | undefined` which means `none`
-                // or it is `some(the_value)`.
-                if let Some(object) = self.input.to_object(scope) {
-                    // If there is `tag` field, treat this as a normal sum.
-                    // Otherwise, we have `some(the_value)`.
-                    let tag_field = self.common.key_cache.tag(scope);
-                    if object
-                        .has_own_property(scope, tag_field.into())
-                        .ok_or_else(exception_already_thrown)?
-                    {
-                        break 'treat_as_regular_sum (object, tag_field);
-                    }
-                } else if self.input.is_null_or_undefined() {
-                    // JS has support for `undefined` and `null` values.
-                    // It's reasonable to interpret these as `None`
-                    // when we're deserializing to an optional value
-                    // rust-side, such as `Option<T>`.
-                    return visitor.visit_sum(NoneAccess::new());
-                }
-
-                return visitor.visit_sum(SomeAccess::new(self));
-            } else {
-                let tag_field = self.common.key_cache.tag(scope);
-                let val = cast!(scope, self.input, Object, "object for sum type `{}`", sum_name)?;
-                (val, tag_field)
-            }
-        };
+        // `{ tag: "foo", value: a_value_for_foo }`.
+        let tag_field = self.common.key_cache.tag(scope);
+        let object = cast!(scope, self.input, Object, "object for sum type `{}`", sum_name)?;
 
         // Extract the `tag` field. It needs to contain a string.
         let tag = object
@@ -320,7 +332,7 @@ impl<'de, 'scope: 'de> de::NamedProductAccess<'de> for ProductAccess<'_, 'scope>
         Ok(None)
     }
 
-    fn get_field_value_seed<T: de::DeserializeSeed<'de>>(&mut self, seed: T) -> Result<T::Output, Self::Error> {
+    fn get_field_value_seed<T: DeserializeSeed<'de>>(&mut self, seed: T) -> Result<T::Output, Self::Error> {
         let common = self.common.reborrow();
         // Extract the field's value.
         let input = self
@@ -369,7 +381,7 @@ impl<'de, 'this, 'scope: 'de> de::SumAccess<'de> for SumAccess<'this, 'scope> {
 impl<'de, 'this, 'scope: 'de> de::VariantAccess<'de> for Deserializer<'this, 'scope> {
     type Error = Error<'scope>;
 
-    fn deserialize_seed<T: de::DeserializeSeed<'de>>(self, seed: T) -> Result<T::Output, Self::Error> {
+    fn deserialize_seed<T: DeserializeSeed<'de>>(self, seed: T) -> Result<T::Output, Self::Error> {
         seed.deserialize(self)
     }
 }

crates/core/src/host/v8/error.rs

@@ -3,7 +3,7 @@
 use v8::{Exception, HandleScope, Local, Value};
 
 /// The result of trying to convert a [`Value`] in scope `'scope` to some type `T`.
-pub(super) type ValueResult<'scope, T> = Result<T, Local<'scope, Value>>;
+pub(super) type ValueResult<'scope, T> = Result<T, ExceptionValue<'scope>>;
 
 /// Types that can convert into a JS string type.
 pub(super) trait IntoJsString {
@@ -17,20 +17,43 @@ impl IntoJsString for String {
     }
 }
 
+/// A JS exception value.
+///
+/// Newtyped for additional type safety and to track JS exceptions in the type system.
+#[derive(Debug)]
+pub(super) struct ExceptionValue<'scope>(Local<'scope, Value>);
+
 /// Error types that can convert into JS exception values.
-pub(super) trait IntoException {
+pub(super) trait IntoException<'scope> {
     /// Converts `self` into a JS exception value.
-    fn into_exception<'scope>(self, scope: &mut HandleScope<'scope>) -> Local<'scope, Value>;
+    fn into_exception(self, scope: &mut HandleScope<'scope>) -> ExceptionValue<'scope>;
+}
+
+impl<'scope> IntoException<'scope> for ExceptionValue<'scope> {
+    fn into_exception(self, _: &mut HandleScope<'scope>) -> ExceptionValue<'scope> {
+        self
+    }
 }
 
 /// A type converting into a JS `TypeError` exception.
 #[derive(Copy, Clone)]
 pub struct TypeError<M>(pub M);
 
-impl<M: IntoJsString> IntoException for TypeError<M> {
-    fn into_exception<'scope>(self, scope: &mut HandleScope<'scope>) -> Local<'scope, Value> {
+impl<'scope, M: IntoJsString> IntoException<'scope> for TypeError<M> {
+    fn into_exception(self, scope: &mut HandleScope<'scope>) -> ExceptionValue<'scope> {
         let msg = self.0.into_string(scope);
-        Exception::type_error(scope, msg)
+        ExceptionValue(Exception::type_error(scope, msg))
+    }
+}
+
+/// A type converting into a JS `RangeError` exception.
+#[derive(Copy, Clone)]
+pub struct RangeError<M>(pub M);
+
+impl<'scope, M: IntoJsString> IntoException<'scope> for RangeError<M> {
+    fn into_exception(self, scope: &mut HandleScope<'scope>) -> ExceptionValue<'scope> {
+        let msg = self.0.into_string(scope);
+        ExceptionValue(Exception::range_error(scope, msg))
     }
 }
 
@@ -39,7 +62,27 @@ pub(super) struct ExceptionThrown {
     _priv: (),
 }
 
+/// A result where the error indicates that an exception has already been thrown in V8.
+pub(super) type ExcResult<T> = Result<T, ExceptionThrown>;
+
 /// Indicates that the JS side had thrown an exception.
 pub(super) fn exception_already_thrown() -> ExceptionThrown {
     ExceptionThrown { _priv: () }
 }
+
+/// Types that can be thrown as a V8 exception.
+pub(super) trait Throwable<'scope> {
+    /// Throw `self` into the V8 engine as an exception.
+    ///
+    /// If an exception has already been thrown,
+    /// [`ExceptionThrown`] can be returned directly.
+    fn throw(self, scope: &mut HandleScope<'scope>) -> ExceptionThrown;
+}
+
+impl<'scope, T: IntoException<'scope>> Throwable<'scope> for T {
+    fn throw(self, scope: &mut HandleScope<'scope>) -> ExceptionThrown {
+        let ExceptionValue(exception) = self.into_exception(scope);
+        scope.throw_exception(exception);
+        exception_already_thrown()
+    }
+}

crates/core/src/host/v8/from_value.rs

@@ -1,5 +1,7 @@
 #![allow(dead_code)]
 
+use crate::host::v8::error::ExceptionValue;
+
 use super::error::{IntoException as _, TypeError, ValueResult};
 use bytemuck::{AnyBitPattern, NoUninit};
 use spacetimedb_sats::{i256, u256};
@@ -48,13 +50,13 @@ pub(super) use cast;
 
 /// Returns a JS exception value indicating that a value overflowed
 /// when converting to the type `rust_ty`.
-fn value_overflowed<'scope>(rust_ty: &str, scope: &mut HandleScope<'scope>) -> Local<'scope, Value> {
+fn value_overflowed<'scope>(rust_ty: &str, scope: &mut HandleScope<'scope>) -> ExceptionValue<'scope> {
     TypeError(format!("Value overflowed `{rust_ty}`")).into_exception(scope)
 }
 
 /// Returns a JS exception value indicating that a value underflowed
 /// when converting to the type `rust_ty`.
-fn value_underflowed<'scope>(rust_ty: &str, scope: &mut HandleScope<'scope>) -> Local<'scope, Value> {
+fn value_underflowed<'scope>(rust_ty: &str, scope: &mut HandleScope<'scope>) -> ExceptionValue<'scope> {
     TypeError(format!("Value underflowed `{rust_ty}`")).into_exception(scope)
 }