fix(ops): defer mut ref for ops2 scope (denoland#21)

Allows returning from functions that take a HandleScope
hardfist · Jul 8, 2023 · da8c117 · da8c117
1 parent b8639d5
commit da8c117
Show file tree

Hide file tree

Showing 17 changed files with 146 additions and 60 deletions.
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
@@ -356,6 +356,7 @@ mod tests {
       op_test_v8_type_return_option,
       op_test_v8_type_handle_scope,
       op_test_v8_type_handle_scope_obj,
+      op_test_v8_type_handle_scope_result,
       op_test_serde_v8,
     ]
   );
@@ -758,6 +759,18 @@ mod tests {
     o.get(scope, key)
   }
 
+  /// Extract whatever lives in "key" from the object.
+  #[op2(core)]
+  pub fn op_test_v8_type_handle_scope_result<'s>(
+    scope: &mut v8::HandleScope<'s>,
+    o: &v8::Object,
+  ) -> Result<v8::Local<'s, v8::Value>, AnyError> {
+    let key = v8::String::new(scope, "key").unwrap().into();
+    o.get(scope, key)
+      .filter(|v| !v.is_null_or_undefined())
+      .ok_or(generic_error("error!!!"))
+  }
+
   #[tokio::test]
   pub async fn test_op_v8_types() -> Result<(), Box<dyn std::error::Error>> {
     for (a, b) in [("a", 1), ("b", 2), ("c", 3)] {
@@ -785,9 +798,17 @@ mod tests {
         "{'no_key': 'abc'}",
         "null",
       ),
+      (
+        "op_test_v8_type_handle_scope_result",
+        "{'key': 'abc'}",
+        "'abc'",
+      ),
     ] {
       run_test2(1, a, &format!("assert({a}({b}) == {c})"))?;
     }
+
+    // Test the error case for op_test_v8_type_handle_scope_result
+    run_test2(1, "op_test_v8_type_handle_scope_result", "try { op_test_v8_type_handle_scope_result({}); assert(false); } catch (e) {}")?;
     Ok(())
   }
 

diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
@@ -108,7 +108,7 @@ fn with_scope(generator_state: &mut GeneratorState) -> TokenStream {
     ..
   } = &generator_state;
 
