Repository: denoland/deno_core (rust)

Issue:
`op2` doesn't support `Result<*mut c_void>`

Pull request:
fix(ops): multiple fixes for #[op2]
Fixes #174, #185, and provides an error for #186

Patch:
diff --git a/ops/op2/config.rs b/ops/op2/config.rs
index d242f18..ec14c76 100644
--- a/ops/op2/config.rs
+++ b/ops/op2/config.rs
@@ -123,17 +123,17 @@ impl MacroConfig {
   pub fn from_maybe_attribute_tokens(
     tokens: TokenStream,
   ) -> Result<Option<Self>, Op2Error> {
-    Ok(rules!(tokens => {
+    rules!(tokens => {
       (#[op2]) => {
-        Some(MacroConfig::default())
+        Ok(Some(MacroConfig::default()))
       }
       (#[op2 $flags:tt ]) => {
-        Some(MacroConfig::from_token_tree(flags)?)
+        Ok(Some(MacroConfig::from_token_tree(flags)?))
       }
       (#[$_attr:meta]) => {
-        None
+        Ok(None)
       }
-    }))
+    })
   }
 }
 
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index abb8ca7..c48406b 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -222,7 +222,9 @@ pub fn generate_dispatch_fast(
             // allowing us to perform this one little bit of mutable magic.
             unsafe { #opctx.unsafely_set_last_error_for_ops_only(err); }
             #fast_api_callback_options.fallback = true;
-            return ::std::default::Default::default();
+
+            // SAFETY: All fast return types have zero as a valid value
+            return unsafe { std::mem::zeroed() };
           }
         };
       }
@@ -392,7 +394,8 @@ fn map_v8_fastcall_arg_to_arg(
         *needs_fast_api_callback_options = true;
         Ok(quote! {
           #fast_api_callback_options.fallback = true;
-          return ::std::default::Default::default();
+          // SAFETY: All fast return types have zero as a valid value
+          return unsafe { std::mem::zeroed() };
         })
       };
       let extract_intermediate = v8_intermediate_to_arg(&arg_ident, arg);
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index 961a4af..443c70c 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -12,6 +12,7 @@ use strum::IntoEnumIterator;
 use strum::IntoStaticStr;
 use strum_macros::EnumIter;
 use strum_macros::EnumString;
+use syn::AttrStyle;
 use syn::Attribute;
 use syn::FnArg;
 use syn::GenericParam;
