From 270d912f7e863d3fa53078ce5b1d696a629a2b2b Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Fri, 29 Sep 2023 03:22:31 +0200 Subject: [PATCH 1/4] Add max array index --- boa_engine/src/builtins/function/mod.rs | 2 +- .../src/builtins/object/for_in_iterator.rs | 4 +- boa_engine/src/builtins/object/mod.rs | 2 +- .../src/object/internal_methods/arguments.rs | 20 ++++---- .../src/object/internal_methods/array.rs | 6 ++- .../internal_methods/integer_indexed.rs | 16 +++--- .../internal_methods/module_namespace.rs | 8 +-- .../src/object/internal_methods/string.rs | 2 +- boa_engine/src/object/operations.rs | 2 +- boa_engine/src/object/property_map.rs | 8 +-- boa_engine/src/property/mod.rs | 51 ++++++++++++------- boa_engine/src/property/nonmaxu32.rs | 39 ++++++++++++++ .../src/value/conversions/serde_json.rs | 2 +- boa_engine/src/value/display.rs | 2 +- boa_engine/src/vm/opcode/get/property.rs | 4 +- boa_engine/src/vm/opcode/set/property.rs | 2 +- 16 files changed, 113 insertions(+), 57 deletions(-) create mode 100644 boa_engine/src/property/nonmaxu32.rs diff --git a/boa_engine/src/builtins/function/mod.rs b/boa_engine/src/builtins/function/mod.rs index 9574320945f..a9c1e4027fb 100644 --- a/boa_engine/src/builtins/function/mod.rs +++ b/boa_engine/src/builtins/function/mod.rs @@ -1048,7 +1048,7 @@ pub(crate) fn set_function_name( ) } PropertyKey::String(string) => string.clone(), - PropertyKey::Index(index) => js_string!(format!("{index}")), + PropertyKey::Index(index) => js_string!(format!("{}", index.get())), }; // 3. Else if name is a Private Name, then diff --git a/boa_engine/src/builtins/object/for_in_iterator.rs b/boa_engine/src/builtins/object/for_in_iterator.rs index 3d521ff6cb6..95cb76b71cb 100644 --- a/boa_engine/src/builtins/object/for_in_iterator.rs +++ b/boa_engine/src/builtins/object/for_in_iterator.rs @@ -117,7 +117,9 @@ impl ForInIterator { iterator.remaining_keys.push_back(k.clone()); } PropertyKey::Index(i) => { - iterator.remaining_keys.push_back(i.to_string().into()); + iterator + .remaining_keys + .push_back(i.get().to_string().into()); } PropertyKey::Symbol(_) => {} } diff --git a/boa_engine/src/builtins/object/mod.rs b/boa_engine/src/builtins/object/mod.rs index a74e6ab5250..e1222cd4479 100644 --- a/boa_engine/src/builtins/object/mod.rs +++ b/boa_engine/src/builtins/object/mod.rs @@ -1420,7 +1420,7 @@ fn get_own_property_keys( (PropertyKeyType::String, PropertyKey::String(_)) | (PropertyKeyType::Symbol, PropertyKey::Symbol(_)) => Some(next_key.into()), (PropertyKeyType::String, PropertyKey::Index(index)) => { - Some(js_string!(index.to_string()).into()) + Some(js_string!(index.get().to_string()).into()) } _ => None, } diff --git a/boa_engine/src/object/internal_methods/arguments.rs b/boa_engine/src/object/internal_methods/arguments.rs index bb919def58a..e086cde3d29 100644 --- a/boa_engine/src/object/internal_methods/arguments.rs +++ b/boa_engine/src/object/internal_methods/arguments.rs @@ -41,7 +41,7 @@ pub(crate) fn arguments_exotic_get_own_property( .borrow() .as_mapped_arguments() .expect("arguments exotic method must only be callable from arguments objects") - .get(*index) + .get(index.get()) { // a. Set desc.[[Value]] to Get(map, P). return Ok(Some( @@ -73,12 +73,12 @@ pub(crate) fn arguments_exotic_define_own_property( context: &mut Context<'_>, ) -> JsResult { // 2. Let isMapped be HasOwnProperty(map, P). - let mapped = if let &PropertyKey::Index(index) = key { + let mapped = if let &PropertyKey::Index(index) = &key { // 1. Let map be args.[[ParameterMap]]. obj.borrow() .as_mapped_arguments() .expect("arguments exotic method must only be callable from arguments objects") - .get(index) + .get(index.get()) .map(|value| (index, value)) } else { None @@ -128,7 +128,7 @@ pub(crate) fn arguments_exotic_define_own_property( // a. If IsAccessorDescriptor(Desc) is true, then if desc.is_accessor_descriptor() { // i. Call map.[[Delete]](P). - map.delete(index); + map.delete(index.get()); } // b. Else, else { @@ -136,13 +136,13 @@ pub(crate) fn arguments_exotic_define_own_property( if let Some(value) = desc.value() { // 1. Let setStatus be Set(map, P, Desc.[[Value]], false). // 2. Assert: setStatus is true because formal parameters mapped by argument objects are always writable. - map.set(index, value); + map.set(index.get(), value); } // ii. If Desc.[[Writable]] is present and its value is false, then if desc.writable() == Some(false) { // 1. Call map.[[Delete]](P). - map.delete(index); + map.delete(index.get()); } } } @@ -170,7 +170,7 @@ pub(crate) fn arguments_exotic_get( .borrow() .as_mapped_arguments() .expect("arguments exotic method must only be callable from arguments objects") - .get(*index) + .get(index.get()) { // a. Assert: map contains a formal parameter mapping for P. // b. Return Get(map, P). @@ -199,7 +199,7 @@ pub(crate) fn arguments_exotic_set( // 1. If SameValue(args, Receiver) is false, then // a. Let isMapped be false. // 2. Else, - if let PropertyKey::Index(index) = key { + if let PropertyKey::Index(index) = &key { if JsValue::same_value(&obj.clone().into(), &receiver) { // a. Let map be args.[[ParameterMap]]. // b. Let isMapped be ! HasOwnProperty(map, P). @@ -209,7 +209,7 @@ pub(crate) fn arguments_exotic_set( obj.borrow_mut() .as_mapped_arguments_mut() .expect("arguments exotic method must only be callable from arguments objects") - .set(index, &value); + .set(index.get(), &value); } } @@ -240,7 +240,7 @@ pub(crate) fn arguments_exotic_delete( obj.borrow_mut() .as_mapped_arguments_mut() .expect("arguments exotic method must only be callable from arguments objects") - .delete(*index); + .delete(index.get()); } } diff --git a/boa_engine/src/object/internal_methods/array.rs b/boa_engine/src/object/internal_methods/array.rs index 6e095174f51..0d95759acf8 100644 --- a/boa_engine/src/object/internal_methods/array.rs +++ b/boa_engine/src/object/internal_methods/array.rs @@ -32,7 +32,7 @@ pub(crate) fn array_exotic_define_own_property( context: &mut Context<'_>, ) -> JsResult { // 1. Assert: IsPropertyKey(P) is true. - match *key { + match key { // 2. If P is "length", then PropertyKey::String(ref s) if s == utf16!("length") => { // a. Return ? ArraySetLength(A, Desc). @@ -40,7 +40,9 @@ pub(crate) fn array_exotic_define_own_property( array_set_length(obj, desc, context) } // 3. Else if P is an array index, then - PropertyKey::Index(index) if index < u32::MAX => { + PropertyKey::Index(index) => { + let index = index.get(); + // a. Let oldLenDesc be OrdinaryGetOwnProperty(A, "length"). let old_len_desc = super::ordinary_get_own_property(obj, &utf16!("length").into(), context)? diff --git a/boa_engine/src/object/internal_methods/integer_indexed.rs b/boa_engine/src/object/internal_methods/integer_indexed.rs index 71c1255095c..cd722b935d2 100644 --- a/boa_engine/src/object/internal_methods/integer_indexed.rs +++ b/boa_engine/src/object/internal_methods/integer_indexed.rs @@ -69,7 +69,7 @@ pub(crate) fn integer_indexed_exotic_get_own_property( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -111,7 +111,7 @@ pub(crate) fn integer_indexed_exotic_has_property( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -142,7 +142,7 @@ pub(crate) fn integer_indexed_exotic_define_own_property( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -204,7 +204,7 @@ pub(crate) fn integer_indexed_exotic_get( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -237,7 +237,7 @@ pub(crate) fn integer_indexed_exotic_set( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -279,7 +279,7 @@ pub(crate) fn integer_indexed_exotic_delete( // 1.a. Let numericIndex be CanonicalNumericIndexString(P). canonical_numeric_index_string(key) } - PropertyKey::Index(index) => Some((*index).into()), + PropertyKey::Index(index) => Some(index.get().into()), PropertyKey::Symbol(_) => None, }; @@ -317,9 +317,7 @@ pub(crate) fn integer_indexed_exotic_own_property_keys( // 2. If IsDetachedBuffer(O.[[ViewedArrayBuffer]]) is false, then // a. For each integer i starting with 0 such that i < O.[[ArrayLength]], in ascending order, do // i. Add ! ToString(𝔽(i)) as the last element of keys. - (0..inner.array_length()) - .map(|index| PropertyKey::Index(index as u32)) - .collect() + (0..inner.array_length()).map(PropertyKey::from).collect() }; // 3. For each own property key P of O such that Type(P) is String and P is not an array index, in ascending chronological order of property creation, do diff --git a/boa_engine/src/object/internal_methods/module_namespace.rs b/boa_engine/src/object/internal_methods/module_namespace.rs index 1b95eec8a00..359ed2cd8f3 100644 --- a/boa_engine/src/object/internal_methods/module_namespace.rs +++ b/boa_engine/src/object/internal_methods/module_namespace.rs @@ -89,7 +89,7 @@ fn module_namespace_exotic_get_own_property( // 1. If P is a Symbol, return OrdinaryGetOwnProperty(O, P). let key = match key { PropertyKey::Symbol(_) => return ordinary_get_own_property(obj, key, context), - PropertyKey::Index(idx) => js_string!(format!("{idx}")), + PropertyKey::Index(idx) => js_string!(format!("{}", idx.get())), PropertyKey::String(s) => s.clone(), }; @@ -169,7 +169,7 @@ fn module_namespace_exotic_has_property( // 1. If P is a Symbol, return ! OrdinaryHasProperty(O, P). let key = match key { PropertyKey::Symbol(_) => return ordinary_has_property(obj, key, context), - PropertyKey::Index(idx) => js_string!(format!("{idx}")), + PropertyKey::Index(idx) => js_string!(format!("{}", idx.get())), PropertyKey::String(s) => s.clone(), }; @@ -199,7 +199,7 @@ fn module_namespace_exotic_get( // a. Return ! OrdinaryGet(O, P, Receiver). let key = match key { PropertyKey::Symbol(_) => return ordinary_get(obj, key, receiver, context), - PropertyKey::Index(idx) => js_string!(format!("{idx}")), + PropertyKey::Index(idx) => js_string!(format!("{}", idx.get())), PropertyKey::String(s) => s.clone(), }; @@ -287,7 +287,7 @@ fn module_namespace_exotic_delete( // a. Return ! OrdinaryDelete(O, P). let key = match key { PropertyKey::Symbol(_) => return ordinary_delete(obj, key, context), - PropertyKey::Index(idx) => js_string!(format!("{idx}")), + PropertyKey::Index(idx) => js_string!(format!("{}", idx.get())), PropertyKey::String(s) => s.clone(), }; diff --git a/boa_engine/src/object/internal_methods/string.rs b/boa_engine/src/object/internal_methods/string.rs index 1a99da753bc..aacb579603b 100644 --- a/boa_engine/src/object/internal_methods/string.rs +++ b/boa_engine/src/object/internal_methods/string.rs @@ -143,7 +143,7 @@ fn string_get_own_property(obj: &JsObject, key: &PropertyKey) -> Option *index as usize, + PropertyKey::Index(index) => index.get() as usize, _ => return None, }; diff --git a/boa_engine/src/object/operations.rs b/boa_engine/src/object/operations.rs index a2861c47016..3b294039e4c 100644 --- a/boa_engine/src/object/operations.rs +++ b/boa_engine/src/object/operations.rs @@ -568,7 +568,7 @@ impl JsObject { // a. If Type(key) is String, then let key_str = match &key { PropertyKey::String(s) => Some(s.clone()), - PropertyKey::Index(i) => Some(i.to_string().into()), + PropertyKey::Index(i) => Some(i.get().to_string().into()), PropertyKey::Symbol(_) => None, }; diff --git a/boa_engine/src/object/property_map.rs b/boa_engine/src/object/property_map.rs index 1bea280e093..8320fe1d88a 100644 --- a/boa_engine/src/object/property_map.rs +++ b/boa_engine/src/object/property_map.rs @@ -268,7 +268,7 @@ impl PropertyMap { #[must_use] pub fn get(&self, key: &PropertyKey) -> Option { if let PropertyKey::Index(index) = key { - return self.indexed_properties.get(*index); + return self.indexed_properties.get(index.get()); } if let Some(slot) = self.shape.lookup(key) { return Some(self.get_storage(slot)); @@ -301,7 +301,7 @@ impl PropertyMap { /// Insert the given property descriptor with the given key [`PropertyMap`]. pub fn insert(&mut self, key: &PropertyKey, property: PropertyDescriptor) -> bool { if let PropertyKey::Index(index) = key { - return self.indexed_properties.insert(*index, property); + return self.indexed_properties.insert(index.get(), property); } let attributes = property.to_slot_attributes(); @@ -390,7 +390,7 @@ impl PropertyMap { /// Remove the property with the given key from the [`PropertyMap`]. pub fn remove(&mut self, key: &PropertyKey) -> bool { if let PropertyKey::Index(index) = key { - return self.indexed_properties.remove(*index); + return self.indexed_properties.remove(index.get()); } if let Some(slot) = self.shape.lookup(key) { // shift all elements when removing. @@ -461,7 +461,7 @@ impl PropertyMap { #[must_use] pub fn contains_key(&self, key: &PropertyKey) -> bool { if let PropertyKey::Index(index) = key { - return self.indexed_properties.contains_key(*index); + return self.indexed_properties.contains_key(index.get()); } if self.shape.lookup(key).is_some() { return true; diff --git a/boa_engine/src/property/mod.rs b/boa_engine/src/property/mod.rs index 4d68fa4c8b2..b462892241d 100644 --- a/boa_engine/src/property/mod.rs +++ b/boa_engine/src/property/mod.rs @@ -16,12 +16,13 @@ //! [section]: https://tc39.es/ecma262/#sec-property-attributes mod attribute; +mod nonmaxu32; use crate::{js_string, object::shape::slot::SlotAttributes, JsString, JsSymbol, JsValue}; use boa_gc::{Finalize, Trace}; use std::{fmt, iter::FusedIterator}; -pub use attribute::Attribute; +pub use {attribute::Attribute, nonmaxu32::NonMaxU32}; /// This represents an ECMAScript Property AKA The Property Descriptor. /// @@ -598,7 +599,7 @@ pub enum PropertyKey { Symbol(JsSymbol), /// A numeric property key. - Index(u32), + Index(NonMaxU32), } /// Utility function for parsing [`PropertyKey`]. @@ -674,7 +675,9 @@ impl From<&[u16]> for PropertyKey { impl From for PropertyKey { #[inline] fn from(string: JsString) -> Self { - parse_u32_index(string.as_slice().iter().copied()).map_or(Self::String(string), Self::Index) + parse_u32_index(string.as_slice().iter().copied()) + .and_then(NonMaxU32::new) + .map_or(Self::String(string), Self::Index) } } @@ -691,7 +694,7 @@ impl fmt::Display for PropertyKey { match self { Self::String(ref string) => string.to_std_string_escaped().fmt(f), Self::Symbol(ref symbol) => symbol.descriptive_string().to_std_string_escaped().fmt(f), - Self::Index(index) => index.fmt(f), + Self::Index(index) => index.get().fmt(f), } } } @@ -703,7 +706,7 @@ impl From<&PropertyKey> for JsValue { PropertyKey::String(ref string) => string.clone().into(), PropertyKey::Symbol(ref symbol) => symbol.clone().into(), PropertyKey::Index(index) => { - i32::try_from(*index).map_or_else(|_| Self::new(*index), Self::new) + i32::try_from(index.get()).map_or_else(|_| Self::new(index.get()), Self::new) } } } @@ -715,72 +718,84 @@ impl From for JsValue { match property_key { PropertyKey::String(ref string) => string.clone().into(), PropertyKey::Symbol(ref symbol) => symbol.clone().into(), - PropertyKey::Index(index) => js_string!(index.to_string()).into(), + PropertyKey::Index(index) => js_string!(index.get().to_string()).into(), } } } impl From for PropertyKey { fn from(value: u8) -> Self { - Self::Index(value.into()) + // SAFETY: `u8` can never be `u32::MAX`. + unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } } } impl From for PropertyKey { fn from(value: u16) -> Self { - Self::Index(value.into()) + // SAFETY: `u16` can never be `u32::MAX`. + unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } } } impl From for PropertyKey { fn from(value: u32) -> Self { - Self::Index(value) + NonMaxU32::new(value).map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: usize) -> Self { u32::try_from(value) - .map_or_else(|_| Self::String(js_string!(value.to_string())), Self::Index) + .ok() + .and_then(NonMaxU32::new) + .map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: i64) -> Self { u32::try_from(value) - .map_or_else(|_| Self::String(js_string!(value.to_string())), Self::Index) + .ok() + .and_then(NonMaxU32::new) + .map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: u64) -> Self { u32::try_from(value) - .map_or_else(|_| Self::String(js_string!(value.to_string())), Self::Index) + .ok() + .and_then(NonMaxU32::new) + .map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: isize) -> Self { u32::try_from(value) - .map_or_else(|_| Self::String(js_string!(value.to_string())), Self::Index) + .ok() + .and_then(NonMaxU32::new) + .map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: i32) -> Self { u32::try_from(value) - .map_or_else(|_| Self::String(js_string!(value.to_string())), Self::Index) + .ok() + .and_then(NonMaxU32::new) + .map_or(Self::String(js_string!(value.to_string())), Self::Index) } } impl From for PropertyKey { fn from(value: f64) -> Self { use num_traits::cast::FromPrimitive; - if let Some(index) = u32::from_f64(value) { - return Self::Index(index); - } - Self::String(ryu_js::Buffer::new().format(value).into()) + u32::from_f64(value).and_then(NonMaxU32::new).map_or( + Self::String(ryu_js::Buffer::new().format(value).into()), + Self::Index, + ) } } diff --git a/boa_engine/src/property/nonmaxu32.rs b/boa_engine/src/property/nonmaxu32.rs new file mode 100644 index 00000000000..de0ffc78068 --- /dev/null +++ b/boa_engine/src/property/nonmaxu32.rs @@ -0,0 +1,39 @@ +use std::num::NonZeroU32; + +/// An integer that is not `u32::MAX`. +#[derive(PartialEq, Debug, Clone, Copy, Eq, Hash)] +pub struct NonMaxU32 { + inner: NonZeroU32, +} + +impl NonMaxU32 { + /// Creates a non-max `u32`. + /// + /// # Safety + /// + /// The caller must ensure that the given value is not `u32::MAX`. + #[must_use] + pub const unsafe fn new_unchecked(inner: u32) -> Self { + // SAFETY: The caller must ensure that `inner` is not `u32::MAX`. + let inner = unsafe { NonZeroU32::new_unchecked(inner + 1) }; + + Self { inner } + } + + /// Creates a non-max `u32` if the given value is not `u32::MAX`. + #[must_use] + pub const fn new(inner: u32) -> Option { + if inner == u32::MAX { + return None; + } + + // SAFETY: We checked that `inner` is not `u32::MAX`. + unsafe { Some(Self::new_unchecked(inner)) } + } + + /// Returns the value as a primitive type. + #[must_use] + pub const fn get(&self) -> u32 { + self.inner.get() - 1 + } +} diff --git a/boa_engine/src/value/conversions/serde_json.rs b/boa_engine/src/value/conversions/serde_json.rs index 3f84565d657..f4f73f63eeb 100644 --- a/boa_engine/src/value/conversions/serde_json.rs +++ b/boa_engine/src/value/conversions/serde_json.rs @@ -143,7 +143,7 @@ impl JsValue { for property_key in obj.borrow().properties().shape.keys() { let key = match &property_key { PropertyKey::String(string) => string.to_std_string_escaped(), - PropertyKey::Index(i) => i.to_string(), + PropertyKey::Index(i) => i.get().to_string(), PropertyKey::Symbol(_sym) => { return Err(JsNativeError::typ() .with_message("cannot convert Symbol to JSON") diff --git a/boa_engine/src/value/display.rs b/boa_engine/src/value/display.rs index e0b4352f330..1cdff70f63a 100644 --- a/boa_engine/src/value/display.rs +++ b/boa_engine/src/value/display.rs @@ -67,7 +67,7 @@ macro_rules! print_obj_value { } }; (props of $obj:expr, $display_fn:ident, $indent:expr, $encounters:expr, $print_internals:expr) => { - {let mut keys: Vec<_> = $obj.borrow().properties().index_property_keys().map(crate::property::PropertyKey::Index).collect(); + {let mut keys: Vec<_> = $obj.borrow().properties().index_property_keys().map(crate::property::PropertyKey::from).collect(); keys.extend($obj.borrow().properties().shape.keys()); let mut result = Vec::default(); for key in keys { diff --git a/boa_engine/src/vm/opcode/get/property.rs b/boa_engine/src/vm/opcode/get/property.rs index 5028b343cfb..c183ebec3a8 100644 --- a/boa_engine/src/vm/opcode/get/property.rs +++ b/boa_engine/src/vm/opcode/get/property.rs @@ -66,7 +66,7 @@ impl Operation for GetPropertyByValue { if let Some(element) = object_borrowed .properties() .dense_indexed_properties() - .and_then(|vec| vec.get(*index as usize)) + .and_then(|vec| vec.get(index.get() as usize)) { context.vm.push(element.clone()); return Ok(CompletionType::Normal); @@ -137,7 +137,7 @@ impl Operation for GetPropertyByValuePush { if let Some(element) = object_borrowed .properties() .dense_indexed_properties() - .and_then(|vec| vec.get(*index as usize)) + .and_then(|vec| vec.get(index.get() as usize)) { context.vm.push(key); context.vm.push(element.clone()); diff --git a/boa_engine/src/vm/opcode/set/property.rs b/boa_engine/src/vm/opcode/set/property.rs index 245f3184619..afa2c42e66c 100644 --- a/boa_engine/src/vm/opcode/set/property.rs +++ b/boa_engine/src/vm/opcode/set/property.rs @@ -86,7 +86,7 @@ impl Operation for SetPropertyByValue { .properties_mut() .dense_indexed_properties_mut() { - let index = *index as usize; + let index = index.get() as usize; if let Some(element) = dense_elements.get_mut(index) { *element = value; context.vm.push(element.clone()); From db37eed4efd0157e09be168ceb108c10e8f63a07 Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Fri, 29 Sep 2023 21:11:40 +0200 Subject: [PATCH 2/4] Apply review --- boa_engine/src/property/mod.rs | 19 +++++++++++-------- boa_engine/src/property/nonmaxu32.rs | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/boa_engine/src/property/mod.rs b/boa_engine/src/property/mod.rs index b462892241d..23ab3834730 100644 --- a/boa_engine/src/property/mod.rs +++ b/boa_engine/src/property/mod.rs @@ -603,7 +603,7 @@ pub enum PropertyKey { } /// Utility function for parsing [`PropertyKey`]. -fn parse_u32_index(mut input: I) -> Option +fn parse_u32_index(mut input: I) -> Option where I: Iterator + ExactSizeIterator + FusedIterator, T: Into, @@ -635,7 +635,8 @@ where let byte = input.next()?.into(); if byte == CHAR_ZERO { if len == 1 { - return Some(0); + // SAFETY: `0` is not `u32::MAX`. + return unsafe { Some(NonMaxU32::new_unchecked(0)) }; } // String "012345" is not a valid index. @@ -644,19 +645,23 @@ where let mut result = to_digit(byte)?; - // If the len is equal to max chars, then we need to do checked opterations, + // If the len is equal to max chars, then we need to do checked operations, // in case of overflows. If less use unchecked versions. if len == MAX_CHAR_COUNT { for c in input { result = result.checked_mul(10)?.checked_add(to_digit(c.into())?)?; } + + NonMaxU32::new(result) } else { for c in input { result = result * 10 + to_digit(c.into())?; } - } - Some(result) + // SAFETY: `result` cannot be `u32::MAX`, + // because the length of the input is smaller than `MAX_CHAR_COUNT`. + unsafe { Some(NonMaxU32::new_unchecked(result)) } + } } impl From<&[u16]> for PropertyKey { @@ -675,9 +680,7 @@ impl From<&[u16]> for PropertyKey { impl From for PropertyKey { #[inline] fn from(string: JsString) -> Self { - parse_u32_index(string.as_slice().iter().copied()) - .and_then(NonMaxU32::new) - .map_or(Self::String(string), Self::Index) + parse_u32_index(string.as_slice().iter().copied()).map_or(Self::String(string), Self::Index) } } diff --git a/boa_engine/src/property/nonmaxu32.rs b/boa_engine/src/property/nonmaxu32.rs index de0ffc78068..b9ebe3d8665 100644 --- a/boa_engine/src/property/nonmaxu32.rs +++ b/boa_engine/src/property/nonmaxu32.rs @@ -15,7 +15,7 @@ impl NonMaxU32 { #[must_use] pub const unsafe fn new_unchecked(inner: u32) -> Self { // SAFETY: The caller must ensure that `inner` is not `u32::MAX`. - let inner = unsafe { NonZeroU32::new_unchecked(inner + 1) }; + let inner = unsafe { NonZeroU32::new_unchecked(inner.wrapping_add(1)) }; Self { inner } } @@ -34,6 +34,6 @@ impl NonMaxU32 { /// Returns the value as a primitive type. #[must_use] pub const fn get(&self) -> u32 { - self.inner.get() - 1 + self.inner.get().wrapping_sub(1) } } From d775d379e80189acf6b76b3ea58f4a4844430fea Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Fri, 29 Sep 2023 21:39:23 +0200 Subject: [PATCH 3/4] Switch to u32 --- boa_engine/src/property/mod.rs | 8 ++++---- boa_engine/src/property/nonmaxu32.rs | 14 +++++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/boa_engine/src/property/mod.rs b/boa_engine/src/property/mod.rs index 23ab3834730..39fe0158808 100644 --- a/boa_engine/src/property/mod.rs +++ b/boa_engine/src/property/mod.rs @@ -636,7 +636,7 @@ where if byte == CHAR_ZERO { if len == 1 { // SAFETY: `0` is not `u32::MAX`. - return unsafe { Some(NonMaxU32::new_unchecked(0)) }; + return Some(NonMaxU32::new_unchecked(0)); } // String "012345" is not a valid index. @@ -660,7 +660,7 @@ where // SAFETY: `result` cannot be `u32::MAX`, // because the length of the input is smaller than `MAX_CHAR_COUNT`. - unsafe { Some(NonMaxU32::new_unchecked(result)) } + Some(NonMaxU32::new_unchecked(result)) } } @@ -729,14 +729,14 @@ impl From for JsValue { impl From for PropertyKey { fn from(value: u8) -> Self { // SAFETY: `u8` can never be `u32::MAX`. - unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } + Self::Index(NonMaxU32::new_unchecked(value.into())) } } impl From for PropertyKey { fn from(value: u16) -> Self { // SAFETY: `u16` can never be `u32::MAX`. - unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } + Self::Index(NonMaxU32::new_unchecked(value.into())) } } diff --git a/boa_engine/src/property/nonmaxu32.rs b/boa_engine/src/property/nonmaxu32.rs index b9ebe3d8665..9fd06c412b0 100644 --- a/boa_engine/src/property/nonmaxu32.rs +++ b/boa_engine/src/property/nonmaxu32.rs @@ -1,9 +1,7 @@ -use std::num::NonZeroU32; - /// An integer that is not `u32::MAX`. #[derive(PartialEq, Debug, Clone, Copy, Eq, Hash)] pub struct NonMaxU32 { - inner: NonZeroU32, + inner: u32, } impl NonMaxU32 { @@ -13,9 +11,8 @@ impl NonMaxU32 { /// /// The caller must ensure that the given value is not `u32::MAX`. #[must_use] - pub const unsafe fn new_unchecked(inner: u32) -> Self { - // SAFETY: The caller must ensure that `inner` is not `u32::MAX`. - let inner = unsafe { NonZeroU32::new_unchecked(inner.wrapping_add(1)) }; + pub const fn new_unchecked(inner: u32) -> Self { + debug_assert!(inner != u32::MAX); Self { inner } } @@ -27,13 +24,12 @@ impl NonMaxU32 { return None; } - // SAFETY: We checked that `inner` is not `u32::MAX`. - unsafe { Some(Self::new_unchecked(inner)) } + Some(Self::new_unchecked(inner)) } /// Returns the value as a primitive type. #[must_use] pub const fn get(&self) -> u32 { - self.inner.get().wrapping_sub(1) + self.inner } } From 1c4dde416fd7713f2e03d4f3b994fac214a97391 Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Sat, 30 Sep 2023 19:23:35 +0200 Subject: [PATCH 4/4] Mark function as unsafe --- boa_engine/src/property/mod.rs | 8 ++++---- boa_engine/src/property/nonmaxu32.rs | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/boa_engine/src/property/mod.rs b/boa_engine/src/property/mod.rs index 39fe0158808..23ab3834730 100644 --- a/boa_engine/src/property/mod.rs +++ b/boa_engine/src/property/mod.rs @@ -636,7 +636,7 @@ where if byte == CHAR_ZERO { if len == 1 { // SAFETY: `0` is not `u32::MAX`. - return Some(NonMaxU32::new_unchecked(0)); + return unsafe { Some(NonMaxU32::new_unchecked(0)) }; } // String "012345" is not a valid index. @@ -660,7 +660,7 @@ where // SAFETY: `result` cannot be `u32::MAX`, // because the length of the input is smaller than `MAX_CHAR_COUNT`. - Some(NonMaxU32::new_unchecked(result)) + unsafe { Some(NonMaxU32::new_unchecked(result)) } } } @@ -729,14 +729,14 @@ impl From for JsValue { impl From for PropertyKey { fn from(value: u8) -> Self { // SAFETY: `u8` can never be `u32::MAX`. - Self::Index(NonMaxU32::new_unchecked(value.into())) + unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } } } impl From for PropertyKey { fn from(value: u16) -> Self { // SAFETY: `u16` can never be `u32::MAX`. - Self::Index(NonMaxU32::new_unchecked(value.into())) + unsafe { Self::Index(NonMaxU32::new_unchecked(value.into())) } } } diff --git a/boa_engine/src/property/nonmaxu32.rs b/boa_engine/src/property/nonmaxu32.rs index 9fd06c412b0..c3a7fddcb25 100644 --- a/boa_engine/src/property/nonmaxu32.rs +++ b/boa_engine/src/property/nonmaxu32.rs @@ -11,7 +11,7 @@ impl NonMaxU32 { /// /// The caller must ensure that the given value is not `u32::MAX`. #[must_use] - pub const fn new_unchecked(inner: u32) -> Self { + pub const unsafe fn new_unchecked(inner: u32) -> Self { debug_assert!(inner != u32::MAX); Self { inner } @@ -24,7 +24,8 @@ impl NonMaxU32 { return None; } - Some(Self::new_unchecked(inner)) + // SAFETY: We checked that `inner` is not `u32::MAX`. + unsafe { Some(Self::new_unchecked(inner)) } } /// Returns the value as a primitive type.