Add `NonMaxU32` as integer variant for `PropertyKey` (#3321)

* Add max array index

* Apply review

* Switch to u32

* Mark function as unsafe

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

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(_) => {}
                     }

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,
         }

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<bool> {
     // 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());
         }
     }
 

boa_engine/src/object/internal_methods/array.rs

@@ -32,7 +32,7 @@ pub(crate) fn array_exotic_define_own_property(
     context: &mut Context<'_>,
 ) -> JsResult<bool> {
     // 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)?

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

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(),
     };
 

boa_engine/src/object/internal_methods/string.rs

@@ -143,7 +143,7 @@ fn string_get_own_property(obj: &JsObject, key: &PropertyKey) -> Option<Property
     // 6. If IsIntegralNumber(index) is false, return undefined.
     // 7. If index is -0𝔽, return undefined.
     let pos = match key {
-        PropertyKey::Index(index) => *index as usize,
+        PropertyKey::Index(index) => index.get() as usize,
         _ => return None,
     };
 

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,
             };
 

boa_engine/src/object/property_map.rs

@@ -268,7 +268,7 @@ impl PropertyMap {
     #[must_use]
     pub fn get(&self, key: &PropertyKey) -> Option<PropertyDescriptor> {
         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;

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,11 +599,11 @@ pub enum PropertyKey {
     Symbol(JsSymbol),
 
     /// A numeric property key.
-    Index(u32),
+    Index(NonMaxU32),
 }
 
 /// Utility function for parsing [`PropertyKey`].
-fn parse_u32_index<I, T>(mut input: I) -> Option<u32>
+fn parse_u32_index<I, T>(mut input: I) -> Option<NonMaxU32>
 where
     I: Iterator<Item = T> + ExactSizeIterator + FusedIterator,
     T: Into<u16>,
@@ -634,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.
@@ -643,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 {
@@ -691,7 +697,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 +709,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 +721,84 @@ impl From<PropertyKey> 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<u8> 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<u16> 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<u32> 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<usize> 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<i64> 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<u64> 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<isize> 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<i32> 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<f64> 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,
+        )
     }
 }
 

boa_engine/src/property/nonmaxu32.rs

@@ -0,0 +1,36 @@
+/// An integer that is not `u32::MAX`.
+#[derive(PartialEq, Debug, Clone, Copy, Eq, Hash)]
+pub struct NonMaxU32 {
+    inner: u32,
+}
+
+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 {
+        debug_assert!(inner != u32::MAX);
+
+        Self { inner }
+    }
+
+    /// Creates a non-max `u32` if the given value is not `u32::MAX`.
+    #[must_use]
+    pub const fn new(inner: u32) -> Option<Self> {
+        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
+    }
+}

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")

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 {

boa_engine/src/vm/opcode/get/property.rs

@@ -79,7 +79,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);
@@ -125,7 +125,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());

boa_engine/src/vm/opcode/set/property.rs

@@ -99,7 +99,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());