-  quote!(let #scope = &mut unsafe { #deno_core::v8::CallbackScope::new(&*#info) };)
+  quote!(let mut #scope = unsafe { #deno_core::v8::CallbackScope::new(&*#info) };)
 }
 
 fn with_retval(generator_state: &mut GeneratorState) -> TokenStream {
@@ -221,34 +221,34 @@ pub fn from_arg(
     Arg::Option(Special::String) => {
       *needs_scope = true;
       quote! {
-        let #arg_ident = #arg_ident.to_rust_string_lossy(#scope);
+        let #arg_ident = #arg_ident.to_rust_string_lossy(&mut #scope);
       }
     }
     Arg::Special(Special::String) => {
       *needs_scope = true;
       quote! {
-        let #arg_ident = #arg_ident.to_rust_string_lossy(#scope);
+        let #arg_ident = #arg_ident.to_rust_string_lossy(&mut #scope);
       }
     }
     Arg::Special(Special::RefStr) => {
       *needs_scope = true;
       quote! {
         // Trade 1024 bytes of stack space for potentially non-allocating strings
         let mut #arg_temp: [::std::mem::MaybeUninit<u8>; 1024] = [::std::mem::MaybeUninit::uninit(); 1024];
-        let #arg_ident = &#deno_core::_ops::to_str(#scope, &#arg_ident, &mut #arg_temp);
+        let #arg_ident = &#deno_core::_ops::to_str(&mut #scope, &#arg_ident, &mut #arg_temp);
       }
     }
     Arg::Special(Special::CowStr) => {
       *needs_scope = true;
       quote! {
         // Trade 1024 bytes of stack space for potentially non-allocating strings
         let mut #arg_temp: [::std::mem::MaybeUninit<u8>; 1024] = [::std::mem::MaybeUninit::uninit(); 1024];
-        let #arg_ident = #deno_core::_ops::to_str(#scope, &#arg_ident, &mut #arg_temp);
+        let #arg_ident = #deno_core::_ops::to_str(&mut #scope, &#arg_ident, &mut #arg_temp);
       }
     }
     Arg::Ref(_, Special::HandleScope) => {
       *needs_scope = true;
-      quote!(let #arg_ident = #scope;)
+      quote!(let #arg_ident = &mut #scope;)
     }
     Arg::V8Ref(_, v8) => {
       let arg_ident = arg_ident.clone();
@@ -302,7 +302,7 @@ pub fn from_arg(
       let err = format_ident!("{}_err", arg_ident);
       let throw_exception = throw_type_error_string(generator_state, &err)?;
       quote! {
-        let #arg_ident = match #deno_core::_ops::serde_v8_to_rust(#scope, #arg_ident) {
+        let #arg_ident = match #deno_core::_ops::serde_v8_to_rust(&mut #scope, #arg_ident) {
           Ok(t) => t,
           Err(#err) => {
             #throw_exception;
@@ -411,7 +411,7 @@ pub fn return_value_infallible(
           // This should not fail in normal cases
           // TODO(mmastrac): This has extra allocations that we need to get rid of, especially if the string
           // is ASCII. We could make an "external Rust String" string in V8 from these and re-use the allocation.
-          let temp = #deno_core::v8::String::new(#scope, &#result).unwrap();
+          let temp = #deno_core::v8::String::new(&mut #scope, &#result).unwrap();
           #retval.set(temp.into());
         }
       }
@@ -463,7 +463,7 @@ pub fn return_value_infallible(
       let throw_exception = throw_type_error_string(generator_state, &err)?;
 
       quote! {
-        let #result = match #deno_core::_ops::serde_rust_to_v8(#scope, #result) {
+        let #result = match #deno_core::_ops::serde_rust_to_v8(&mut #scope, #result) {
           Ok(t) => t,
           Err(#err) => {
             #throw_exception
@@ -540,7 +540,7 @@ fn throw_exception(
     #maybe_opctx
     let opstate = ::std::cell::RefCell::borrow(&*#opctx.state);
     let exception = #deno_core::error::to_v8_error(
-      #scope,
+      &mut #scope,
       opstate.get_error_class_fn,
       &err,
     );
@@ -573,8 +573,8 @@ fn throw_type_error(
 
   Ok(quote! {
     #maybe_scope
-    let msg = #deno_core::v8::String::new_from_one_byte(#scope, #message.as_bytes(), #deno_core::v8::NewStringType::Normal).unwrap();
-    let exc = #deno_core::v8::Exception::error(#scope, msg);
+    let msg = #deno_core::v8::String::new_from_one_byte(&mut #scope, #message.as_bytes(), #deno_core::v8::NewStringType::Normal).unwrap();
+    let exc = #deno_core::v8::Exception::error(&mut #scope, msg);
     #scope.throw_exception(exc);
     return;
   })
@@ -598,8 +598,8 @@ fn throw_type_error_string(
   Ok(quote! {
     #maybe_scope
     // TODO(mmastrac): This might be allocating too much, even if it's on the error path
-    let msg = #deno_core::v8::String::new(#scope, &format!("{}", #deno_core::anyhow::Error::from(#message))).unwrap();
-    let exc = #deno_core::v8::Exception::error(#scope, msg);
+    let msg = #deno_core::v8::String::new(&mut #scope, &format!("{}", #deno_core::anyhow::Error::from(#message))).unwrap();
+    let exc = #deno_core::v8::Exception::error(&mut #scope, msg);
     #scope.throw_exception(exc);
     return;
   })

diff --git a/ops/op2/test_cases/sync/bool_result.out b/ops/op2/test_cases/sync/bool_result.out
@@ -85,10 +85,10 @@ impl op_bool {
                 as *const deno_core::_ops::OpCtx)
         };
         if let Some(err) = unsafe { opctx.unsafely_take_last_error_for_ops_only() } {
-            let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+            let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
             let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
             let exception = deno_core::error::to_v8_error(
-                scope,
+                &mut scope,
                 opstate.get_error_class_fn,
                 &err,
             );
@@ -103,10 +103,10 @@ impl op_bool {
                 rv.set_bool(result as bool);
             }
             Err(err) => {
-                let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+                let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
                 let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
                 let exception = deno_core::error::to_v8_error(
-                    scope,
+                    &mut scope,
                     opstate.get_error_class_fn,
                     &err,
                 );

diff --git a/ops/op2/test_cases/sync/result_primitive.out b/ops/op2/test_cases/sync/result_primitive.out
@@ -83,13 +83,13 @@ impl op_u32_with_result {
                 as *const deno_core::_ops::OpCtx)
         };
         if let Some(err) = unsafe { opctx.unsafely_take_last_error_for_ops_only() } {
-            let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+            let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
             let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
                 &*info
             });
             let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
             let exception = deno_core::error::to_v8_error(
-                scope,
+                &mut scope,
                 opstate.get_error_class_fn,
                 &err,
             );
@@ -102,13 +102,13 @@ impl op_u32_with_result {
                 rv.set_uint32(result as u32);
             }
             Err(err) => {
-                let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+                let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
                 let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
                     &*info
                 });
                 let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
                 let exception = deno_core::error::to_v8_error(
-                    scope,
+                    &mut scope,
                     opstate.get_error_class_fn,
                     &err,
                 );

diff --git a/ops/op2/test_cases/sync/result_scope.out b/ops/op2/test_cases/sync/result_scope.out
@@ -0,0 +1,61 @@
+#[allow(non_camel_case_types)]
+pub struct op_void_with_result {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_void_with_result {
+    const NAME: &'static str = stringify!(op_void_with_result);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl {
+        name: stringify!(op_void_with_result),
+        v8_fn_ptr: Self::v8_fn_ptr as _,
+        enabled: true,
+        fast_fn: None,
+        is_async: false,
+        is_unstable: false,
+        is_v8: false,
+        arg_count: 1usize as u8,
+    };
+}
+impl op_void_with_result {
+    pub const fn name() -> &'static str {
+        stringify!(op_void_with_result)
+    }
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        deno_core::_ops::OpDecl {
+            name: stringify!(op_void_with_result),
+            v8_fn_ptr: Self::v8_fn_ptr as _,
+            enabled: true,
+            fast_fn: None,
+            is_async: false,
+            is_unstable: false,
+            is_v8: false,
+            arg_count: 1usize as u8,
+        }
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+            &*info
+        });
+        let arg0 = &mut scope;
+        let result = Self::call(arg0);
+        match result {
+            Ok(result) => {}
+            Err(err) => {
+                let opctx = unsafe {
+                    &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data())
+                        .value() as *const deno_core::_ops::OpCtx)
+                };
+                let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
+                let exception = deno_core::error::to_v8_error(
+                    &mut scope,
+                    opstate.get_error_class_fn,
+                    &err,
+                );
+                scope.throw_exception(exception);
+                return;
+            }
+        };
+    }
+    #[inline(always)]
+    pub fn call(scope: &mut v8::HandleScope) -> Result<(), AnyError> {}
+}
diff --git a/ops/op2/test_cases/sync/result_scope.rs b/ops/op2/test_cases/sync/result_scope.rs
@@ -0,0 +1,4 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+#[op2]
+pub fn op_void_with_result(scope: &mut v8::HandleScope) -> Result<(), AnyError> {}
diff --git a/ops/op2/test_cases/sync/result_void.out b/ops/op2/test_cases/sync/result_void.out
@@ -80,13 +80,13 @@ impl op_void_with_result {
                 as *const deno_core::_ops::OpCtx)
         };
         if let Some(err) = unsafe { opctx.unsafely_take_last_error_for_ops_only() } {
-            let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+            let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
             let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
                 &*info
             });
             let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
             let exception = deno_core::error::to_v8_error(
-                scope,
+                &mut scope,
                 opstate.get_error_class_fn,
                 &err,
             );
@@ -97,13 +97,13 @@ impl op_void_with_result {
         match result {
             Ok(result) => {}
             Err(err) => {
-                let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+                let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
                 let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
                     &*info
                 });
                 let opstate = ::std::cell::RefCell::borrow(&*opctx.state);
                 let exception = deno_core::error::to_v8_error(
-                    scope,
+                    &mut scope,
                     opstate.get_error_class_fn,
                     &err,
                 );

diff --git a/ops/op2/test_cases/sync/serde_v8.out b/ops/op2/test_cases/sync/serde_v8.out
@@ -32,37 +32,37 @@ impl op_serde_v8 {
         }
     }
     extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
-        let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
         let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
             &*info
         });
         let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
             &*info
         });
         let arg0 = args.get(0usize as i32);
