Improve Object::get_property_names() and Object::get_own_property_names() (#1049)

This change allows the customization of the behavior of
v8::Object::GetOwnPropertyNames() and v8::Object::GetPropertyNames() by
accepting all the options that the raw V8 API supports.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

examples/process.rs

@@ -358,7 +358,9 @@ where
       .to_object(scope)
       .unwrap();
 
-    let props = output.get_property_names(scope).unwrap();
+    let props = output
+      .get_property_names(scope, v8::GetPropertyNamesArgsBuilder::new().build())
+      .unwrap();
     for i in 0..props.length() {
       let key = props.get_index(scope, i).unwrap();
       let value = output.get(scope, key).unwrap();

src/binding.cc

@@ -1249,16 +1249,48 @@ const v8::Context* v8__Object__GetCreationContext(const v8::Object& self) {
   return maybe_local_to_ptr(ptr_to_local(&self)->GetCreationContext());
 }
 
-const v8::Array* v8__Object__GetOwnPropertyNames(const v8::Object* self,
+// v8::PropertyFilter
-                                                 const v8::Context* context) {
+static_assert(v8::ALL_PROPERTIES == 0, "v8::ALL_PROPERTIES is not 0");
-  return maybe_local_to_ptr(
+static_assert(v8::ONLY_WRITABLE == 1, "v8::ONLY_WRITABLE is not 1");
-      ptr_to_local(self)->GetOwnPropertyNames(ptr_to_local(context)));
+static_assert(v8::ONLY_ENUMERABLE == 2, "v8::ONLY_ENUMERABLE is not 2");
+static_assert(v8::ONLY_CONFIGURABLE == 4, "v8::ONLY_CONFIGURABLE is not 4");
+static_assert(v8::SKIP_STRINGS == 8, "v8::SKIP_STRINGS is not 8");
+static_assert(v8::SKIP_SYMBOLS == 16, "v8::SKIP_SYMBOLS is not 16");
+
+// v8::KeyConversionMode
+static_assert(static_cast<int>(v8::KeyConversionMode::kConvertToString) == 0,
+              "v8::KeyConversionMode::kConvertToString is not 0");
+static_assert(static_cast<int>(v8::KeyConversionMode::kKeepNumbers) == 1,
+              "v8::KeyConversionMode::kKeepNumbers is not 1");
+static_assert(static_cast<int>(v8::KeyConversionMode::kNoNumbers) == 2,
+              "v8::KeyConversionMode::kNoNumbers is not 2");
+
+// v8::KeyCollectionMode
+static_assert(static_cast<int>(v8::KeyCollectionMode::kOwnOnly) == 0,
+              "v8::KeyCollectionMode::kOwnOnly is not 0");
+static_assert(static_cast<int>(v8::KeyCollectionMode::kIncludePrototypes) == 1,
+              "v8::KeyCollectionMode::kIncludePrototypes is not 1");
+
+// v8::IndexFilter
+static_assert(static_cast<int>(v8::IndexFilter::kIncludeIndices) == 0,
+              "v8::IndexFilter::kIncludeIndices is not 0");
+static_assert(static_cast<int>(v8::IndexFilter::kSkipIndices) == 1,
+              "v8::IndexFilter::kSkipIndices is not 1");
+
+const v8::Array* v8__Object__GetOwnPropertyNames(
+    const v8::Object* self, const v8::Context* context,
+    v8::PropertyFilter filter, v8::KeyConversionMode key_conversion) {
+  return maybe_local_to_ptr(ptr_to_local(self)->GetOwnPropertyNames(
+      ptr_to_local(context), filter, key_conversion));
 }
 
-const v8::Array* v8__Object__GetPropertyNames(const v8::Object* self,
+const v8::Array* v8__Object__GetPropertyNames(
-                                              const v8::Context* context) {
+    const v8::Object* self, const v8::Context* context,
-  return maybe_local_to_ptr(
+    v8::KeyCollectionMode mode, v8::PropertyFilter property_filter,
-      ptr_to_local(self)->GetPropertyNames(ptr_to_local(context)));
+    v8::IndexFilter index_filter, v8::KeyConversionMode key_conversion) {
+  return maybe_local_to_ptr(ptr_to_local(self)->GetPropertyNames(
+      ptr_to_local(context), mode, property_filter, index_filter,
+      key_conversion));
 }
 
 MaybeBool v8__Object__Has(const v8::Object& self, const v8::Context& context,

src/get_property_names_args_builder.rs

@@ -0,0 +1,120 @@
+use crate::PropertyFilter;
+use crate::ONLY_ENUMERABLE;
+use crate::SKIP_SYMBOLS;
+
+#[derive(Debug, Clone, Copy)]
+#[repr(C)]
+pub enum KeyConversionMode {
+  /// kConvertToString will convert integer indices to strings.
+  ConvertToString,
+  /// kKeepNumbers will return numbers for integer indices.
+  KeepNumbers,
+  NoNumbers,
+}
+
+/// Keys/Properties filter enums:
+///
+/// KeyCollectionMode limits the range of collected properties. kOwnOnly limits
+/// the collected properties to the given Object only. kIncludesPrototypes will
+/// include all keys of the objects's prototype chain as well.
+#[derive(Debug, Clone, Copy)]
+#[repr(C)]
+pub enum KeyCollectionMode {
+  /// OwnOnly limits the collected properties to the given Object only.
+  OwnOnly,
+  /// kIncludesPrototypes will include all keys of the objects's prototype chain
+  /// as well.
+  IncludePrototypes,
+}
+
+#[derive(Debug, Clone, Copy)]
+#[repr(C)]
+pub enum IndexFilter {
+  /// kIncludesIndices allows for integer indices to be collected.
+  IncludeIndices,
+  /// kSkipIndices will exclude integer indices from being collected.
+  SkipIndices,
+}
+
+pub struct GetPropertyNamesArgs {
+  pub mode: KeyCollectionMode,
+  pub property_filter: PropertyFilter,
+  pub index_filter: IndexFilter,
+  pub key_conversion: KeyConversionMode,
+}
+
+impl Default for GetPropertyNamesArgs {
+  fn default() -> Self {
+    GetPropertyNamesArgs {
+      mode: KeyCollectionMode::IncludePrototypes,
+      property_filter: ONLY_ENUMERABLE | SKIP_SYMBOLS,
+      index_filter: IndexFilter::IncludeIndices,
+      key_conversion: KeyConversionMode::KeepNumbers,
+    }
+  }
+}
+
+pub struct GetPropertyNamesArgsBuilder {
+  mode: KeyCollectionMode,
+  property_filter: PropertyFilter,
+  index_filter: IndexFilter,
+  key_conversion: KeyConversionMode,
+}
+
+impl Default for GetPropertyNamesArgsBuilder {
+  fn default() -> Self {
+    Self::new()
+  }
+}
+
+impl GetPropertyNamesArgsBuilder {
+  pub fn new() -> Self {
+    Self {
+      mode: KeyCollectionMode::IncludePrototypes,
+      property_filter: ONLY_ENUMERABLE | SKIP_SYMBOLS,
+      index_filter: IndexFilter::IncludeIndices,
+      key_conversion: KeyConversionMode::KeepNumbers,
+    }
+  }
+
+  pub fn build(&self) -> GetPropertyNamesArgs {
+    GetPropertyNamesArgs {
+      mode: self.mode,
+      property_filter: self.property_filter,
+      index_filter: self.index_filter,
+      key_conversion: self.key_conversion,
+    }
+  }
+
+  pub fn mode(
+    &mut self,
+    mode: KeyCollectionMode,
+  ) -> &mut GetPropertyNamesArgsBuilder {
+    self.mode = mode;
+    self
+  }
+
+  pub fn property_filter(
+    &mut self,
+    property_filter: PropertyFilter,
+  ) -> &mut GetPropertyNamesArgsBuilder {
+    self.property_filter = property_filter;
+    self
+  }
+
+  pub fn index_filter(
+    &mut self,
+    index_filter: IndexFilter,
+  ) -> &mut GetPropertyNamesArgsBuilder {
+    self.index_filter = index_filter;
+    self
+  }
+
+  pub fn key_conversion(
+    &mut self,
+    key_conversion: KeyConversionMode,
+  ) -> &mut Self {
+    self.key_conversion = key_conversion;
+    self
+  }
+}

src/lib.rs

@@ -41,6 +41,7 @@ mod external_references;
 pub mod fast_api;
 mod fixed_array;
 mod function;
+mod get_property_names_args_builder;
 mod handle;
 pub mod icu;
 mod isolate;
@@ -55,6 +56,7 @@ mod primitives;
 mod private;
 mod promise;
 mod property_attribute;
+mod property_filter;
 mod proxy;
 mod scope;
 mod script;
@@ -88,6 +90,7 @@ pub use exception::*;
 pub use external_references::ExternalReference;
 pub use external_references::ExternalReferences;
 pub use function::*;
+pub use get_property_names_args_builder::*;
 pub use handle::Global;
 pub use handle::Handle;
 pub use handle::Local;
@@ -118,6 +121,7 @@ pub use primitives::*;
 pub use private::*;
 pub use promise::{PromiseRejectEvent, PromiseRejectMessage, PromiseState};
 pub use property_attribute::*;
+pub use property_filter::*;
 pub use proxy::*;
 pub use scope::CallbackScope;
 pub use scope::ContextScope;

src/object.rs

@@ -8,13 +8,18 @@ use crate::AccessorNameGetterCallback;
 use crate::AccessorNameSetterCallback;
 use crate::Array;
 use crate::Context;
+use crate::GetPropertyNamesArgs;
 use crate::HandleScope;
+use crate::IndexFilter;
+use crate::KeyCollectionMode;
+use crate::KeyConversionMode;
 use crate::Local;
 use crate::Map;
 use crate::Name;
 use crate::Object;
 use crate::Private;
 use crate::PropertyAttribute;
+use crate::PropertyFilter;
 use crate::Value;
 use std::convert::TryFrom;
 use std::num::NonZeroI32;
@@ -87,10 +92,16 @@ extern "C" {
   fn v8__Object__GetOwnPropertyNames(
     this: *const Object,
     context: *const Context,
+    filter: PropertyFilter,
+    key_conversion: KeyConversionMode,
   ) -> *const Array;
   fn v8__Object__GetPropertyNames(
     this: *const Object,
     context: *const Context,
+    mode: KeyCollectionMode,
+    property_filter: PropertyFilter,
+    index_filter: IndexFilter,
+    key_conversion: KeyConversionMode,
   ) -> *const Array;
   fn v8__Object__Has(
     this: *const Object,
@@ -414,10 +425,16 @@ impl Object {
   pub fn get_own_property_names<'s>(
     &self,
     scope: &mut HandleScope<'s>,
+    args: GetPropertyNamesArgs,
   ) -> Option<Local<'s, Array>> {
     unsafe {
       scope.cast_local(|sd| {
-        v8__Object__GetOwnPropertyNames(self, sd.get_current_context())
+        v8__Object__GetOwnPropertyNames(
+          self,
+          sd.get_current_context(),
+          args.property_filter,
+          args.key_conversion,
+        )
       })
     }
   }
@@ -429,10 +446,18 @@ impl Object {
   pub fn get_property_names<'s>(
     &self,
     scope: &mut HandleScope<'s>,
+    args: GetPropertyNamesArgs,
   ) -> Option<Local<'s, Array>> {
     unsafe {
       scope.cast_local(|sd| {
-        v8__Object__GetPropertyNames(self, sd.get_current_context())
+        v8__Object__GetPropertyNames(
+          self,
+          sd.get_current_context(),
+          args.mode,
+          args.property_filter,
+          args.index_filter,
+          args.key_conversion,
+        )
       })
     }
   }

src/property_filter.rs

@@ -0,0 +1,125 @@
+#[repr(C)]
+#[derive(Debug, Eq, PartialEq, Clone, Copy)]
+pub struct PropertyFilter(u32);
+
+pub const ALL_PROPERTIES: PropertyFilter = PropertyFilter(0);
+
+pub const ONLY_WRITABLE: PropertyFilter = PropertyFilter(1);
+
+pub const ONLY_ENUMERABLE: PropertyFilter = PropertyFilter(2);
+
+pub const ONLY_CONFIGURABLE: PropertyFilter = PropertyFilter(4);
+
+pub const SKIP_STRINGS: PropertyFilter = PropertyFilter(8);
+
+pub const SKIP_SYMBOLS: PropertyFilter = PropertyFilter(16);
+
+impl PropertyFilter {
+  /// Test if all property filters are set.
+  pub fn is_all_properties(&self) -> bool {
+    *self == ALL_PROPERTIES
+  }
+
+  /// Test if the only-writable property filter is set.
+  pub fn is_only_writable(&self) -> bool {
+    self.has(ONLY_WRITABLE)
+  }
+
+  /// Test if the only-enumerable property filter is set.
+  pub fn is_only_enumerable(&self) -> bool {
+    self.has(ONLY_ENUMERABLE)
+  }
+
+  /// Test if the only-configurable property filter is set.
+  pub fn is_only_configurable(&self) -> bool {
+    self.has(ONLY_CONFIGURABLE)
+  }
+
+  /// Test if the skip-strings property filter is set.
+  pub fn is_skip_strings(&self) -> bool {
+    self.has(SKIP_STRINGS)
+  }
+
+  /// Test if the skip-symbols property filter is set.
+  pub fn is_skip_symbols(&self) -> bool {
+    self.has(SKIP_SYMBOLS)
+  }
+
+  fn has(&self, that: Self) -> bool {
+    let Self(lhs) = self;
+    let Self(rhs) = that;
+    0 != lhs & rhs
+  }
+}
+
+// Identical to #[derive(Default)] but arguably clearer when made explicit.
+impl Default for PropertyFilter {
+  fn default() -> Self {
+    ALL_PROPERTIES
+  }
+}
+
+impl std::ops::BitOr for PropertyFilter {
+  type Output = Self;
+
+  fn bitor(self, Self(rhs): Self) -> Self {
+    let Self(lhs) = self;
+    Self(lhs | rhs)
+  }
+}
+
+#[test]
+fn test_attr() {
+  assert!(ALL_PROPERTIES.is_all_properties());
+  assert!(!ALL_PROPERTIES.is_only_writable());
+  assert!(!ALL_PROPERTIES.is_only_enumerable());
+  assert!(!ALL_PROPERTIES.is_only_configurable());
+  assert!(!ALL_PROPERTIES.is_skip_strings());
+  assert!(!ALL_PROPERTIES.is_skip_symbols());
+
+  assert!(!ONLY_WRITABLE.is_all_properties());
+  assert!(ONLY_WRITABLE.is_only_writable());
+  assert!(!ONLY_WRITABLE.is_only_enumerable());
+  assert!(!ONLY_WRITABLE.is_only_configurable());
+  assert!(!ONLY_WRITABLE.is_skip_strings());
+  assert!(!ONLY_WRITABLE.is_skip_symbols());
+
+  assert!(!ONLY_ENUMERABLE.is_all_properties());
+  assert!(!ONLY_ENUMERABLE.is_only_writable());
+  assert!(ONLY_ENUMERABLE.is_only_enumerable());
+  assert!(!ONLY_ENUMERABLE.is_only_configurable());
+  assert!(!ONLY_ENUMERABLE.is_skip_strings());
+  assert!(!ONLY_ENUMERABLE.is_skip_symbols());
+
+  assert!(!ONLY_CONFIGURABLE.is_all_properties());
+  assert!(!ONLY_CONFIGURABLE.is_only_writable());
+  assert!(!ONLY_CONFIGURABLE.is_only_enumerable());
+  assert!(ONLY_CONFIGURABLE.is_only_configurable());
+  assert!(!ONLY_CONFIGURABLE.is_skip_strings());
+  assert!(!ONLY_CONFIGURABLE.is_skip_symbols());
+
+  assert!(!SKIP_STRINGS.is_all_properties());
+  assert!(!SKIP_STRINGS.is_only_writable());
+  assert!(!SKIP_STRINGS.is_only_enumerable());
+  assert!(!SKIP_STRINGS.is_only_configurable());
+  assert!(SKIP_STRINGS.is_skip_strings());
+  assert!(!SKIP_STRINGS.is_skip_symbols());
+
+  assert!(!SKIP_SYMBOLS.is_all_properties());
+  assert!(!SKIP_SYMBOLS.is_only_writable());
+  assert!(!SKIP_SYMBOLS.is_only_enumerable());
+  assert!(!SKIP_SYMBOLS.is_only_configurable());
+  assert!(!SKIP_SYMBOLS.is_skip_strings());
+  assert!(SKIP_SYMBOLS.is_skip_symbols());
+
+  assert_eq!(ALL_PROPERTIES, Default::default());
+  assert_eq!(ONLY_WRITABLE, ALL_PROPERTIES | ONLY_WRITABLE);
+
+  let attr = ONLY_WRITABLE | ONLY_WRITABLE | SKIP_STRINGS;
+  assert!(!attr.is_all_properties());
+  assert!(attr.is_only_writable());
+  assert!(!attr.is_only_enumerable());
+  assert!(!attr.is_only_configurable());
+  assert!(attr.is_skip_strings());
+  assert!(!attr.is_skip_symbols());
+}

tests/test_api.rs

@@ -4845,15 +4845,19 @@ fn test_object_get_property_names() {
     proto_obj.set(scope, js_proto_test_str, js_null);
     obj.set_prototype(scope, proto_obj.into());
 
-    let own_props = obj.get_own_property_names(scope).unwrap();
+    let own_props = obj
+      .get_own_property_names(scope, Default::default())
+      .unwrap();
     assert_eq!(own_props.length(), 1);
     assert!(own_props.get_index(scope, 0).unwrap() == js_test_str);
 
-    let proto_props = proto_obj.get_own_property_names(scope).unwrap();
+    let proto_props = proto_obj
+      .get_own_property_names(scope, Default::default())
+      .unwrap();
     assert_eq!(proto_props.length(), 1);
     assert!(proto_props.get_index(scope, 0).unwrap() == js_proto_test_str);
 
-    let all_props = obj.get_property_names(scope).unwrap();
+    let all_props = obj.get_property_names(scope, Default::default()).unwrap();
     js_sort_fn.call(scope, all_props.into(), &[]).unwrap();
     assert_eq!(all_props.length(), 2);
     assert!(all_props.get_index(scope, 0).unwrap() == js_proto_test_str);
@@ -4865,10 +4869,128 @@ fn test_object_get_property_names() {
     obj.set(scope, js_test_str, js_null);
     obj.set(scope, js_test_symbol, js_null);
 
-    let own_props = obj.get_own_property_names(scope).unwrap();
+    let own_props = obj
+      .get_own_property_names(scope, Default::default())
+      .unwrap();
     assert_eq!(own_props.length(), 1);
     assert!(own_props.get_index(scope, 0).unwrap() == js_test_str);
   }
+
+  {
+    let obj = v8::Object::new(scope);
+    obj.set(scope, js_test_str, js_null);
+    obj.set(scope, js_test_symbol, js_null);
+
+    let own_props = obj
+      .get_property_names(
+        scope,
+        v8::GetPropertyNamesArgs {
+          mode: v8::KeyCollectionMode::IncludePrototypes,
+          property_filter: v8::ONLY_ENUMERABLE | v8::SKIP_SYMBOLS,
+          index_filter: v8::IndexFilter::IncludeIndices,
+          key_conversion: v8::KeyConversionMode::KeepNumbers,
+        },
+      )
+      .unwrap();
+    assert_eq!(own_props.length(), 1);
+    assert!(own_props.get_index(scope, 0).unwrap() == js_test_str);
+  }
+
+  {
+    let context = v8::Context::new(scope);
+    let scope = &mut v8::ContextScope::new(scope, context);
+
+    let val = eval(scope, "({ 'a': 3, 2: 'b', '7': 'c' })").unwrap();
+    let obj = val.to_object(scope).unwrap();
+
+    {
+      let own_props = obj
+        .get_own_property_names(scope, Default::default())
+        .unwrap();
+
+      assert_eq!(own_props.length(), 3);
+
+      assert!(own_props.get_index(scope, 0).unwrap().is_number());
+      assert_eq!(
+        own_props.get_index(scope, 0).unwrap(),
+        v8::Integer::new(scope, 2)
+      );
+
+      assert!(own_props.get_index(scope, 1).unwrap().is_number());
+      assert_eq!(
+        own_props.get_index(scope, 1).unwrap(),
+        v8::Integer::new(scope, 7)
+      );
+
+      assert!(own_props.get_index(scope, 2).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 2).unwrap(),
+        v8::String::new(scope, "a").unwrap()
+      );
+    }
+
+    {
+      let own_props = obj
+        .get_own_property_names(
+          scope,
+          v8::GetPropertyNamesArgsBuilder::new()
+            .key_conversion(v8::KeyConversionMode::ConvertToString)
+            .build(),
+        )
+        .unwrap();
+
+      assert_eq!(own_props.length(), 3);
+
+      assert!(own_props.get_index(scope, 0).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 0).unwrap(),
+        v8::String::new(scope, "2").unwrap()
+      );
+
+      assert!(own_props.get_index(scope, 1).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 1).unwrap(),
+        v8::String::new(scope, "7").unwrap()
+      );
+
+      assert!(own_props.get_index(scope, 2).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 2).unwrap(),
+        v8::String::new(scope, "a").unwrap()
+      );
+    }
+
+    {
+      let own_props = obj
+        .get_property_names(
+          scope,
+          v8::GetPropertyNamesArgsBuilder::new()
+            .key_conversion(v8::KeyConversionMode::ConvertToString)
+            .build(),
+        )
+        .unwrap();
+
+      assert_eq!(own_props.length(), 3);
+
+      assert!(own_props.get_index(scope, 0).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 0).unwrap(),
+        v8::String::new(scope, "2").unwrap()
+      );
+
+      assert!(own_props.get_index(scope, 1).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 1).unwrap(),
+        v8::String::new(scope, "7").unwrap()
+      );
+
+      assert!(own_props.get_index(scope, 2).unwrap().is_string());
+      assert_eq!(
+        own_props.get_index(scope, 2).unwrap(),
+        v8::String::new(scope, "a").unwrap()
+      );
+    }
+  }
 }
 
 #[test]