Repository: denoland/deno_core
Pull request #29 (2023-07-10 19:18:06)

feat(ops): Implement #[state] for automatic OpState extraction
This provides a useful shortcut for `OpState`-based parameters using a `#[state]` annotation. Parameters annotation w/`#[state]` can be either `&Something`, `&mut Something`, `Option<&Something>`, or `Option<&mut Something>`.

Note that due to the Rust aliasing rules, only one `&mut Something` is allowed. This might be fixable in the future if we modify how the internal state system works. In many cases, a single mutable borrow should be sufficient.


Patch:
diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs
index dcf4457..e316804 100644
--- a/core/runtime/ops.rs
+++ b/core/runtime/ops.rs
@@ -392,9 +392,12 @@ mod tests {
       op_state_rc,
       op_state_ref,
       op_state_mut,
+      op_state_mut_attr,
+      op_state_multi_attr,
     ],
     state = |state| {
-      state.put(1234u32)
+      state.put(1234u32);
+      state.put(10000u16);
     }
   );
 
@@ -886,7 +889,6 @@ mod tests {
   pub fn op_state_rc(state: Rc<RefCell<OpState>>, value: u32) -> u32 {
     let old_value: u32 = state.borrow_mut().take();
     state.borrow_mut().put(value);
-    println!("{value} {old_value}");
     old_value
   }
 
@@ -901,6 +903,23 @@ mod tests {
     *state.borrow_mut() = value;
   }
 
+  #[op2(core, fast)]
+  pub fn op_state_mut_attr(#[state] value: &mut u32, new_value: u32) -> u32 {
+    let old_value = *value;
+    *value = new_value;
+    old_value
+  }
+
+  #[op2(core, fast)]
+  pub fn op_state_multi_attr(
+    #[state] value32: &u32,
+    #[state] value16: &u16,
+    #[state] value8: Option<&u8>,
+  ) -> u32 {
+    assert_eq!(value8, None);
+    *value32 + *value16 as u32
+  }
+
   #[tokio::test]
   pub async fn test_op_state() -> Result<(), Box<dyn std::error::Error>> {
     run_test2(
@@ -908,8 +927,18 @@ mod tests {
       "op_state_rc",
       "if (__index__ == 0) { op_state_rc(__index__) } else { assert(op_state_rc(__index__) == __index__ - 1) }",
     )?;
+    run_test2(
+      10000,
+      "op_state_mut_attr",
+      "if (__index__ == 0) { op_state_mut_attr(__index__) } else { assert(op_state_mut_attr(__index__) == __index__ - 1) }",
+    )?;
     run_test2(10000, "op_state_mut", "op_state_mut(__index__)")?;
     run_test2(10000, "op_state_ref", "assert(op_state_ref() == 1234)")?;
+    run_test2(
+      10000,
+      "op_state_multi_attr",
+      "assert(op_state_multi_attr() == 11234)",
+    )?;
     Ok(())
   }
 }
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index befe60c..c0b82ef 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -14,6 +14,7 @@ use proc_macro2::TokenStream;
 use quote::format_ident;
 use quote::quote;
 use std::iter::zip;
+use syn2::Type;
 
 #[allow(unused)]
 #[derive(Debug, Default, PartialEq, Clone)]
@@ -293,6 +294,42 @@ fn map_v8_fastcall_arg_to_arg(
       *needs_opctx = true;
       quote!(let #arg_ident = #opctx.state.clone();)
     }
+    Arg::State(RefType::Ref, state) => {
+      *needs_opctx = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let #arg_ident = #opctx.state.borrow();
+        let #arg_ident = #arg_ident.borrow::<#state>();
+      }
+    }
+    Arg::State(RefType::Mut, state) => {
+      *needs_opctx = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let mut #arg_ident = #opctx.state.borrow_mut();
+        let #arg_ident = #arg_ident.borrow_mut::<#state>();
+      }
+    }
+    Arg::OptionState(RefType::Ref, state) => {
+      *needs_opctx = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let #arg_ident = #opctx.state.borrow();
+        let #arg_ident = #arg_ident.try_borrow::<#state>();
+      }
+    }
+    Arg::OptionState(RefType::Mut, state) => {
+      *needs_opctx = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let mut #arg_ident = #opctx.state.borrow_mut();
+        let #arg_ident = #arg_ident.try_borrow_mut::<#state>();
+      }
+    }
     Arg::Special(Special::RefStr) => {
       quote! {
         let mut #arg_temp: [::std::mem::MaybeUninit<u8>; 1024] = [::std::mem::MaybeUninit::uninit(); 1024];
@@ -342,9 +379,10 @@ fn map_arg_to_v8_fastcall_type(
 ) -> Result<Option<V8FastCallType>, V8MappingError> {
   let rv = match arg {
     // Virtual OpState arguments
-    Arg::RcRefCell(Special::OpState) | Arg::Ref(_, Special::OpState) => {
-      V8FastCallType::Virtual
-    }
+    Arg::RcRefCell(Special::OpState)
+    | Arg::Ref(_, Special::OpState)
+    | Arg::State(..)
+    | Arg::OptionState(..) => V8FastCallType::Virtual,
     // Other types + ref types are not handled
     Arg::OptionNumeric(_) | Arg::SerdeV8(_) | Arg::Ref(..) => return Ok(None),
     // We don't support v8 global arguments
diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
index 5475e02..e7bf0c4 100644
--- a/ops/op2/dispatch_slow.rs
+++ b/ops/op2/dispatch_slow.rs
@@ -14,6 +14,7 @@ use proc_macro2::Ident;
 use proc_macro2::TokenStream;
 use quote::format_ident;
 use quote::quote;
+use syn2::Type;
 
 pub(crate) fn generate_dispatch_slow(
   config: &MacroConfig,
@@ -284,6 +285,42 @@ pub fn from_arg(
       *needs_opstate = true;
       quote!(let #arg_ident = #opstate.clone();)
     }
+    Arg::State(RefType::Ref, state) => {
+      *needs_opstate = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let #arg_ident = #opstate.borrow();
+        let #arg_ident = #arg_ident.borrow::<#state>();
+      }
+    }
+    Arg::State(RefType::Mut, state) => {
+      *needs_opstate = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let mut #arg_ident = #opstate.borrow_mut();
+        let #arg_ident = #arg_ident.borrow_mut::<#state>();
+      }
+    }
+    Arg::OptionState(RefType::Ref, state) => {
+      *needs_opstate = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let #arg_ident = #opstate.borrow();
+        let #arg_ident = #arg_ident.try_borrow::<#state>();
+      }
+    }
+    Arg::OptionState(RefType::Mut, state) => {
+      *needs_opstate = true;
+      let state =
+        syn2::parse_str::<Type>(state).expect("Failed to reparse state type");
+      quote! {
+        let mut #arg_ident = #opstate.borrow_mut();
+        let #arg_ident = #arg_ident.try_borrow_mut::<#state>();
+      }
+    }
     Arg::V8Local(v8)
     | Arg::OptionV8Local(v8)
     | Arg::V8Ref(_, v8)
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index f5e74cb..00f0762 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -148,6 +148,8 @@ pub enum Arg {
   V8Ref(RefType, V8Arg),
   Numeric(NumericArg),
   SerdeV8(String),
+  State(RefType, String),
+  OptionState(RefType, String),
 }
 
 impl Arg {
@@ -172,6 +174,7 @@ impl Arg {
         | Special::OpState
         | Special::HandleScope,
       ) => true,
+      Self::State(..) | Self::OptionState(..) => true,
       _ => false,
     }
   }
@@ -204,6 +207,7 @@ impl Arg {
         | Arg::OptionV8Local(..)
         | Arg::OptionNumeric(..)
         | Arg::Option(..)
+        | Arg::OptionState(..)
     )
   }
 }
