Repository: denoland/deno_core
Pull request #672 (2024-03-22 14:31:05)

feat(op2): #[cppgc] return values

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/denoland/deno_core/pull/672?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) Report
Attention: Patch coverage is `88.23529%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 80.94%. Comparing base [(`8765f38`)](https://app.codecov.io/gh/denoland/deno_core/commit/8765f387b99190c74a5be17ac9ef9cf4d66a96b2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) to head [(`9bf1f66`)](https://app.codecov.io/gh/denoland/deno_core/pull/672?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).

> :exclamation: Current head 9bf1f66 differs from pull request most recent head fc57d82. Consider uploading reports for the commit fc57d82 to get more accurate results

| [Files](https://app.codecov.io/gh/denoland/deno_core/pull/672?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) | Patch % | Lines |
|---|---|---|
| [ops/op2/dispatch\_slow.rs](https://app.codecov.io/gh/denoland/deno_core/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-b3BzL29wMi9kaXNwYXRjaF9zbG93LnJz) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |
| [ops/op2/signature.rs](https://app.codecov.io/gh/denoland/deno_core/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-b3BzL29wMi9zaWduYXR1cmUucnM=) | 90.90% | [1 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main     #672   +/-   ##
=======================================
  Coverage   80.93%   80.94%           
=======================================
  Files          97       97           
  Lines       24419    24425    +6     
=======================================
+ Hits        19764    19770    +6     
  Misses       4655     4655           
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/denoland/deno_core/pull/672?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


Patch:
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
index a4d2889..f616a26 100644
--- a/core/runtime/ops.rs
+++ b/core/runtime/ops.rs
@@ -1854,10 +1854,9 @@ mod tests {
   }
 
   #[op2]
-  pub fn op_test_make_cppgc_resource<'s>(
-    scope: &'s mut v8::HandleScope,
-  ) -> v8::Local<'s, v8::Object> {
-    crate::cppgc::make_cppgc_object(scope, TestResource { value: 42 })
+  #[cppgc]
+  pub fn op_test_make_cppgc_resource() -> TestResource {
+    TestResource { value: 42 }
   }
 
   #[op2(fast)]
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index 9960368..4b9766b 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -876,7 +876,8 @@ fn map_retval_to_v8_fastcall_type(
     | Arg::V8Local(_)
     | Arg::OptionV8Local(_)
     | Arg::OptionV8Global(_)
-    | Arg::OptionV8Ref(..) => return Ok(None),
+    | Arg::OptionV8Ref(..)
+    | Arg::CppGcResource(..) => return Ok(None),
     Arg::Buffer(..) | Arg::OptionBuffer(..) => return Ok(None),
     Arg::External(..) => V8FastCallType::Pointer,
     _ => return Err("a fast return value"),
diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
index c0b7221..bb08de1 100644
--- a/ops/op2/dispatch_slow.rs
+++ b/ops/op2/dispatch_slow.rs
@@ -715,6 +715,10 @@ pub fn return_value_infallible(
     ArgMarker::Number => {
       gs_quote!(generator_state(result) => (deno_core::_ops::RustToV8Marker::<deno_core::_ops::NumberMarker, _>::from(#result)))
     }
+    ArgMarker::Cppgc => {
+      generator_state.needs_scope = true;
+      gs_quote!(generator_state(scope, result) => (deno_core::v8::Local::<deno_core::v8::Value>::from(deno_core::cppgc::make_cppgc_object(&mut #scope, #result))))
+    }
     ArgMarker::None => gs_quote!(generator_state(result) => (#result)),
   };
   let res = match ret_type.slow_retval() {
@@ -777,6 +781,9 @@ pub fn return_value_v8_value(
     ArgMarker::Number => {
       quote!(deno_core::_ops::RustToV8Marker::<deno_core::_ops::NumberMarker, _>::from(#result))
     }
+    ArgMarker::Cppgc => {
+      quote!(deno_core::cppgc::make_cppgc_object(&mut #scope, #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 e22147c..ad45e2d 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -476,6 +476,7 @@ impl Arg {
         Arg::OptionBuffer(.., BufferSource::TypedArray) => {
           ArgSlowRetval::V8LocalFalliable
         }
+        Arg::CppGcResource(_) => ArgSlowRetval::V8LocalNoScope,
         _ => ArgSlowRetval::None,
       }
     }
@@ -491,6 +492,7 @@ impl Arg {
       Arg::SerdeV8(_) => ArgMarker::Serde,
       Arg::Numeric(NumericArg::__SMI__, _) => ArgMarker::Smi,
       Arg::Numeric(_, NumericFlag::Number) => ArgMarker::Number,
+      Arg::CppGcResource(_) => ArgMarker::Cppgc,
       _ => ArgMarker::None,
     }
   }
@@ -527,6 +529,8 @@ pub enum ArgMarker {
   Number,
   /// This buffer type should be serialized as an ArrayBuffer.
   ArrayBuffer,
+  /// This type should be wrapped as a cppgc V8 object.
+  Cppgc,
 }
 
 #[derive(Debug)]
@@ -1449,12 +1453,17 @@ fn parse_type_state(ty: &Type) -> Result<Arg, ArgError> {
   Ok(s)
 }
 
-fn parse_cppgc(ty: &Type) -> Result<Arg, ArgError> {
-  match ty {
-    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(ty))),
-    },
+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(ty))),
+      }
+    }
+    (Position::RetVal, Type::Path(of)) => {
+      Ok(Arg::CppGcResource(stringify_token(&of.path)))
+    }
     _ => Err(ArgError::ExpectedCppGcReference(stringify_token(ty))),
   }
 }
@@ -1469,7 +1478,7 @@ pub(crate) fn parse_type(
 
   if let Some(primary) = attrs.primary {
     match primary {
-      AttributeModifier::CppGcResource => return parse_cppgc(ty),
+      AttributeModifier::CppGcResource => return parse_cppgc(position, ty),
       AttributeModifier::Serde => match ty {
         Type::Tuple(of) => {
           return Ok(Arg::SerdeV8(stringify_token(of)));


Test patch:
diff --git a/ops/op2/test_cases/sync/cppgc_resource.out b/ops/op2/test_cases/sync/cppgc_resource.out
index e0db584..5c99b71 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.out
+++ b/ops/op2/test_cases/sync/cppgc_resource.out
@@ -152,3 +152,85 @@ impl op_file {
     #[inline(always)]
     fn call(_file: &std::fs::File) {}
 }
+
+#[allow(non_camel_case_types)]
+struct op_make_cppgc_object {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl ::deno_core::_ops::Op for op_make_cppgc_object {
+    const NAME: &'static str = stringify!(op_make_cppgc_object);
+    const DECL: ::deno_core::_ops::OpDecl = ::deno_core::_ops::OpDecl::new_internal_op2(
+        ::deno_core::__op_name_fast!(op_make_cppgc_object),
+        false,
+        false,
+        0usize as u8,
+        Self::v8_fn_ptr as _,
+        Self::v8_fn_ptr_metrics as _,
+        None,
+        None,
+        ::deno_core::OpMetadata {
+            ..::deno_core::OpMetadata::default()
+        },
+    );
+}
+impl op_make_cppgc_object {
+    pub const fn name() -> &'static str {
+        stringify!(op_make_cppgc_object)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    #[inline(always)]
+    fn slow_function_impl(info: *const 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 scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
+            &*info
+        });
+        let result = { Self::call() };
+        rv.set(
+            deno_core::_ops::RustToV8NoScope::to_v8(
+                deno_core::v8::Local::<
+                    deno_core::v8::Value,
+                >::from(deno_core::cppgc::make_cppgc_object(&mut scope, result)),
+            ),
+        );
+        return 0;
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        Self::slow_function_impl(info);
+    }
+    extern "C" fn v8_fn_ptr_metrics(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+            &*info
+        });
+        let opctx = 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,
+            );
+        }
+    }
+    #[inline(always)]
+    fn call() -> Wrap {
+        Wrap
+    }
+}
diff --git a/ops/op2/test_cases/sync/cppgc_resource.rs b/ops/op2/test_cases/sync/cppgc_resource.rs
index 21d0ef9..2c77500 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.rs
+++ b/ops/op2/test_cases/sync/cppgc_resource.rs
@@ -4,3 +4,11 @@ deno_ops_compile_test_runner::prelude!();
 
 #[op2(fast)]
 fn op_file(#[cppgc] _file: &std::fs::File) {}
+
+struct Wrap;
+
+#[op2]
+#[cppgc]
+fn op_make_cppgc_object() -> Wrap {
+    Wrap
+}