Repository: denoland/deno_core
Pull request #168 (2023-08-30 16:04:57)

fix(ops): Handle #[smi] types
`#[smi]`-tagged arguments were going through the wrong path because I neglected to restore the custom `Numeric(__SMI__)` handling in  #165.

To fix this, we use a `RustToV8Marker` to override the default behaviour and ensure that SMIs are always serialized as v8 i32s. Fixes a Deno crash in the HTTP test.


Patch:
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
index 8a8a37c..25be931 100644
--- a/core/runtime/ops.rs
+++ b/core/runtime/ops.rs
@@ -835,6 +835,8 @@ mod tests {
     Ok(())
   }
 
+  // Note: #[smi] parameters are signed in JS regardless of the sign in Rust. Overflow and underflow
+  // of valid ranges result in automatic wrapping.
   #[op2(core, fast)]
   #[smi]
   pub fn op_test_add_smi_unsigned(#[smi] a: u32, #[smi] b: u16) -> u32 {
@@ -849,6 +851,11 @@ mod tests {
       "op_test_add_smi_unsigned",
       "assert(op_test_add_smi_unsigned(1000, 2000) == 3000)",
     )?;
+    run_test2(
+      10000,
+      "op_test_add_smi_unsigned",
+      "assert(op_test_add_smi_unsigned(-1000, 10) == -990)",
+    )?;
     Ok(())
   }
 
@@ -1632,14 +1639,16 @@ mod tests {
 
   #[op2(async, core)]
   async fn op_async_add(x: u32, y: u32) -> u32 {
-    x + y
+    x.wrapping_add(y)
   }
 
+  // Note: #[smi] parameters are signed in JS regardless of the sign in Rust. Overflow and underflow
+  // of valid ranges result in automatic wrapping.
   #[op2(async, core)]
   #[smi]
   async fn op_async_add_smi(#[smi] x: u32, #[smi] y: u32) -> u32 {
     tokio::time::sleep(Duration::from_millis(10)).await;
-    x + y
+    x.wrapping_add(y)
   }
 
   #[tokio::test]
@@ -1663,6 +1672,12 @@ mod tests {
       "assert(await op_async_add_smi(__index__, 100) == __index__ + 100)",
     )
     .await?;
+    // See note about overflow on the op method
+    run_async_test(
+      10,
+      "op_async_add_smi",
+      "assert(await op_async_add_smi(__index__ * -100, 100) == __index__ * -100 + 100)",
+    ).await?;
     Ok(())
   }
 
diff --git a/core/runtime/ops_rust_to_v8.rs b/core/runtime/ops_rust_to_v8.rs
index 10bf367..7254a18 100644
--- a/core/runtime/ops_rust_to_v8.rs
+++ b/core/runtime/ops_rust_to_v8.rs
@@ -147,6 +147,9 @@ impl<M, T> From<T> for RustToV8Marker<M, T> {
 /// This struct should be serialized using `serde`.
 pub struct SerdeMarker;
 
+/// This primitive should be serialized as an SMI.
+pub struct SmiMarker;
+
 /// Helper macro for [`RustToV8`] to reduce boilerplate.
 ///
 /// Implements a Rust-to-v8 conversion that cannot fail. Multiple types may be specified
@@ -362,6 +365,36 @@ impl<'a, T: serde::Serialize> RustToV8Fallible<'a>
   }
 }
 
