Repository: denoland/deno_core
Pull request #793 (2024-06-21 08:08:09)

Revert "Allow `#[cppgc] &mut T` in sync ops"
Reverts denoland/deno_core#791

See https://github.com/denoland/deno_core/pull/791#pullrequestreview-2132050958

The following needs to be addressed before relanding:
1. Disallow multiple `&mut T` in the same op.
2. Make a `try_unwrap_cppgc_object` unsafe variant.
3. Disallow use in `op(reentrant)`

littledivy: Two PRs have landed that use this in Deno (that are no affected by these issues). I'll try to fix this before next deno_core release, if not then we can revert the Deno changes

Patch:
diff --git a/core/cppgc.rs b/core/cppgc.rs
index 254bb90..8b5c002 100644
--- a/core/cppgc.rs
+++ b/core/cppgc.rs
@@ -43,17 +43,10 @@ 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 mut T> {
+) -> Option<&'sc T> {
   let Ok(obj): Result<v8::Local<v8::Object>, _> = val.try_into() else {
     return None;
   };
@@ -69,13 +62,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 *mut InnerMember);
-    obj.ptr.cast::<CppGcObject<T>>().as_mut().unwrap()
+    let obj = &*(ptr as *const v8::cppgc::InnerMember);
+    obj.get::<CppGcObject<T>>()
   };
 
   if member.tag != TypeId::of::<T>() {
     return None;
   }
 
-  Some(&mut member.member)
+  Some(&member.member)
 }
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
index 05743d2..1d22ed7 100644
--- a/core/runtime/ops.rs
+++ b/core/runtime/ops.rs
@@ -610,7 +610,6 @@ 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,
@@ -1928,14 +1927,6 @@ 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(
@@ -1953,7 +1944,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_set_cppgc_resource",
+      "op_test_make_cppgc_resource, op_test_get_cppgc_resource, op_test_get_cppgc_resource_option, op_test_make_cppgc_resource_option",
       r"
       const resource = op_test_make_cppgc_resource();
       assert((await op_test_get_cppgc_resource(resource)) === 42);
@@ -1962,9 +1953,7 @@ 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);
-      op_test_set_cppgc_resource(resource, 43);
-      assert((await op_test_get_cppgc_resource(resource)) == 43);"
+      assert((await op_test_get_cppgc_resource(resource3)) === 42);",
     ).await?;
     Ok(())
   }
diff --git a/ops/op2/dispatch_async.rs b/ops/op2/dispatch_async.rs
index b07dece..a5ec579 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, true, &signature.ret_val)
+    with_self(generator_state, &signature.ret_val)
       .map_err(V8SignatureMappingError::NoSelfMapping)?
   } else {
     quote!()
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index e979f30..ea0325c 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 as _)
+          Some(#arg_ident)
         } 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 4e91496..96c52c0 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, false, &signature.ret_val)
+    with_self(generator_state, &signature.ret_val)
       .map_err(V8SignatureMappingError::NoSelfMapping)?
   } else {
     quote!()
@@ -246,7 +246,6 @@ 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;
@@ -255,7 +254,6 @@ 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 {
@@ -619,7 +617,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 as _)
+            Some(#arg_ident)
           } else {
             #throw_exception;
           };
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index aacf982..4187e56 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -1473,17 +1473,19 @@ 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)) => match &*of.elem {
-      Type::Path(of) => Ok(Arg::CppGcResource(stringify_token(&of.path))),
-      _ => Err(ArgError::InvalidCppGcType(stringify_token(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::Path(of)) => {
       rules!(of.to_token_stream() => {
         ( Option < $ty:ty $(,)? > ) => {
           match ty {
-            Type::Reference(of) => {
+            Type::Reference(of) if of.mutability.is_none() => {
               match &*of.elem {
-                Type::Path(f) => Ok(Arg::OptionCppGcResource(stringify_token(&f.path))),
+                Type::Path(of) => Ok(Arg::OptionCppGcResource(stringify_token(&of.path))),
                 _ => Err(ArgError::InvalidCppGcType(stringify_token(&of.elem))),
               }
             }
@@ -2048,7 +2050,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 41bed7a..196abce 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 as _)
+                    Some(arg0)
                 } 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 as _)
+                    Some(arg0)
                 } else {
                     let mut scope = unsafe { deno_core::v8::CallbackScope::new(info) };
                     let msg = deno_core::v8::String::new_from_one_byte(
@@ -599,173 +599,6 @@ 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 369cb30..d2fa98c 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.rs
+++ b/ops/op2/test_cases/sync/cppgc_resource.rs
@@ -22,9 +22,6 @@ 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 497c9f0..bafad16 100644
--- a/ops/op2/test_cases_fail/lifetimes.rs
+++ b/ops/op2/test_cases_fail/lifetimes.rs
@@ -14,6 +14,3 @@ 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 1de84be..89fd320 100644
--- a/ops/op2/test_cases_fail/lifetimes.stderr
+++ b/ops/op2/test_cases_fail/lifetimes.stderr
@@ -17,26 +17,6 @@ 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
    |