feat: Expose set_accessor_with_configuration to include data and property attribute (#1220)

src/binding.cc

@@ -1126,17 +1126,15 @@ void v8__ObjectTemplate__SetInternalFieldCount(const v8::ObjectTemplate& self,
   ptr_to_local(&self)->SetInternalFieldCount(value);
 }
 
-void v8__ObjectTemplate__SetAccessor(const v8::ObjectTemplate& self,
+void v8__ObjectTemplate__SetAccessor(
-                                     const v8::Name& key,
-                                     v8::AccessorNameGetterCallback getter) {
-  ptr_to_local(&self)->SetAccessor(ptr_to_local(&key), getter);
-}
-
-void v8__ObjectTemplate__SetAccessorWithSetter(
     const v8::ObjectTemplate& self, const v8::Name& key,
     v8::AccessorNameGetterCallback getter,
-    v8::AccessorNameSetterCallback setter) {
+    v8::AccessorNameSetterCallback setter,
-  ptr_to_local(&self)->SetAccessor(ptr_to_local(&key), getter, setter);
+    const v8::Value* data_or_null, 
+    v8::PropertyAttribute attr) {
+  ptr_to_local(&self)->SetAccessor(
+    ptr_to_local(&key), getter, setter,  ptr_to_local(data_or_null), v8::AccessControl::DEFAULT,
+    attr);
 }
 
 void v8__ObjectTemplate__SetNamedPropertyHandler(
@@ -1273,17 +1271,13 @@ MaybeBool v8__Object__DefineProperty(const v8::Object& self,
 MaybeBool v8__Object__SetAccessor(const v8::Object& self,
                                   const v8::Context& context,
                                   const v8::Name& key,
-                                  v8::AccessorNameGetterCallback getter) {
+                                  v8::AccessorNameGetterCallback getter, 
+                                  v8::AccessorNameSetterCallback setter,
+                                  const v8::Value* data_or_null, 
+                                  v8::PropertyAttribute attr) {
   return maybe_to_maybe_bool(ptr_to_local(&self)->SetAccessor(
-      ptr_to_local(&context), ptr_to_local(&key), getter));
+      ptr_to_local(&context), ptr_to_local(&key), getter, setter,
-}
+      ptr_to_local(data_or_null), v8::AccessControl::DEFAULT,attr));
-
-MaybeBool v8__Object__SetAccessorWithSetter(
-    const v8::Object& self, const v8::Context& context, const v8::Name& key,
-    v8::AccessorNameGetterCallback getter,
-    v8::AccessorNameSetterCallback setter) {
-  return maybe_to_maybe_bool(ptr_to_local(&self)->SetAccessor(
-      ptr_to_local(&context), ptr_to_local(&key), getter, setter));
 }
 
 v8::Isolate* v8__Object__GetIsolate(const v8::Object& self) {

src/object.rs

@@ -2,6 +2,7 @@ use crate::isolate::Isolate;
 use crate::support::int;
 use crate::support::MapFnTo;
 use crate::support::MaybeBool;
+use crate::AccessorConfiguration;
 use crate::AccessorNameGetterCallback;
 use crate::AccessorNameSetterCallback;
 use crate::Array;
@@ -25,6 +26,7 @@ use crate::Value;
 use std::convert::TryFrom;
 use std::ffi::c_void;
 use std::num::NonZeroI32;
+use std::ptr::null;
 
 extern "C" {
   fn v8__Object__New(isolate: *mut Isolate) -> *const Object;
@@ -40,13 +42,9 @@ extern "C" {
     context: *const Context,
     key: *const Name,
     getter: AccessorNameGetterCallback,
-  ) -> MaybeBool;
+    setter: Option<AccessorNameSetterCallback>,
-  fn v8__Object__SetAccessorWithSetter(
+    data_or_null: *const Value,
-    this: *const Object,
+    attr: PropertyAttribute,
-    context: *const Context,
-    key: *const Name,
-    getter: AccessorNameGetterCallback,
-    setter: AccessorNameSetterCallback,
   ) -> MaybeBool;
   fn v8__Object__Get(
     this: *const Object,
@@ -435,15 +433,11 @@ impl Object {
     name: Local<Name>,
     getter: impl for<'s> MapFnTo<AccessorNameGetterCallback<'s>>,
   ) -> Option<bool> {
-    unsafe {
+    self.set_accessor_with_configuration(
-      v8__Object__SetAccessor(
+      scope,
-        self,
+      name,
-        &*scope.get_current_context(),
+      AccessorConfiguration::new(getter),
-        &*name,
+    )
-        getter.map_fn_to(),
-      )
-    }
-    .into()
   }
 
   #[inline(always)]
@@ -453,14 +447,29 @@ impl Object {
     name: Local<Name>,
     getter: impl for<'s> MapFnTo<AccessorNameGetterCallback<'s>>,
     setter: impl for<'s> MapFnTo<AccessorNameSetterCallback<'s>>,
+  ) -> Option<bool> {
+    self.set_accessor_with_configuration(
+      scope,
+      name,
+      AccessorConfiguration::new(getter).setter(setter),
+    )
+  }
+  #[inline(always)]
+  pub fn set_accessor_with_configuration(
+    &self,
+    scope: &mut HandleScope,
+    name: Local<Name>,
+    configuration: AccessorConfiguration,
   ) -> Option<bool> {
     unsafe {
-      v8__Object__SetAccessorWithSetter(
+      v8__Object__SetAccessor(
         self,
         &*scope.get_current_context(),
         &*name,
-        getter.map_fn_to(),
+        configuration.getter,
-        setter.map_fn_to(),
+        configuration.setter,
+        configuration.data.map_or_else(null, |p| &*p),
+        configuration.property_attribute,
       )
     }
     .into()

src/template.rs

@@ -91,16 +91,14 @@ extern "C" {
     this: *const ObjectTemplate,
     value: int,
   );
+
   fn v8__ObjectTemplate__SetAccessor(
     this: *const ObjectTemplate,
     key: *const Name,
     getter: AccessorNameGetterCallback,
-  );
+    setter: Option<AccessorNameSetterCallback>,
-  fn v8__ObjectTemplate__SetAccessorWithSetter(
+    data_or_null: *const Value,
-    this: *const ObjectTemplate,
+    attr: PropertyAttribute,
-    key: *const Name,
-    getter: AccessorNameGetterCallback,
-    setter: AccessorNameSetterCallback,
   );
   fn v8__ObjectTemplate__SetAccessorProperty(
     this: *const ObjectTemplate,
@@ -135,6 +133,46 @@ extern "C" {
   fn v8__ObjectTemplate__SetImmutableProto(this: *const ObjectTemplate);
 }
 
+pub struct AccessorConfiguration<'s> {
+  pub(crate) getter: AccessorNameGetterCallback<'s>,
+  pub(crate) setter: Option<AccessorNameSetterCallback<'s>>,
+  pub(crate) data: Option<Local<'s, Value>>,
+  pub(crate) property_attribute: PropertyAttribute,
+}
+
+impl<'s> AccessorConfiguration<'s> {
+  pub fn new(getter: impl MapFnTo<AccessorNameGetterCallback<'s>>) -> Self {
+    Self {
+      getter: getter.map_fn_to(),
+      setter: None,
+      data: None,
+      property_attribute: NONE,
+    }
+  }
+
+  pub fn setter(
+    mut self,
+    setter: impl MapFnTo<AccessorNameSetterCallback<'s>>,
+  ) -> Self {
+    self.setter = Some(setter.map_fn_to());
+    self
+  }
+
+  pub fn property_attribute(
+    mut self,
+    property_attribute: PropertyAttribute,
+  ) -> Self {
+    self.property_attribute = property_attribute;
+    self
+  }
+
+  /// Set the associated data. The default is no associated data.
+  pub fn data(mut self, data: Local<'s, Value>) -> Self {
+    self.data = Some(data);
+    self
+  }
+}
+
 #[derive(Default)]
 pub struct NamedPropertyHandlerConfiguration<'s> {
   pub(crate) getter: Option<AccessorNameGetterCallback<'s>>,
@@ -620,7 +658,8 @@ impl ObjectTemplate {
     key: Local<Name>,
     getter: impl for<'s> MapFnTo<AccessorNameGetterCallback<'s>>,
   ) {
-    unsafe { v8__ObjectTemplate__SetAccessor(self, &*key, getter.map_fn_to()) }
+    self
+      .set_accessor_with_configuration(key, AccessorConfiguration::new(getter))
   }
 
   #[inline(always)]
@@ -629,13 +668,27 @@ impl ObjectTemplate {
     key: Local<Name>,
     getter: impl for<'s> MapFnTo<AccessorNameGetterCallback<'s>>,
     setter: impl for<'s> MapFnTo<AccessorNameSetterCallback<'s>>,
+  ) {
+    self.set_accessor_with_configuration(
+      key,
+      AccessorConfiguration::new(getter).setter(setter),
+    );
+  }
+
+  #[inline(always)]
+  pub fn set_accessor_with_configuration(
+    &self,
+    key: Local<Name>,
+    configuration: AccessorConfiguration,
   ) {
     unsafe {
-      v8__ObjectTemplate__SetAccessorWithSetter(
+      v8__ObjectTemplate__SetAccessor(
         self,
         &*key,
-        getter.map_fn_to(),
+        configuration.getter,
-        setter.map_fn_to(),
+        configuration.setter,
+        configuration.data.map_or_else(null, |p| &*p),
+        configuration.property_attribute,
       )
     }
   }

tests/test_api.rs

@@ -14,10 +14,10 @@ use std::ptr::{addr_of, NonNull};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::Arc;
 use std::sync::Mutex;
-use v8::fast_api;
 use v8::fast_api::CType;
 use v8::fast_api::Type::*;
 use v8::inspector::ChannelBase;
+use v8::{fast_api, AccessorConfiguration};
 
 // TODO(piscisaureus): Ideally there would be no need to import this trait.
 use v8::MapFnTo;
@@ -1801,6 +1801,43 @@ fn object_template_set_accessor() {
       assert!(this.set_internal_field(0, value));
     };
 
+    let getter_with_data =
+      |scope: &mut v8::HandleScope,
+       key: v8::Local<v8::Name>,
+       args: v8::PropertyCallbackArguments,
+       mut rv: v8::ReturnValue| {
+        let this = args.this();
+
+        assert_eq!(args.holder(), this);
+        assert!(args.data().is_string());
+        assert!(!args.should_throw_on_error());
+        assert_eq!(args.data().to_rust_string_lossy(scope), "data");
+
+        let expected_key = v8::String::new(scope, "key").unwrap();
+        assert!(key.strict_equals(expected_key.into()));
+
+        rv.set(this.get_internal_field(scope, 0).unwrap());
+      };
+
+    let setter_with_data =
+      |scope: &mut v8::HandleScope,
+       key: v8::Local<v8::Name>,
+       value: v8::Local<v8::Value>,
+       args: v8::PropertyCallbackArguments| {
+        let this = args.this();
+
+        assert_eq!(args.holder(), this);
+        assert!(args.data().is_string());
+        assert!(!args.should_throw_on_error());
+        assert_eq!(args.data().to_rust_string_lossy(scope), "data");
+
+        let expected_key = v8::String::new(scope, "key").unwrap();
+        assert!(key.strict_equals(expected_key.into()));
+
+        assert!(value.is_int32());
+        assert!(this.set_internal_field(0, value));
+      };
+
     let key = v8::String::new(scope, "key").unwrap();
     let name = v8::String::new(scope, "obj").unwrap();
 
@@ -1840,6 +1877,34 @@ fn object_template_set_accessor() {
     // Falls back on standard setter
     assert!(eval(scope, "obj.key2 = null; obj.key2").unwrap().is_null());
 
+    // Getter + setter + data
+
+    let templ = v8::ObjectTemplate::new(scope);
+    templ.set_internal_field_count(1);
+    let data = v8::String::new(scope, "data").unwrap();
+    templ.set_accessor_with_configuration(
+      key.into(),
+      AccessorConfiguration::new(getter_with_data)
+        .setter(setter_with_data)
+        .data(data.into()),
+    );
+
+    let obj = templ.new_instance(scope).unwrap();
+    obj.set_internal_field(0, int.into());
+    scope.get_current_context().global(scope).set(
+      scope,
+      name.into(),
+      obj.into(),
+    );
+    let new_int = v8::Integer::new(scope, 9);
+    eval(scope, "obj.key = 9");
+    assert!(obj
+      .get_internal_field(scope, 0)
+      .unwrap()
+      .strict_equals(new_int.into()));
+    // Falls back on standard setter
+    assert!(eval(scope, "obj.key2 = null; obj.key2").unwrap().is_null());
+
     // Accessor property
     let getter = v8::FunctionTemplate::new(scope, fortytwo_callback);
     fn property_setter(
@@ -2621,6 +2686,218 @@ fn object_set_accessor_with_setter() {
   }
 }
 
+#[test]
+fn object_set_accessor_with_setter_with_property() {
+  let _setup_guard = setup::parallel_test();
+  let isolate = &mut v8::Isolate::new(Default::default());
+  let scope = &mut v8::HandleScope::new(isolate);
+  let context = v8::Context::new(scope);
+  let scope = &mut v8::ContextScope::new(scope, context);
+
+  {
+    static CALL_COUNT: AtomicUsize = AtomicUsize::new(0);
+
+    let getter = |scope: &mut v8::HandleScope,
+                  key: v8::Local<v8::Name>,
+                  args: v8::PropertyCallbackArguments,
+                  mut rv: v8::ReturnValue| {
+      let this = args.this();
+
+      assert_eq!(args.holder(), this);
+      assert!(args.data().is_undefined());
+      assert!(!args.should_throw_on_error());
+
+      let expected_key = v8::String::new(scope, "getter_setter_key").unwrap();
+      assert!(key.strict_equals(expected_key.into()));
+
+      let int_key = v8::String::new(scope, "int_key").unwrap();
+      let int_value = this.get(scope, int_key.into()).unwrap();
+      let int_value = v8::Local::<v8::Integer>::try_from(int_value).unwrap();
+      assert_eq!(int_value.value(), 42);
+
+      let s = v8::String::new(scope, "hello").unwrap();
+      assert!(rv.get(scope).is_undefined());
+      rv.set(s.into());
+
+      CALL_COUNT.fetch_add(1, Ordering::SeqCst);
+    };
+
+    let setter = |scope: &mut v8::HandleScope,
+                  key: v8::Local<v8::Name>,
+                  value: v8::Local<v8::Value>,
+                  args: v8::PropertyCallbackArguments| {
+      println!("setter called");
+
+      let this = args.this();
+
+      assert_eq!(args.holder(), this);
+      assert!(args.data().is_undefined());
+      assert!(!args.should_throw_on_error());
+
+      let expected_key = v8::String::new(scope, "getter_setter_key").unwrap();
+      assert!(key.strict_equals(expected_key.into()));
+
+      let int_key = v8::String::new(scope, "int_key").unwrap();
+      let int_value = this.get(scope, int_key.into()).unwrap();
+      let int_value = v8::Local::<v8::Integer>::try_from(int_value).unwrap();
+      assert_eq!(int_value.value(), 42);
+
+      let new_value = v8::Local::<v8::Integer>::try_from(value).unwrap();
+      this.set(scope, int_key.into(), new_value.into());
+
+      CALL_COUNT.fetch_add(1, Ordering::SeqCst);
+    };
+
+    let obj = v8::Object::new(scope);
+
+    let getter_setter_key =
+      v8::String::new(scope, "getter_setter_key").unwrap();
+    obj.set_accessor_with_configuration(
+      scope,
+      getter_setter_key.into(),
+      AccessorConfiguration::new(getter)
+        .setter(setter)
+        .property_attribute(v8::READ_ONLY),
+    );
+
+    let int_key = v8::String::new(scope, "int_key").unwrap();
+    let int_value = v8::Integer::new(scope, 42);
+    obj.set(scope, int_key.into(), int_value.into());
+
+    let obj_name = v8::String::new(scope, "obj").unwrap();
+    context
+      .global(scope)
+      .set(scope, obj_name.into(), obj.into());
+
+    let actual = eval(scope, "obj.getter_setter_key").unwrap();
+    let expected = v8::String::new(scope, "hello").unwrap();
+    assert!(actual.strict_equals(expected.into()));
+
+    eval(scope, "obj.getter_setter_key = 123").unwrap();
+    assert_eq!(
+      obj
+        .get(scope, int_key.into())
+        .unwrap()
+        .to_integer(scope)
+        .unwrap()
+        .value(),
+      42 //Since it is read only
+    );
+
+    assert_eq!(CALL_COUNT.load(Ordering::SeqCst), 1);
+  }
+}
+
+#[test]
+fn object_set_accessor_with_data() {
+  let _setup_guard = setup::parallel_test();
+  let isolate = &mut v8::Isolate::new(Default::default());
+  let scope = &mut v8::HandleScope::new(isolate);
+  let context = v8::Context::new(scope);
+  let scope = &mut v8::ContextScope::new(scope, context);
+
+  {
+    static CALL_COUNT: AtomicUsize = AtomicUsize::new(0);
+
+    let getter = |scope: &mut v8::HandleScope,
+                  key: v8::Local<v8::Name>,
+                  args: v8::PropertyCallbackArguments,
+                  mut rv: v8::ReturnValue| {
+      let this = args.this();
+
+      assert_eq!(args.holder(), this);
+      assert!(args.data().is_string());
+      assert!(!args.should_throw_on_error());
+
+      let data = v8::String::new(scope, "data").unwrap();
+      assert!(data.strict_equals(args.data()));
+
+      let expected_key = v8::String::new(scope, "getter_setter_key").unwrap();
+      assert!(key.strict_equals(expected_key.into()));
+
+      let int_key = v8::String::new(scope, "int_key").unwrap();
+      let int_value = this.get(scope, int_key.into()).unwrap();
+      let int_value = v8::Local::<v8::Integer>::try_from(int_value).unwrap();
+      assert_eq!(int_value.value(), 42);
+
+      let s = v8::String::new(scope, "hello").unwrap();
+      assert!(rv.get(scope).is_undefined());
+      rv.set(s.into());
+
+      CALL_COUNT.fetch_add(1, Ordering::SeqCst);
+    };
+
+    let setter = |scope: &mut v8::HandleScope,
+                  key: v8::Local<v8::Name>,
+                  value: v8::Local<v8::Value>,
+                  args: v8::PropertyCallbackArguments| {
+      println!("setter called");
+
+      let this = args.this();
+
+      assert_eq!(args.holder(), this);
+      assert!(args.data().is_string());
+      assert!(!args.should_throw_on_error());
+
+      let data = v8::String::new(scope, "data").unwrap();
+      assert!(data.strict_equals(args.data()));
+
+      let expected_key = v8::String::new(scope, "getter_setter_key").unwrap();
+      assert!(key.strict_equals(expected_key.into()));
+
+      let int_key = v8::String::new(scope, "int_key").unwrap();
+      let int_value = this.get(scope, int_key.into()).unwrap();
+      let int_value = v8::Local::<v8::Integer>::try_from(int_value).unwrap();
+      assert_eq!(int_value.value(), 42);
+
+      let new_value = v8::Local::<v8::Integer>::try_from(value).unwrap();
+      this.set(scope, int_key.into(), new_value.into());
+
+      CALL_COUNT.fetch_add(1, Ordering::SeqCst);
+    };
+
+    let obj = v8::Object::new(scope);
+
+    let getter_setter_key =
+      v8::String::new(scope, "getter_setter_key").unwrap();
+
+    let data = v8::String::new(scope, "data").unwrap();
+    obj.set_accessor_with_configuration(
+      scope,
+      getter_setter_key.into(),
+      AccessorConfiguration::new(getter)
+        .setter(setter)
+        .data(data.into()),
+    );
+
+    let int_key = v8::String::new(scope, "int_key").unwrap();
+    let int_value = v8::Integer::new(scope, 42);
+    obj.set(scope, int_key.into(), int_value.into());
+
+    let obj_name = v8::String::new(scope, "obj").unwrap();
+    context
+      .global(scope)
+      .set(scope, obj_name.into(), obj.into());
+
+    let actual = eval(scope, "obj.getter_setter_key").unwrap();
+    let expected = v8::String::new(scope, "hello").unwrap();
+    assert!(actual.strict_equals(expected.into()));
+
+    eval(scope, "obj.getter_setter_key = 123").unwrap();
+    assert_eq!(
+      obj
+        .get(scope, int_key.into())
+        .unwrap()
+        .to_integer(scope)
+        .unwrap()
+        .value(),
+      123
+    );
+
+    assert_eq!(CALL_COUNT.load(Ordering::SeqCst), 2);
+  }
+}
+
 #[test]
 fn promise_resolved() {
   let _setup_guard = setup::parallel_test();