Repository: denoland/deno_core
Pull request #791 (2024-06-20 18:00:00)

Allow `#[cppgc] &mut T` in sync ops
Allow `#[cppgc] _: &mut T` and `#[cppgc] _: Option<&mut T>`

Closes https://github.com/denoland/deno_core/issues/787

Trying to use it with async op gives a compiler error:
```
error[E0308]: mismatched types
    --> core/runtime/ops.rs:1911:3
     |
1911 |   #[op2(async)]
     |   ^^^^^^^^^^^^^
     |   |
     |   types differ in mutability
     |   arguments to this function are incorrect
     |
     = note: expected mutable reference `&mut TestResource`
                        found reference `&TestResource`
note: associated function defined here
    --> core/runtime/ops.rs:1911:3
     |
1911 |   #[op2(async)]
     |   ^^^^^^^^^^^^^
1912 |   pub async fn op_test_set_cppgc_resource(#[cppgc] resource: &mut TestResource, value: u32) {
```


Patch:
diff --git a/core/cppgc.rs b/core/cppgc.rs
index 8b5c002..254bb90 100644
--- a/core/cppgc.rs
+++ b/core/cppgc.rs
@@ -43,10 +43,17 @@ pub fn make_cppgc_object<'a, T: GcResource + 'static>(
   obj
 }
 
+// TODO(littledivy): https://github.com/denoland/rusty_v8/pull/1505
+#[repr(C)]
+struct InnerMember {
+  inner: [usize; 2],
+  ptr: *mut (),
+}
+
 #[allow(clippy::needless_lifetimes)]
 pub fn try_unwrap_cppgc_object<'sc, T: GcResource + 'static>(
   val: v8::Local<'sc, v8::Value>,
-) -> Option<&'sc T> {
+) -> Option<&'sc mut T> {
   let Ok(obj): Result<v8::Local<v8::Object>, _> = val.try_into() else {
     return None;
   };
