From dbf19c8545b2f7fa9a12063820642dcb6560355f Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sat, 3 Sep 2022 21:41:40 +0530 Subject: [PATCH] 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 --- examples/process.rs | 4 +- src/binding.cc | 48 +++++++-- src/get_property_names_args_builder.rs | 120 +++++++++++++++++++++++ src/lib.rs | 4 + src/object.rs | 29 +++++- src/property_filter.rs | 125 ++++++++++++++++++++++++ tests/test_api.rs | 130 ++++++++++++++++++++++++- 7 files changed, 445 insertions(+), 15 deletions(-) create mode 100644 src/get_property_names_args_builder.rs create mode 100644 src/property_filter.rs diff --git a/examples/process.rs b/examples/process.rs index bf84df6e..79f83351 100644 --- a/examples/process.rs +++ b/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(); diff --git a/src/binding.cc b/src/binding.cc index c1d3e40f..d89aae62 100644 --- a/src/binding.cc +++ b/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, - const v8::Context* context) { - return maybe_local_to_ptr( - ptr_to_local(self)->GetOwnPropertyNames(ptr_to_local(context))); +// v8::PropertyFilter +static_assert(v8::ALL_PROPERTIES == 0, "v8::ALL_PROPERTIES is not 0"); +static_assert(v8::ONLY_WRITABLE == 1, "v8::ONLY_WRITABLE is not 1"); +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(v8::KeyConversionMode::kConvertToString) == 0, + "v8::KeyConversionMode::kConvertToString is not 0"); +static_assert(static_cast(v8::KeyConversionMode::kKeepNumbers) == 1, + "v8::KeyConversionMode::kKeepNumbers is not 1"); +static_assert(static_cast(v8::KeyConversionMode::kNoNumbers) == 2, + "v8::KeyConversionMode::kNoNumbers is not 2"); + +// v8::KeyCollectionMode +static_assert(static_cast(v8::KeyCollectionMode::kOwnOnly) == 0, + "v8::KeyCollectionMode::kOwnOnly is not 0"); +static_assert(static_cast(v8::KeyCollectionMode::kIncludePrototypes) == 1, + "v8::KeyCollectionMode::kIncludePrototypes is not 1"); + +// v8::IndexFilter +static_assert(static_cast(v8::IndexFilter::kIncludeIndices) == 0, + "v8::IndexFilter::kIncludeIndices is not 0"); +static_assert(static_cast(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::Context* context) { - return maybe_local_to_ptr( - ptr_to_local(self)->GetPropertyNames(ptr_to_local(context))); +const v8::Array* v8__Object__GetPropertyNames( + const v8::Object* self, const v8::Context* context, + v8::KeyCollectionMode mode, v8::PropertyFilter property_filter, + 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, diff --git a/src/get_property_names_args_builder.rs b/src/get_property_names_args_builder.rs new file mode 100644 index 00000000..bb3c13d5 --- /dev/null +++ b/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 + } +} diff --git a/src/lib.rs b/src/lib.rs index c3f4f0b5..faaaf8ce 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/object.rs b/src/object.rs index bf8af59d..268fd0e4 100644 --- a/src/object.rs +++ b/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> { 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> { 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, + ) }) } } diff --git a/src/property_filter.rs b/src/property_filter.rs new file mode 100644 index 00000000..fe44f20b --- /dev/null +++ b/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()); +} diff --git a/tests/test_api.rs b/tests/test_api.rs index de7d0af4..443aa586 100644 --- a/tests/test_api.rs +++ b/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]