-        let arg0 = match deno_core::_ops::serde_v8_to_rust(scope, arg0) {
+        let arg0 = match deno_core::_ops::serde_v8_to_rust(&mut scope, arg0) {
             Ok(t) => t,
             Err(arg0_err) => {
                 let msg = deno_core::v8::String::new(
-                        scope,
+                        &mut scope,
                         &format!("{}", deno_core::anyhow::Error::from(arg0_err)),
                     )
                     .unwrap();
-                let exc = deno_core::v8::Exception::error(scope, msg);
+                let exc = deno_core::v8::Exception::error(&mut scope, msg);
                 scope.throw_exception(exc);
                 return;
             }
         };
         let result = Self::call(arg0);
-        let result = match deno_core::_ops::serde_rust_to_v8(scope, result) {
+        let result = match deno_core::_ops::serde_rust_to_v8(&mut scope, result) {
             Ok(t) => t,
             Err(rv_err) => {
                 let msg = deno_core::v8::String::new(
-                        scope,
+                        &mut scope,
                         &format!("{}", deno_core::anyhow::Error::from(rv_err)),
                     )
                     .unwrap();
-                let exc = deno_core::v8::Exception::error(scope, msg);
+                let exc = deno_core::v8::Exception::error(&mut scope, msg);
                 scope.throw_exception(exc);
                 return;
             }

diff --git a/ops/op2/test_cases/sync/string_cow.out b/ops/op2/test_cases/sync/string_cow.out
@@ -57,7 +57,7 @@ impl op_string_cow {
         result
     }
     extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
-        let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
         let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
             &*info
         });
@@ -66,7 +66,7 @@ impl op_string_cow {
         });
         let arg0 = args.get(0usize as i32);
         let mut arg0_temp: [::std::mem::MaybeUninit<u8>; 1024] = [::std::mem::MaybeUninit::uninit(); 1024];
-        let arg0 = deno_core::_ops::to_str(scope, &arg0, &mut arg0_temp);
+        let arg0 = deno_core::_ops::to_str(&mut scope, &arg0, &mut arg0_temp);
         let result = Self::call(arg0);
         rv.set_uint32(result as u32);
     }

diff --git a/ops/op2/test_cases/sync/string_option_return.out b/ops/op2/test_cases/sync/string_option_return.out
@@ -32,7 +32,7 @@ impl op_string_return {
         }
     }
     extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
-        let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
         let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
             &*info
         });
@@ -41,7 +41,7 @@ impl op_string_return {
             if result.is_empty() {
                 rv.set_empty_string();
             } else {
-                let temp = deno_core::v8::String::new(scope, &result).unwrap();
+                let temp = deno_core::v8::String::new(&mut scope, &result).unwrap();
                 rv.set(temp.into());
             }
         } else {