@@ -253,6 +257,8 @@ enum AttributeModifier {
   Smi,
   /// #[string], for strings.
   String,
+  /// #[state], for automatic OpState extraction
+  State,
 }
 
 #[derive(Error, Debug)]
@@ -275,6 +281,8 @@ pub enum SignatureError {
   InvalidGeneric(String),
   #[error("Invalid predicate: '{0}' Only simple where predicates are allowed (eg: T: Trait)")]
   InvalidWherePredicate(String),
+  #[error("State may be either a single OpState parameter, one mutable #[state], or multiple immultiple #[state]s")]
+  InvalidOpStateCombination,
 }
 
 #[derive(Error, Debug)]
@@ -305,6 +313,8 @@ pub enum ArgError {
   InternalError(String),
   #[error("Missing a #[string] attribute")]
   MissingStringAttribute,
+  #[error("Invalid #[state] type '{0}'")]
+  InvalidStateType(String),
 }
 
 #[derive(Copy, Clone, Default)]
@@ -352,6 +362,34 @@ pub fn parse_signature(
     parse_return(parse_attributes(&attributes)?, &signature.output)?;
   let lifetime = parse_lifetime(&signature.generics)?;
   let generic_bounds = parse_generics(&signature.generics)?;
+
+  let mut has_opstate = false;
+  let mut has_mut_state = false;
+  let mut has_ref_state = false;
+
+  for arg in &args {
+    match arg {
+      Arg::RcRefCell(Special::OpState) | Arg::Ref(_, Special::OpState) => {
+        has_opstate = true
+      }
+      Arg::State(RefType::Ref, _) | Arg::OptionState(RefType::Ref, _) => {
+        has_ref_state = true
+      }
+      Arg::State(RefType::Mut, _) | Arg::OptionState(RefType::Mut, _) => {
+        if has_mut_state {
+          return Err(SignatureError::InvalidOpStateCombination);
+        }
+        has_mut_state = true;
+      }
+      _ => {}
+    }
+  }
+
+  // Ensure that either zero or one and only one of these are true
+  if has_opstate as u8 + has_mut_state as u8 + has_ref_state as u8 > 1 {
+    return Err(SignatureError::InvalidOpStateCombination);
+  }
+
   Ok(ParsedSignature {
     args,
     names,
@@ -478,6 +516,7 @@ fn parse_attribute(attr: &Attribute) -> Option<AttributeModifier> {
       (#[serde]) => Some(AttributeModifier::Serde),
       (#[smi]) => Some(AttributeModifier::Smi),
       (#[string]) => Some(AttributeModifier::String),
+      (#[state]) => Some(AttributeModifier::State),
       (#[$_attr:meta]) => None,
     })
   })
@@ -628,6 +667,33 @@ fn parse_type_special(
   }
 }
 
+fn parse_type_state(ty: &Type) -> Result<Arg, ArgError> {
+  let s = match ty {
+    Type::Path(of) => {
+      let inner_type = std::panic::catch_unwind(|| {
+        use syn2 as syn;
+        rules!(of.into_token_stream() => {
+          (Option< $ty:ty >) => ty,
+        })
+      })
+      .map_err(|_| ArgError::InvalidStateType(stringify_token(ty)))?;
+      match parse_type_state(&inner_type)? {
+        Arg::State(reftype, state) => Arg::OptionState(reftype, state),
+        _ => return Err(ArgError::InvalidStateType(stringify_token(ty))),
+      }
+    }
+    Type::Reference(of) => {
+      if of.mutability.is_some() {
+        Arg::State(RefType::Mut, stringify_token(&of.elem))
+      } else {
+        Arg::State(RefType::Ref, stringify_token(&of.elem))
+      }
+    }
+    _ => return Err(ArgError::InvalidStateType(stringify_token(ty))),
+  };
+  Ok(s)
+}
+
 fn parse_type(attrs: Attributes, ty: &Type) -> Result<Arg, ArgError> {
   use ParsedType::*;
   use ParsedTypeContainer::*;
@@ -667,6 +733,9 @@ fn parse_type(attrs: Attributes, ty: &Type) -> Result<Arg, ArgError> {
           return Err(ArgError::InvalidSerdeAttributeType(stringify_token(ty)))
         }
       },
+      AttributeModifier::State => {
+        return parse_type_state(ty);
+      }
       AttributeModifier::String => {
         // We handle this as part of the normal parsing process
       }
@@ -903,6 +972,11 @@ mod tests {
     fn op_state_ref(state: &OpState);
     (Ref(Ref, OpState)) -> Infallible(Void)
   );
+  test!(
+    fn op_state_attr(#[state] something: &Something, #[state] another: Option<&Something>);
+    (State(Ref, "Something"), OptionState(Ref, "Something")) -> Infallible(Void)
+  );
+
   // Args
 
   expect_fail!(


Test patch:
diff --git a/ops/op2/test_cases/sync/op_state_attr.out b/ops/op2/test_cases/sync/op_state_attr.out
new file mode 100644
index 0000000..bff4fc6
--- /dev/null
+++ b/ops/op2/test_cases/sync/op_state_attr.out
@@ -0,0 +1,68 @@
+#[allow(non_camel_case_types)]
+struct op_state_rc {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl deno_core::_ops::Op for op_state_rc {
+    const NAME: &'static str = stringify!(op_state_rc);
+    const DECL: deno_core::_ops::OpDecl = deno_core::_ops::OpDecl::new_internal(
+        stringify!(op_state_rc),
+        false,
+        false,
+        false,
+        2usize 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(
+                &[Type::V8Value, Type::CallbackOptions],
+                CType::Void,
+                Self::v8_fn_ptr_fast as *const ::std::ffi::c_void,
+            )
+        }),
+    );
+}
+impl op_state_rc {
+    pub const fn name() -> &'static str {
+        stringify!(op_state_rc)
+    }
+    #[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,
+    ) -> () {
+        let fast_api_callback_options = unsafe { &mut *fast_api_callback_options };
+        let opctx = unsafe {
+            &*(deno_core::v8::Local::<
+                v8::External,
+            >::cast(unsafe { fast_api_callback_options.data.data })
+                .value() as *const deno_core::_ops::OpCtx)
+        };
+        let arg0 = opctx.state.borrow();
+        let arg0 = arg0.borrow::<Something>();
+        let arg1 = opctx.state.borrow();
+        let arg1 = arg1.try_borrow::<Something>();
+        let result = Self::call(arg0, arg1);
+        result
+    }
+    extern "C" fn v8_fn_ptr(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)
+        };
+        let opstate = &opctx.state;
+        let arg0 = opstate.borrow();
+        let arg0 = arg0.borrow::<Something>();
+        let arg1 = opstate.borrow();
+        let arg1 = arg1.try_borrow::<Something>();
+        let result = Self::call(arg0, arg1);
+    }
+    #[inline(always)]
+    fn call(arg: &Something, arg_opt: Option<&Something>) {}
+}
diff --git a/ops/op2/test_cases/sync/op_state_attr.rs b/ops/op2/test_cases/sync/op_state_attr.rs
new file mode 100644
index 0000000..388527b
--- /dev/null
+++ b/ops/op2/test_cases/sync/op_state_attr.rs
@@ -0,0 +1,4 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+#[op2(fast)]
+fn op_state_rc(#[state] arg: &Something, #[state] arg_opt: Option<&Something>) {}
diff --git a/ops/op2/test_cases/sync/op_state_rc.rs b/ops/op2/test_cases/sync/op_state_rc.rs
index 609e805..e4672ad 100644
--- a/ops/op2/test_cases/sync/op_state_rc.rs
+++ b/ops/op2/test_cases/sync/op_state_rc.rs
@@ -1,4 +1,4 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
 #[op2(fast)]
-fn op_state_rc(state: Rc<RefCell<OpState>>) {}
\ No newline at end of file
+fn op_state_rc(state: Rc<RefCell<OpState>>) {}
diff --git a/ops/op2/test_cases/sync/op_state_ref.rs b/ops/op2/test_cases/sync/op_state_ref.rs
index 1920d52..d0531d4 100644
--- a/ops/op2/test_cases/sync/op_state_ref.rs
+++ b/ops/op2/test_cases/sync/op_state_ref.rs
@@ -1,4 +1,4 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
 #[op2(fast)]
-fn op_state_rc(state: &OpState) {}
\ No newline at end of file
+fn op_state_rc(state: &OpState) {}