@@ -619,6 +620,8 @@ pub enum SignatureError {
 pub enum AttributeError {
   #[error("Unknown or invalid attribute '{0}'")]
   InvalidAttribute(String),
+  #[error("Invalid inner attribute (#![attr]) in this position. Use an equivalent outer attribute (#[attr]) on the function instead.")]
+  InvalidInnerAttribute,
   #[error("Too many attributes")]
   TooManyAttributes,
 }
@@ -930,6 +933,9 @@ fn parse_attribute(
   attr: &Attribute,
 ) -> Result<Option<AttributeModifier>, AttributeError> {
   let tokens = attr.into_token_stream();
+  if matches!(attr.style, AttrStyle::Inner(_)) {
+    return Err(AttributeError::InvalidInnerAttribute);
+  }
   let res = std::panic::catch_unwind(|| {
     rules!(tokens => {
       (#[bigint]) => Some(AttributeModifier::Bigint),
@@ -945,6 +951,7 @@ fn parse_attribute(
       (#[global]) => Some(AttributeModifier::Global),
       (#[allow ($_rule:path)]) => None,
       (#[doc = $_attr:literal]) => None,
+      (#[cfg $_cfg:tt]) => None,
     })
   }).map_err(|_| AttributeError::InvalidAttribute(stringify_token(attr)))?;
   Ok(res)


Test patch:
diff --git a/ops/op2/test_cases/sync/bool_result.out b/ops/op2/test_cases/sync/bool_result.out
index 8dfe836..f9a977b 100644
--- a/ops/op2/test_cases/sync/bool_result.out
+++ b/ops/op2/test_cases/sync/bool_result.out
@@ -53,7 +53,7 @@ impl op_bool {
                     opctx.unsafely_set_last_error_for_ops_only(err);
                 }
                 fast_api_callback_options.fallback = true;
-                return ::std::default::Default::default();
+                return unsafe { std::mem::zeroed() };
             }
         };
         result as _
diff --git a/ops/op2/test_cases/sync/cfg.out b/ops/op2/test_cases/sync/cfg.out
new file mode 100644
index 0000000..3bf7aa2
--- /dev/null
+++ b/ops/op2/test_cases/sync/cfg.out
@@ -0,0 +1,101 @@
+#[allow(non_camel_case_types)]
+/// This is a doc comment.
+#[cfg(windows)]
+pub struct op_maybe_windows {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_maybe_windows {
+    const NAME: &'static str = stringify!(op_maybe_windows);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl::new_internal(
+        stringify!(op_maybe_windows),
+        false,
+        false,
+        false,
+        0usize as u8,
+        Self::v8_fn_ptr 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],
+                CType::Void,
+                Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+            )
+        }),
+    );
+}
+impl op_maybe_windows {
+    pub const fn name() -> &'static str {
+        stringify!(op_maybe_windows)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    fn v8_fn_ptr_fast(_: deno_core::v8::Local<deno_core::v8::Object>) -> () {
+        let result = { Self::call() };
+        result as _
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
+            &*info
+        });
+        let result = { Self::call() };
+        deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv)
+    }
+    #[inline(always)]
+    /// This is a doc comment.
+    #[cfg(windows)]
+    pub fn call() -> () {}
+}
+
+#[allow(non_camel_case_types)]
+/// This is a doc comment.
+#[cfg(not(windows))]
+pub struct op_maybe_windows {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_maybe_windows {
+    const NAME: &'static str = stringify!(op_maybe_windows);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl::new_internal(
+        stringify!(op_maybe_windows),
+        false,
+        false,
+        false,
+        0usize as u8,
+        Self::v8_fn_ptr 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],
+                CType::Void,
+                Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+            )
+        }),
+    );
+}
+impl op_maybe_windows {
+    pub const fn name() -> &'static str {
+        stringify!(op_maybe_windows)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    fn v8_fn_ptr_fast(_: deno_core::v8::Local<deno_core::v8::Object>) -> () {
+        let result = { Self::call() };
+        result as _
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
+            &*info
+        });
+        let result = { Self::call() };
+        deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv)
+    }
+    #[inline(always)]
+    /// This is a doc comment.
+    #[cfg(not(windows))]
+    pub fn call() -> () {}
+}
diff --git a/ops/op2/test_cases/sync/cfg.rs b/ops/op2/test_cases/sync/cfg.rs
new file mode 100644
index 0000000..5752af5
--- /dev/null
+++ b/ops/op2/test_cases/sync/cfg.rs
@@ -0,0 +1,13 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+#![deny(warnings)]
+deno_ops_compile_test_runner::prelude!();
+
+/// This is a doc comment.
+#[op2(fast)]
+#[cfg(windows)]
+pub fn op_maybe_windows() -> () {}
+
+/// This is a doc comment.
+#[op2(fast)]
+#[cfg(not(windows))]
+pub fn op_maybe_windows() -> () {}
diff --git a/ops/op2/test_cases/sync/clippy_allow.out b/ops/op2/test_cases/sync/clippy_allow.out
index f1de864..4c23b35 100644
--- a/ops/op2/test_cases/sync/clippy_allow.out
+++ b/ops/op2/test_cases/sync/clippy_allow.out
@@ -48,3 +48,52 @@ impl op_extra_annotation {
     #[allow(clippy::some_annotation)]
     pub fn call() -> () {}
 }
+
+#[allow(non_camel_case_types)]
+pub struct op_clippy_internal {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_clippy_internal {
+    const NAME: &'static str = stringify!(op_clippy_internal);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl::new_internal(
+        stringify!(op_clippy_internal),
+        false,
+        false,
+        false,
+        0usize as u8,
+        Self::v8_fn_ptr 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],
+                CType::Void,
+                Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+            )
+        }),
+    );
+}
+impl op_clippy_internal {
+    pub const fn name() -> &'static str {
+        stringify!(op_clippy_internal)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    fn v8_fn_ptr_fast(_: deno_core::v8::Local<deno_core::v8::Object>) -> () {
+        let result = { Self::call() };
+        result as _
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
+            &*info
+        });
+        let result = { Self::call() };
+        deno_core::_ops::RustToV8RetVal::to_v8_rv(result, &mut rv)
+    }
+    #[inline(always)]
+    pub fn call() -> () {
+        { #![allow(clippy::await_holding_refcell_ref)] }
+    }
+}
diff --git a/ops/op2/test_cases/sync/clippy_allow.rs b/ops/op2/test_cases/sync/clippy_allow.rs
index 6c42d4c..2b5aec7 100644
--- a/ops/op2/test_cases/sync/clippy_allow.rs
+++ b/ops/op2/test_cases/sync/clippy_allow.rs
@@ -6,3 +6,10 @@ deno_ops_compile_test_runner::prelude!();
 #[op2(fast)]
 #[allow(clippy::some_annotation)]
 pub fn op_extra_annotation() -> () {}