+//
+// SMI
+//
+
+/// Implement an infallible SMI conversion from each of the integer types. SMI has a subset
+/// of the range of a normal integer, so we just do a bitwise cast.
+// TODO(mmastrac): We should be able to make these things much faster and without a scope
+macro_rules! smi_to_v8 {
+  ($($ty:ident),*) => {
+    $(
+      impl<'a> RustToV8<'a> for RustToV8Marker<SmiMarker, $ty>
+      {
+        #[inline(always)]
+        fn to_v8(self, scope: &mut v8::HandleScope<'a>) -> v8::Local<'a, v8::Value> {
+          v8::Integer::new(scope, self.0 as _).into()
+        }
+      }
+
+      impl <'a> RustToV8RetVal<'a> for RustToV8Marker<SmiMarker, $ty> {
+        #[inline(always)]
+        fn to_v8_rv(self, rv: &mut v8::ReturnValue<'a>) {
+          rv.set_int32(self.0 as _)
+        }
+      }
+    )*
+  };
+}
+
+smi_to_v8!(u8, u16, u32, u64, usize, i8, i16, i32, i64, isize);
+
 //
 // Globals
 //
diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
index 4a391be..6b34c4b 100644
--- a/ops/op2/dispatch_slow.rs
+++ b/ops/op2/dispatch_slow.rs
@@ -521,6 +521,9 @@ pub fn return_value_infallible(
     ArgMarker::Serde => {
       gs_quote!(generator_state(deno_core, result) => (#deno_core::_ops::RustToV8Marker::<#deno_core::_ops::SerdeMarker, _>::from(#result)))
     }
+    ArgMarker::Smi => {
+      gs_quote!(generator_state(deno_core, result) => (#deno_core::_ops::RustToV8Marker::<#deno_core::_ops::SmiMarker, _>::from(#result)))
+    }
     ArgMarker::None => gs_quote!(generator_state(result) => (#result)),
   };
   let res = match ret_type.slow_retval() {
@@ -579,6 +582,9 @@ pub fn return_value_v8_value(
     ArgMarker::Serde => {
       quote!(#deno_core::_ops::RustToV8Marker::<#deno_core::_ops::SerdeMarker, _>::from(#result))
     }
+    ArgMarker::Smi => {
+      quote!(#deno_core::_ops::RustToV8Marker::<#deno_core::_ops::SmiMarker, _>::from(#result))
+    }
     ArgMarker::None => quote!(#result),
   };
   let res = match ret_type.slow_retval() {
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index ff957e6..961a4af 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -354,6 +354,7 @@ impl Arg {
   pub fn marker(&self) -> ArgMarker {
     match self {
       Arg::SerdeV8(_) => ArgMarker::Serde,
+      Arg::Numeric(NumericArg::__SMI__) => ArgMarker::Smi,
       _ => ArgMarker::None,
     }
   }
@@ -382,7 +383,10 @@ pub enum ArgSlowRetval {
 /// Specifies an ArgMarker wrapper for a type used for trait-based serialization.
 pub enum ArgMarker {
   None,
+  /// This type should be serialized with serde_v8.
   Serde,
+  /// This type should be serialized as an SMI.
+  Smi,
 }
 
 pub enum ParsedType {


Test patch:
diff --git a/ops/op2/test_cases/async/async_result_smi.out b/ops/op2/test_cases/async/async_result_smi.out
index fb3fbf5..e2bde97 100644
--- a/ops/op2/test_cases/async/async_result_smi.out
+++ b/ops/op2/test_cases/async/async_result_smi.out
@@ -59,10 +59,28 @@ impl op_async {
                 false,
                 promise_id,
                 result,
-                |scope, result| { Ok(deno_core::_ops::RustToV8::to_v8(result, scope)) },
+                |scope, result| {
+                    Ok(
+                        deno_core::_ops::RustToV8::to_v8(
+                            deno_core::_ops::RustToV8Marker::<
+                                deno_core::_ops::SmiMarker,
+                                _,
+                            >::from(result),
+                            scope,
+                        ),
+                    )
+                },
             ) {
             match result {
-                Ok(result) => deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv),
+                Ok(result) => {
+                    deno_core::_ops::RustToV8RetVal::to_v8_rv(
+                        deno_core::_ops::RustToV8Marker::<
+                            deno_core::_ops::SmiMarker,
+                            _,
+                        >::from(result),
+                        &mut rv,
+                    )
+                }
                 Err(err) => {
                     let err = err.into();
                     let exception = deno_core::error::to_v8_error(
diff --git a/ops/op2/test_cases/sync/smi.out b/ops/op2/test_cases/sync/smi.out
index d1577af..0c0dca1 100644
--- a/ops/op2/test_cases/sync/smi.out
+++ b/ops/op2/test_cases/sync/smi.out
@@ -112,7 +112,13 @@ impl op_smi_unsigned_return {
             let arg3 = arg3 as _;
             Self::call(arg0, arg1, arg2, arg3)
         };
-        deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv)
+        deno_core::_ops::RustToV8RetVal::to_v8_rv(
+            deno_core::_ops::RustToV8Marker::<
+                deno_core::_ops::SmiMarker,
+                _,
+            >::from(result),
+            &mut rv,
+        )
     }
     #[inline(always)]
     fn call(a: Int16, b: Int32, c: Uint16, d: Uint32) -> Uint32 {
@@ -234,7 +240,13 @@ impl op_smi_signed_return {
             let arg3 = arg3 as _;
             Self::call(arg0, arg1, arg2, arg3)
         };
-        deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv)
+        deno_core::_ops::RustToV8RetVal::to_v8_rv(
+            deno_core::_ops::RustToV8Marker::<
+                deno_core::_ops::SmiMarker,
+                _,
+            >::from(result),
+            &mut rv,
+        )
     }
     #[inline(always)]
     fn call(a: Int16, b: Int32, c: Uint16, d: Uint32) -> Int32 {