@@ -62,13 +69,13 @@ pub fn try_unwrap_cppgc_object<'sc, T: GcResource + 'static>(
   // The object can only be created by `make_cppgc_object`.
   let member = unsafe {
     let ptr = obj.get_aligned_pointer_from_internal_field(SLOT_OFFSET);
-    let obj = &*(ptr as *const v8::cppgc::InnerMember);
-    obj.get::<CppGcObject<T>>()
+    let obj = &*(ptr as *mut InnerMember);
+    obj.ptr.cast::<CppGcObject<T>>().as_mut().unwrap()
   };
 
   if member.tag != TypeId::of::<T>() {
     return None;
   }
 
-  Some(&member.member)
+  Some(&mut member.member)
 }
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
index 1d22ed7..05743d2 100644
--- a/core/runtime/ops.rs
+++ b/core/runtime/ops.rs
@@ -610,6 +610,7 @@ mod tests {
       op_test_get_cppgc_resource,
       op_test_get_cppgc_resource_option,
       op_test_make_cppgc_resource,
+      op_test_set_cppgc_resource,
       op_test_make_cppgc_resource_option,
       op_external_make,
       op_external_process,
@@ -1927,6 +1928,14 @@ mod tests {
     resource.value
   }
 
+  #[op2(fast)]
+  pub fn op_test_set_cppgc_resource(
+    #[cppgc] resource: &mut TestResource,
+    value: u32,
+  ) {
+    resource.value = value;
+  }
+
   #[op2(fast)]
   #[smi]
   pub fn op_test_get_cppgc_resource_option(
@@ -1944,7 +1953,7 @@ mod tests {
   {
     run_async_test(
       10,
-      "op_test_make_cppgc_resource, op_test_get_cppgc_resource, op_test_get_cppgc_resource_option, op_test_make_cppgc_resource_option",
+      "op_test_make_cppgc_resource, op_test_get_cppgc_resource, op_test_get_cppgc_resource_option, op_test_make_cppgc_resource_option, op_test_set_cppgc_resource",
       r"
       const resource = op_test_make_cppgc_resource();
       assert((await op_test_get_cppgc_resource(resource)) === 42);
@@ -1953,7 +1962,9 @@ mod tests {
       const resource2 = op_test_make_cppgc_resource_option(false);
       assert(resource2 === null);
       const resource3 = op_test_make_cppgc_resource_option(true);
-      assert((await op_test_get_cppgc_resource(resource3)) === 42);",
+      assert((await op_test_get_cppgc_resource(resource3)) === 42);
+      op_test_set_cppgc_resource(resource, 43);
+      assert((await op_test_get_cppgc_resource(resource)) == 43);"
     ).await?;
     Ok(())
   }
diff --git a/ops/op2/dispatch_async.rs b/ops/op2/dispatch_async.rs
index a5ec579..b07dece 100644
--- a/ops/op2/dispatch_async.rs
+++ b/ops/op2/dispatch_async.rs
@@ -47,7 +47,7 @@ pub(crate) fn generate_dispatch_async(
   let mut output = TokenStream::new();
 
   let with_self = if generator_state.needs_self {
-    with_self(generator_state, &signature.ret_val)
+    with_self(generator_state, true, &signature.ret_val)
       .map_err(V8SignatureMappingError::NoSelfMapping)?
   } else {
     quote!()
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index ea0325c..e979f30 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -757,7 +757,7 @@ fn map_v8_fastcall_arg_to_arg(
         let #arg_ident = if #arg_ident.is_null_or_undefined() {
           None
         } else if let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(#arg_ident) {
-          Some(#arg_ident)
+          Some(#arg_ident as _)
         } else {
           #fast_api_callback_options.fallback = true;
           // SAFETY: All fast return types have zero as a valid value
diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
index 96c52c0..4e91496 100644
--- a/ops/op2/dispatch_slow.rs
+++ b/ops/op2/dispatch_slow.rs
@@ -92,7 +92,7 @@ pub(crate) fn generate_dispatch_slow(
   )?);
 
   let with_self = if generator_state.needs_self {
-    with_self(generator_state, &signature.ret_val)
+    with_self(generator_state, false, &signature.ret_val)
       .map_err(V8SignatureMappingError::NoSelfMapping)?
   } else {
     quote!()
@@ -246,6 +246,7 @@ pub(crate) fn with_js_runtime_state(
 
 pub(crate) fn with_self(
   generator_state: &mut GeneratorState,
+  is_async: bool,
   ret_val: &RetVal,
 ) -> Result<TokenStream, V8MappingError> {
   generator_state.needs_opctx = true;
@@ -254,6 +255,7 @@ pub(crate) fn with_self(
     format!("expected {}", &generator_state.self_ty),
   )?;
   let tokens = if matches!(ret_val, RetVal::Future(_) | RetVal::FutureResult(_))
+    || is_async
   {
     let tokens = gs_quote!(generator_state(self_ty, fn_args, scope) => {
       let Some(self_) = deno_core::_ops::CppGcObjectGuard::<#self_ty>::try_new_from_cppgc_object(&mut *#scope, #fn_args.this().into()) else {
@@ -617,7 +619,7 @@ pub fn from_arg(
           let #arg_ident = if #arg_ident.is_null_or_undefined() {
             None
           } else if let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(#arg_ident) {
-            Some(#arg_ident)
+            Some(#arg_ident as _)
           } else {
             #throw_exception;
           };
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index 4187e56..aacf982 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -1473,19 +1473,17 @@ fn parse_type_state(ty: &Type) -> Result<Arg, ArgError> {
 
 fn parse_cppgc(position: Position, ty: &Type) -> Result<Arg, ArgError> {
   match (position, ty) {
-    (Position::Arg, Type::Reference(of)) if of.mutability.is_none() => {
-      match &*of.elem {
-        Type::Path(of) => Ok(Arg::CppGcResource(stringify_token(&of.path))),
-        _ => Err(ArgError::InvalidCppGcType(stringify_token(&of.elem))),
-      }
-    }
+    (Position::Arg, Type::Reference(of)) => match &*of.elem {
+      Type::Path(of) => Ok(Arg::CppGcResource(stringify_token(&of.path))),
+      _ => Err(ArgError::InvalidCppGcType(stringify_token(ty))),
+    },
     (Position::Arg, Type::Path(of)) => {
       rules!(of.to_token_stream() => {
         ( Option < $ty:ty $(,)? > ) => {
           match ty {
-            Type::Reference(of) if of.mutability.is_none() => {
+            Type::Reference(of) => {
               match &*of.elem {
-                Type::Path(of) => Ok(Arg::OptionCppGcResource(stringify_token(&of.path))),
+                Type::Path(f) => Ok(Arg::OptionCppGcResource(stringify_token(&f.path))),
                 _ => Err(ArgError::InvalidCppGcType(stringify_token(&of.elem))),
               }
             }
@@ -2050,7 +2048,7 @@ mod tests {
   );
   expect_fail!(
     op_cppgc_resource_invalid_type,
-    ArgError("resource", InvalidCppGcType("[std :: fs :: File]")),
+    ArgError("resource", InvalidCppGcType("&[std :: fs :: File]")),
     fn f(#[cppgc] resource: &[std::fs::File]) {}
   );
   expect_fail!(


Test patch:
diff --git a/ops/op2/test_cases/sync/cppgc_resource.out b/ops/op2/test_cases/sync/cppgc_resource.out
index 196abce..41bed7a 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.out
+++ b/ops/op2/test_cases/sync/cppgc_resource.out
@@ -256,7 +256,7 @@ const fn op_option_cppgc_object() -> ::deno_core::_ops::OpDecl {
                 } else if let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
                 >(arg0) {
-                    Some(arg0)
+                    Some(arg0 as _)
                 } else {
                     fast_api_callback_options.fallback = true;
                     return unsafe { std::mem::zeroed() };
@@ -284,7 +284,7 @@ const fn op_option_cppgc_object() -> ::deno_core::_ops::OpDecl {
                 } else if let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
                 >(arg0) {
-                    Some(arg0)
+                    Some(arg0 as _)
                 } else {
                     let mut scope = unsafe { deno_core::v8::CallbackScope::new(info) };
                     let msg = deno_core::v8::String::new_from_one_byte(
@@ -599,6 +599,173 @@ const fn op_use_cppgc_object() -> ::deno_core::_ops::OpDecl {
     <op_use_cppgc_object as ::deno_core::_ops::Op>::DECL
 }
 
+#[allow(non_camel_case_types)]
+const fn op_use_cppgc_object_mut() -> ::deno_core::_ops::OpDecl {
+    #[allow(non_camel_case_types)]
+    struct op_use_cppgc_object_mut {
+        _unconstructable: ::std::marker::PhantomData<()>,
+    }
+    impl ::deno_core::_ops::Op for op_use_cppgc_object_mut {
+        const NAME: &'static str = stringify!(op_use_cppgc_object_mut);
+        const DECL: ::deno_core::_ops::OpDecl = ::deno_core::_ops::OpDecl::new_internal_op2(
+            ::deno_core::__op_name_fast!(op_use_cppgc_object_mut),
+            false,
+            false,
+            1usize as u8,
+            Self::v8_fn_ptr as _,
+            Self::v8_fn_ptr_metrics as _,
+            Some({
+                use deno_core::v8::fast_api::Type;
+                use deno_core::v8::fast_api::CType;
+                deno_core::v8::fast_api::FastFunction::new_with_bigint(
+                    &[Type::V8Value, Type::V8Value, Type::CallbackOptions],
+                    CType::Void,
+                    Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+                )
+            }),
+            Some({
+                use deno_core::v8::fast_api::Type;
+                use deno_core::v8::fast_api::CType;
+                deno_core::v8::fast_api::FastFunction::new_with_bigint(
+                    &[Type::V8Value, Type::V8Value, Type::CallbackOptions],
+                    CType::Void,
+                    Self::v8_fn_ptr_fast_metrics as *const ::std::ffi::c_void,
+                )
+            }),
+            ::deno_core::OpMetadata {
+                ..::deno_core::OpMetadata::default()
+            },
+        );
+    }
+    impl op_use_cppgc_object_mut {
+        pub const fn name() -> &'static str {
+            stringify!(op_use_cppgc_object_mut)
+        }
+        #[allow(clippy::too_many_arguments)]
+        extern "C" fn v8_fn_ptr_fast_metrics<'s>(
+            this: deno_core::v8::Local<deno_core::v8::Object>,
+            arg0: deno_core::v8::Local<deno_core::v8::Value>,
+            fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions<
+                's,
+            >,
+        ) -> () {
+            let fast_api_callback_options: &'s mut _ = unsafe {
+                &mut *fast_api_callback_options
+            };
+            let opctx: &'s _ = unsafe {
+                &*(deno_core::v8::Local::<
+                    deno_core::v8::External,
+                >::cast(unsafe { fast_api_callback_options.data.data })
+                    .value() as *const deno_core::_ops::OpCtx)
+            };
+            deno_core::_ops::dispatch_metrics_fast(
+                opctx,
+                deno_core::_ops::OpMetricsEvent::Dispatched,
+            );
+            let res = Self::v8_fn_ptr_fast(this, arg0, fast_api_callback_options);
+            deno_core::_ops::dispatch_metrics_fast(
+                opctx,
+                deno_core::_ops::OpMetricsEvent::Completed,
+            );
+            res
+        }
+        #[allow(clippy::too_many_arguments)]
+        extern "C" fn v8_fn_ptr_fast<'s>(
+            this: deno_core::v8::Local<deno_core::v8::Object>,
+            arg0: deno_core::v8::Local<deno_core::v8::Value>,
+            fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions<
+                's,
+            >,
+        ) -> () {
+            #[cfg(debug_assertions)]
+            let _reentrancy_check_guard = deno_core::_ops::reentrancy_check(
+                &<Self as deno_core::_ops::Op>::DECL,
+            );
+            let fast_api_callback_options: &'s mut _ = unsafe {
+                &mut *fast_api_callback_options
+            };
+            let result = {
+                let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<Wrap>(arg0)
+                else {
+                    fast_api_callback_options.fallback = true;
+                    return unsafe { std::mem::zeroed() };
+                };
+                Self::call(arg0)
+            };
+            result as _
+        }
+        #[inline(always)]
+        fn slow_function_impl<'s>(
+            info: &'s deno_core::v8::FunctionCallbackInfo,
+        ) -> usize {
+            #[cfg(debug_assertions)]
+            let _reentrancy_check_guard = deno_core::_ops::reentrancy_check(
+                &<Self as deno_core::_ops::Op>::DECL,
+            );
+            let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(info);
+            let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(
+                info,
+            );
+            let result = {
+                let arg0 = args.get(0usize as i32);
+                let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<Wrap>(arg0)
+                else {
+                    let mut scope = unsafe { deno_core::v8::CallbackScope::new(info) };
+                    let msg = deno_core::v8::String::new_from_one_byte(
+                            &mut scope,
+                            "expected Wrap".as_bytes(),
+                            deno_core::v8::NewStringType::Normal,
+                        )
+                        .unwrap();
+                    let exc = deno_core::v8::Exception::type_error(&mut scope, msg);
+                    scope.throw_exception(exc);
+                    return 1;
+                };
+                Self::call(arg0)
+            };
+            deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv);
+            return 0;
+        }
+        extern "C" fn v8_fn_ptr<'s>(info: *const deno_core::v8::FunctionCallbackInfo) {
+            let info: &'s _ = unsafe { &*info };
+            Self::slow_function_impl(info);
+        }
+        extern "C" fn v8_fn_ptr_metrics<'s>(
+            info: *const deno_core::v8::FunctionCallbackInfo,
+        ) {
+            let info: &'s _ = unsafe { &*info };
+            let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(
+                info,
+            );
+            let opctx: &'s _ = unsafe {
+                &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data())
+                    .value() as *const deno_core::_ops::OpCtx)
+            };
+            deno_core::_ops::dispatch_metrics_slow(
+                opctx,
+                deno_core::_ops::OpMetricsEvent::Dispatched,
+            );
+            let res = Self::slow_function_impl(info);
+            if res == 0 {
+                deno_core::_ops::dispatch_metrics_slow(
+                    opctx,
+                    deno_core::_ops::OpMetricsEvent::Completed,
+                );
+            } else {
+                deno_core::_ops::dispatch_metrics_slow(
+                    opctx,
+                    deno_core::_ops::OpMetricsEvent::Error,
+                );
+            }
+        }
+    }
+    impl op_use_cppgc_object_mut {
+        #[inline(always)]
+        fn call(_wrap: &mut Wrap) {}
+    }
+    <op_use_cppgc_object_mut as ::deno_core::_ops::Op>::DECL
+}
+
 #[allow(non_camel_case_types)]
 const fn op_make_cppgc_object_option() -> ::deno_core::_ops::OpDecl {
     #[allow(non_camel_case_types)]
diff --git a/ops/op2/test_cases/sync/cppgc_resource.rs b/ops/op2/test_cases/sync/cppgc_resource.rs
index d2fa98c..369cb30 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.rs
+++ b/ops/op2/test_cases/sync/cppgc_resource.rs
@@ -22,6 +22,9 @@ fn op_make_cppgc_object() -> Wrap {
 #[op2(fast)]
 fn op_use_cppgc_object(#[cppgc] _wrap: &Wrap) {}
 
+#[op2(fast)]
+fn op_use_cppgc_object_mut(#[cppgc] _wrap: &mut Wrap) {}
+
 #[op2]
 #[cppgc]
 fn op_make_cppgc_object_option() -> Option<Wrap> {
diff --git a/ops/op2/test_cases_fail/lifetimes.rs b/ops/op2/test_cases_fail/lifetimes.rs
index bafad16..497c9f0 100644
--- a/ops/op2/test_cases_fail/lifetimes.rs
+++ b/ops/op2/test_cases_fail/lifetimes.rs
@@ -14,3 +14,6 @@ fn op_use_cppgc_object(#[cppgc] _wrap: &'static Wrap) {}
 
 #[op2(fast)]
 fn op_use_buffer(#[buffer] _buffer: &'static [u8]) {}
+
+#[op2(async)]
+async fn op_use_cppgc_object_mut_async(#[cppgc] _wrap: &mut Wrap) {}
diff --git a/ops/op2/test_cases_fail/lifetimes.stderr b/ops/op2/test_cases_fail/lifetimes.stderr
index 89fd320..1de84be 100644
--- a/ops/op2/test_cases_fail/lifetimes.stderr
+++ b/ops/op2/test_cases_fail/lifetimes.stderr
@@ -17,6 +17,26 @@ error: unused import: `std::future::Future`
 6 | use std::future::Future;
   |     ^^^^^^^^^^^^^^^^^^^
 
+error[E0308]: mismatched types
+  --> ../op2/test_cases_fail/lifetimes.rs:18:1
+   |
+18 | #[op2(async)]
+   | ^^^^^^^^^^^^^
+   | |
+   | types differ in mutability
+   | arguments to this function are incorrect
+   |
+   = note: expected mutable reference `&mut Wrap`
+                      found reference `&Wrap`
+note: associated function defined here
+  --> ../op2/test_cases_fail/lifetimes.rs:18:1
+   |
+18 | #[op2(async)]
+   | ^^^^^^^^^^^^^
+19 | async fn op_use_cppgc_object_mut_async(#[cppgc] _wrap: &mut Wrap) {}
+   |                                                 ----------------
+   = note: this error originates in the attribute macro `op2` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0521]: borrowed data escapes outside of associated function
   --> ../op2/test_cases_fail/lifetimes.rs:12:1
    |