+
+#[op2(fast)]
+pub fn op_clippy_internal() -> () {
+  {
+    #![allow(clippy::await_holding_refcell_ref)]
+  }
+}
\ No newline at end of file
diff --git a/ops/op2/test_cases/sync/result_external.out b/ops/op2/test_cases/sync/result_external.out
new file mode 100644
index 0000000..6ccf984
--- /dev/null
+++ b/ops/op2/test_cases/sync/result_external.out
@@ -0,0 +1,105 @@
+#[allow(non_camel_case_types)]
+pub struct op_external_with_result {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_external_with_result {
+    const NAME: &'static str = stringify!(op_external_with_result);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl::new_internal(
+        stringify!(op_external_with_result),
+        false,
+        false,
+        false,
+        0usize as u8,
+        Self::v8_fn_ptr 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::CallbackOptions],
+                CType::Pointer,
+                Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+            )
+        }),
+    );
+}
+impl op_external_with_result {
+    pub const fn name() -> &'static str {
+        stringify!(op_external_with_result)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    fn v8_fn_ptr_fast(
+        _: deno_core::v8::Local<deno_core::v8::Object>,
+        fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
+    ) -> *mut ::std::ffi::c_void {
+        let fast_api_callback_options = unsafe { &mut *fast_api_callback_options };
+        let opctx = unsafe {
+            &*(deno_core::v8::Local::<
+                deno_core::v8::External,
+            >::cast(unsafe { fast_api_callback_options.data.data })
+                .value() as *const deno_core::_ops::OpCtx)
+        };
+        let result = { Self::call() };
+        let result = match result {
+            Ok(result) => result,
+            Err(err) => {
+                unsafe {
+                    opctx.unsafely_set_last_error_for_ops_only(err);
+                }
+                fast_api_callback_options.fallback = true;
+                return unsafe { std::mem::zeroed() };
+            }
+        };
+        result as _
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        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 opctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        if let Some(err) = unsafe { opctx.unsafely_take_last_error_for_ops_only() } {
+            let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
+            let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+                &*info
+            });
+            let err = err.into();
+            let exception = deno_core::error::to_v8_error(
+                &mut scope,
+                opctx.get_error_class_fn,
+                &err,
+            );
+            scope.throw_exception(exception);
+            return;
+        }
+        let result = { Self::call() };
+        match result {
+            Ok(result) => rv.set(deno_core::_ops::RustToV8::to_v8(result, &mut scope)),
+            Err(err) => {
+                let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+                    &*info
+                });
+                let err = err.into();
+                let exception = deno_core::error::to_v8_error(
+                    &mut scope,
+                    opctx.get_error_class_fn,
+                    &err,
+                );
+                scope.throw_exception(exception);
+                return;
+            }
+        };
+    }
+    #[inline(always)]
+    pub fn call() -> Result<*mut std::ffi::c_void, AnyError> {
+        Ok(0 as _)
+    }
+}
diff --git a/ops/op2/test_cases/sync/result_external.rs b/ops/op2/test_cases/sync/result_external.rs
new file mode 100644
index 0000000..742b5cc
--- /dev/null
+++ b/ops/op2/test_cases/sync/result_external.rs
@@ -0,0 +1,10 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+#![deny(warnings)]
+deno_ops_compile_test_runner::prelude!();
+
+use deno_core::error::AnyError;
+
+#[op2(fast)]
+pub fn op_external_with_result() -> Result<*mut std::ffi::c_void, AnyError> {
+    Ok(0 as _)
+}
diff --git a/ops/op2/test_cases/sync/result_primitive.out b/ops/op2/test_cases/sync/result_primitive.out
index 3df832a..620a8b0 100644
--- a/ops/op2/test_cases/sync/result_primitive.out
+++ b/ops/op2/test_cases/sync/result_primitive.out
@@ -49,7 +49,7 @@ impl op_u32_with_result {
                     opctx.unsafely_set_last_error_for_ops_only(err);
                 }
                 fast_api_callback_options.fallback = true;
-                return ::std::default::Default::default();
+                return unsafe { std::mem::zeroed() };
             }
         };
         result as _
diff --git a/ops/op2/test_cases/sync/result_void.out b/ops/op2/test_cases/sync/result_void.out
index 6065f1c..1b638d4 100644
--- a/ops/op2/test_cases/sync/result_void.out
+++ b/ops/op2/test_cases/sync/result_void.out
@@ -49,7 +49,7 @@ impl op_void_with_result {
                     opctx.unsafely_set_last_error_for_ops_only(err);
                 }
                 fast_api_callback_options.fallback = true;
-                return ::std::default::Default::default();
+                return unsafe { std::mem::zeroed() };
             }
         };
         result as _
diff --git a/ops/op2/test_cases/sync/v8_ref_option.out b/ops/op2/test_cases/sync/v8_ref_option.out
index 0284392..48d4708 100644
--- a/ops/op2/test_cases/sync/v8_ref_option.out
+++ b/ops/op2/test_cases/sync/v8_ref_option.out
@@ -42,7 +42,7 @@ impl op_v8_lifetime {
                 deno_core::v8::String,
             >(arg0) else {
                 fast_api_callback_options.fallback = true;
-                return ::std::default::Default::default();
+                return unsafe { std::mem::zeroed() };
             };
             let arg0 = match &arg0 {
                 None => None,
@@ -52,7 +52,7 @@ impl op_v8_lifetime {
                 deno_core::v8::String,
             >(arg1) else {
                 fast_api_callback_options.fallback = true;
-                return ::std::default::Default::default();
+                return unsafe { std::mem::zeroed() };
             };
             let arg1 = match &arg1 {
                 None => None,