Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Define array index as NonMaxU32
Browse files Browse the repository at this point in the history
Based on the definition in spec, [array index][] is only valid from zero
to u32::MAX - 1. It would be helpful to use the NonMaxU32 type to define
it as the property key instead so that contributors won't forget to
check the validity.

[array index]: https://tc39.es/ecma262/#array-index
CYBAI committed Jun 20, 2022
1 parent 28957fa commit f18bf35
Showing 7 changed files with 89 additions and 55 deletions.
12 changes: 6 additions & 6 deletions boa_engine/src/object/internal_methods/arguments.rs
Original file line number Diff line number Diff line change
@@ -44,7 +44,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 as usize)
.get(index.get() as usize)
{
// a. Set desc.[[Value]] to Get(map, P).
return Ok(Some(
@@ -81,8 +81,8 @@ pub(crate) fn arguments_exotic_define_own_property(
obj.borrow()
.as_mapped_arguments()
.expect("arguments exotic method must only be callable from arguments objects")
.get(index as usize)
.map(|value| (index as usize, value))
.get(index.get() as usize)
.map(|value| (index.get() as usize, value))
} else {
None
};
@@ -173,7 +173,7 @@ pub(crate) fn arguments_exotic_get(
.borrow()
.as_mapped_arguments()
.expect("arguments exotic method must only be callable from arguments objects")
.get(*index as usize)
.get(index.get() as usize)
{
// a. Assert: map contains a formal parameter mapping for P.
// b. Return Get(map, P).
@@ -212,7 +212,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 as usize, &value);
.set(index.get() as usize, &value);
}
}

@@ -243,7 +243,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 as usize);
.delete(index.get() as usize);
}
}

8 changes: 6 additions & 2 deletions boa_engine/src/object/internal_methods/array.rs
Original file line number Diff line number Diff line change
@@ -38,7 +38,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, &"length".into(), context)?
.expect("the property descriptor must exist");
@@ -201,14 +203,16 @@ fn array_set_length(
.borrow()
.properties
.index_property_keys()
.filter(|idx| new_len <= **idx && **idx < u32::MAX)
.filter(|idx| new_len <= idx.get())
.copied()
.collect();
keys.sort_unstable_by(|x, y| y.cmp(x));
keys
};

for index in ordered_keys {
let index = index.get();

// a. Let deleteSucceeded be ! A.[[Delete]](P).
// b. If deleteSucceeded is false, then
if !obj.__delete__(&index.into(), context)? {
33 changes: 18 additions & 15 deletions boa_engine/src/object/internal_methods/integer_indexed.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::{
builtins::{array_buffer::SharedMemoryOrder, typed_array::integer_indexed_object::ContentType},
nonmaxu32::NonMaxU32,
object::JsObject,
property::{PropertyDescriptor, PropertyKey},
Context, JsResult, JsValue,
@@ -44,14 +45,16 @@ pub(crate) fn integer_indexed_exotic_get_own_property(
// i. Let value be ! IntegerIndexedElementGet(O, numericIndex).
// ii. If value is undefined, return undefined.
// iii. Return the PropertyDescriptor { [[Value]]: value, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true }.
Ok(integer_indexed_element_get(obj, *index as usize).map(|v| {
PropertyDescriptor::builder()
.value(v)
.writable(true)
.enumerable(true)
.configurable(true)
.build()
}))
Ok(
integer_indexed_element_get(obj, index.get() as usize).map(|v| {
PropertyDescriptor::builder()
.value(v)
.writable(true)
.enumerable(true)
.configurable(true)
.build()
}),
)
} else {
// 2. Return OrdinaryGetOwnProperty(O, P).
super::ordinary_get_own_property(obj, key, context)
@@ -74,7 +77,7 @@ pub(crate) fn integer_indexed_exotic_has_property(
// a. Let numericIndex be ! CanonicalNumericIndexString(P).
if let PropertyKey::Index(index) = key {
// b. If numericIndex is not undefined, return ! IsValidIntegerIndex(O, numericIndex).
Ok(is_valid_integer_index(obj, *index as usize))
Ok(is_valid_integer_index(obj, index.get() as usize))
} else {
// 2. Return ? OrdinaryHasProperty(O, P).
super::ordinary_has_property(obj, key, context)
@@ -103,7 +106,7 @@ pub(crate) fn integer_indexed_exotic_define_own_property(
// iii. If Desc has an [[Enumerable]] field and if Desc.[[Enumerable]] is false, return false.
// v. If Desc has a [[Writable]] field and if Desc.[[Writable]] is false, return false.
// iv. If ! IsAccessorDescriptor(Desc) is true, return false.
if !is_valid_integer_index(obj, index as usize)
if !is_valid_integer_index(obj, index.get() as usize)
|| !desc
.configurable()
.or_else(|| desc.enumerable())
@@ -116,7 +119,7 @@ pub(crate) fn integer_indexed_exotic_define_own_property(

// vi. If Desc has a [[Value]] field, perform ? IntegerIndexedElementSet(O, numericIndex, Desc.[[Value]]).
if let Some(value) = desc.value() {
integer_indexed_element_set(obj, index as usize, value, context)?;
integer_indexed_element_set(obj, index.get() as usize, value, context)?;
}

// vii. Return true.
@@ -145,7 +148,7 @@ pub(crate) fn integer_indexed_exotic_get(
// b. If numericIndex is not undefined, then
if let PropertyKey::Index(index) = key {
// i. Return ! IntegerIndexedElementGet(O, numericIndex).
Ok(integer_indexed_element_get(obj, *index as usize).unwrap_or_default())
Ok(integer_indexed_element_get(obj, index.get() as usize).unwrap_or_default())
} else {
// 2. Return ? OrdinaryGet(O, P, Receiver).
super::ordinary_get(obj, key, receiver, context)
@@ -171,7 +174,7 @@ pub(crate) fn integer_indexed_exotic_set(
// b. If numericIndex is not undefined, then
if let PropertyKey::Index(index) = key {
// i. Perform ? IntegerIndexedElementSet(O, numericIndex, V).
integer_indexed_element_set(obj, index as usize, &value, context)?;
integer_indexed_element_set(obj, index.get() as usize, &value, context)?;

// ii. Return true.
Ok(true)
@@ -198,7 +201,7 @@ pub(crate) fn integer_indexed_exotic_delete(
// b. If numericIndex is not undefined, then
if let PropertyKey::Index(index) = key {
// i. If ! IsValidIntegerIndex(O, numericIndex) is false, return true; else return false.
Ok(!is_valid_integer_index(obj, *index as usize))
Ok(!is_valid_integer_index(obj, index.get() as usize))
} else {
// 2. Return ? OrdinaryDelete(O, P).
super::ordinary_delete(obj, key, context)
@@ -231,7 +234,7 @@ pub(crate) fn integer_indexed_exotic_own_property_keys(
// i. Add ! ToString(𝔽(i)) as the last element of keys.
(0..inner.array_length())
.into_iter()
.map(|index| PropertyKey::Index(index as u32))
.filter_map(|index| NonMaxU32::new(index as u32).map(PropertyKey::Index))
.collect()
};

2 changes: 1 addition & 1 deletion boa_engine/src/object/internal_methods/mod.rs
Original file line number Diff line number Diff line change
@@ -737,7 +737,7 @@ pub(crate) fn ordinary_own_property_keys(

// 2. For each own property key P of O such that P is an array index, in ascending numeric index order, do
// a. Add P as the last element of keys.
keys.extend(ordered_indexes.into_iter().map(Into::into));
keys.extend(ordered_indexes.into_iter().map(|idx| idx.get().into()));

// 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
// a. Add P as the last element of keys.
6 changes: 3 additions & 3 deletions boa_engine/src/object/internal_methods/string.rs
Original file line number Diff line number Diff line change
@@ -113,10 +113,10 @@ pub(crate) fn string_exotic_own_property_keys(
.properties
.index_property_keys()
.copied()
.filter(|idx| (*idx as usize) >= len)
.filter(|idx| (idx.get() as usize) >= len)
.collect();
remaining_indices.sort_unstable();
keys.extend(remaining_indices.into_iter().map(Into::into));
keys.extend(remaining_indices.into_iter().map(|idx| idx.get().into()));

// 7. 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
@@ -159,7 +159,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,
};

24 changes: 12 additions & 12 deletions boa_engine/src/object/property_map.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{PropertyDescriptor, PropertyKey};
use crate::{JsString, JsSymbol};
use crate::{nonmaxu32::NonMaxU32, JsString, JsSymbol};
use boa_gc::{custom_trace, Finalize, Trace};
use indexmap::IndexMap;
use rustc_hash::{FxHashMap, FxHasher};
@@ -30,7 +30,7 @@ unsafe impl<K: Trace> Trace for OrderedHashMap<K> {

#[derive(Default, Debug, Trace, Finalize)]
pub struct PropertyMap {
indexed_properties: FxHashMap<u32, PropertyDescriptor>,
indexed_properties: FxHashMap<NonMaxU32, PropertyDescriptor>,
/// Properties
string_properties: OrderedHashMap<JsString>,
/// Symbol Properties
@@ -126,15 +126,15 @@ impl PropertyMap {
SymbolPropertyValues(self.symbol_properties.0.values())
}

/// An iterator visiting all indexed key-value pairs in arbitrary order. The iterator element type is `(&'a u32, &'a Property)`.
/// An iterator visiting all indexed key-value pairs in arbitrary order. The iterator element type is `(&'a NonMaxU32, &'a Property)`.
///
/// This iterator does not recurse down the prototype chain.
#[inline]
pub fn index_properties(&self) -> IndexProperties<'_> {
IndexProperties(self.indexed_properties.iter())
}

/// An iterator visiting all index keys in arbitrary order. The iterator element type is `&'a u32`.
/// An iterator visiting all index keys in arbitrary order. The iterator element type is `&'a NonMaxU32`.
///
/// This iterator does not recurse down the prototype chain.
#[inline]
@@ -197,7 +197,7 @@ impl PropertyMap {
/// An iterator over the property entries of an `Object`
#[derive(Debug, Clone)]
pub struct Iter<'a> {
indexed_properties: hash_map::Iter<'a, u32, PropertyDescriptor>,
indexed_properties: hash_map::Iter<'a, NonMaxU32, PropertyDescriptor>,
string_properties: indexmap::map::Iter<'a, JsString, PropertyDescriptor>,
symbol_properties: indexmap::map::Iter<'a, JsSymbol, PropertyDescriptor>,
}
@@ -206,7 +206,7 @@ impl<'a> Iterator for Iter<'a> {
type Item = (PropertyKey, &'a PropertyDescriptor);
fn next(&mut self) -> Option<Self::Item> {
if let Some((key, value)) = self.indexed_properties.next() {
Some(((*key).into(), value))
Some((key.get().into(), value))
} else if let Some((key, value)) = self.string_properties.next() {
Some((key.clone().into(), value))
} else {
@@ -350,10 +350,10 @@ impl FusedIterator for SymbolPropertyValues<'_> {}

/// An iterator over the indexed property entries of an `Object`
#[derive(Debug, Clone)]
pub struct IndexProperties<'a>(hash_map::Iter<'a, u32, PropertyDescriptor>);
pub struct IndexProperties<'a>(hash_map::Iter<'a, NonMaxU32, PropertyDescriptor>);

impl<'a> Iterator for IndexProperties<'a> {
type Item = (&'a u32, &'a PropertyDescriptor);
type Item = (&'a NonMaxU32, &'a PropertyDescriptor);

#[inline]
fn next(&mut self) -> Option<Self::Item> {
@@ -375,12 +375,12 @@ impl ExactSizeIterator for IndexProperties<'_> {

impl FusedIterator for IndexProperties<'_> {}

/// An iterator over the index keys (`u32`) of an `Object`.
/// An iterator over the index keys (`NonMaxU32`) of an `Object`.
#[derive(Debug, Clone)]
pub struct IndexPropertyKeys<'a>(hash_map::Keys<'a, u32, PropertyDescriptor>);
pub struct IndexPropertyKeys<'a>(hash_map::Keys<'a, NonMaxU32, PropertyDescriptor>);

impl<'a> Iterator for IndexPropertyKeys<'a> {
type Item = &'a u32;
type Item = &'a NonMaxU32;

#[inline]
fn next(&mut self) -> Option<Self::Item> {
@@ -404,7 +404,7 @@ impl FusedIterator for IndexPropertyKeys<'_> {}

/// An iterator over the index values (`Property`) of an `Object`.
#[derive(Debug, Clone)]
pub struct IndexPropertyValues<'a>(hash_map::Values<'a, u32, PropertyDescriptor>);
pub struct IndexPropertyValues<'a>(hash_map::Values<'a, NonMaxU32, PropertyDescriptor>);

impl<'a> Iterator for IndexPropertyValues<'a> {
type Item = &'a PropertyDescriptor;
59 changes: 43 additions & 16 deletions boa_engine/src/property/mod.rs
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@
//! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty
//! [section]: https://tc39.es/ecma262/#sec-property-attributes
use crate::nonmaxu32::{NonMaxU32, TryFromIntError};
use crate::{JsString, JsSymbol, JsValue};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use std::fmt;
@@ -492,13 +493,13 @@ impl From<PropertyDescriptorBuilder> for PropertyDescriptor {
pub enum PropertyKey {
String(JsString),
Symbol(JsSymbol),
Index(u32),
Index(NonMaxU32),
}

impl From<JsString> for PropertyKey {
#[inline]
fn from(string: JsString) -> Self {
if let Ok(index) = string.parse() {
if let Ok(index) = string.parse::<NonMaxU32>() {
Self::Index(index)
} else {
Self::String(string)
@@ -509,7 +510,7 @@ impl From<JsString> for PropertyKey {
impl From<&str> for PropertyKey {
#[inline]
fn from(string: &str) -> Self {
if let Ok(index) = string.parse() {
if let Ok(index) = string.parse::<NonMaxU32>() {
Self::Index(index)
} else {
Self::String(string.into())
@@ -520,7 +521,7 @@ impl From<&str> for PropertyKey {
impl From<String> for PropertyKey {
#[inline]
fn from(string: String) -> Self {
if let Ok(index) = string.parse() {
if let Ok(index) = string.parse::<NonMaxU32>() {
Self::Index(index)
} else {
Self::String(string.into())
@@ -531,7 +532,7 @@ impl From<String> for PropertyKey {
impl From<Box<str>> for PropertyKey {
#[inline]
fn from(string: Box<str>) -> Self {
if let Ok(index) = string.parse() {
if let Ok(index) = string.parse::<NonMaxU32>() {
Self::Index(index)
} else {
Self::String(string.into())
@@ -564,10 +565,10 @@ impl From<&PropertyKey> for JsValue {
PropertyKey::String(ref string) => string.clone().into(),
PropertyKey::Symbol(ref symbol) => symbol.clone().into(),
PropertyKey::Index(index) => {
if let Ok(integer) = i32::try_from(*index) {
if let Ok(integer) = i32::try_from(index.get()) {
Self::new(integer)
} else {
Self::new(*index)
Self::new(index.get())
}
}
}
@@ -587,25 +588,39 @@ impl From<PropertyKey> for JsValue {

impl From<u8> for PropertyKey {
fn from(value: u8) -> Self {
Self::Index(value.into())
match NonMaxU32::new(value.into()) {
Some(index) => Self::Index(index),
// `u8` should always be valid for `NonMaxU32`
None => unreachable!(),
}
}
}

impl From<u16> for PropertyKey {
fn from(value: u16) -> Self {
Self::Index(value.into())
match NonMaxU32::new(value.into()) {
Some(index) => Self::Index(index),
// `u16` should always be valid for `NonMaxU32`
None => unreachable!(),
}
}
}

impl From<u32> for PropertyKey {
fn from(value: u32) -> Self {
Self::Index(value)
match NonMaxU32::new(value) {
Some(num) => Self::Index(num),
None => Self::String(value.to_string().into()),
}
}
}

impl From<usize> for PropertyKey {
fn from(value: usize) -> Self {
if let Ok(index) = u32::try_from(value) {
if let Ok(index) = u32::try_from(value)
.map_err(TryFromIntError::from)
.and_then(NonMaxU32::try_from)
{
Self::Index(index)
} else {
Self::String(JsString::from(value.to_string()))
@@ -615,7 +630,10 @@ impl From<usize> for PropertyKey {

impl From<i64> for PropertyKey {
fn from(value: i64) -> Self {
if let Ok(index) = u32::try_from(value) {
if let Ok(index) = u32::try_from(value)
.map_err(TryFromIntError::from)
.and_then(NonMaxU32::try_from)
{
Self::Index(index)
} else {
Self::String(JsString::from(value.to_string()))
@@ -625,7 +643,10 @@ impl From<i64> for PropertyKey {

impl From<u64> for PropertyKey {
fn from(value: u64) -> Self {
if let Ok(index) = u32::try_from(value) {
if let Ok(index) = u32::try_from(value)
.map_err(TryFromIntError::from)
.and_then(NonMaxU32::try_from)
{
Self::Index(index)
} else {
Self::String(JsString::from(value.to_string()))
@@ -635,7 +656,10 @@ impl From<u64> for PropertyKey {

impl From<isize> for PropertyKey {
fn from(value: isize) -> Self {
if let Ok(index) = u32::try_from(value) {
if let Ok(index) = u32::try_from(value)
.map_err(TryFromIntError::from)
.and_then(NonMaxU32::try_from)
{
Self::Index(index)
} else {
Self::String(JsString::from(value.to_string()))
@@ -645,7 +669,10 @@ impl From<isize> for PropertyKey {

impl From<i32> for PropertyKey {
fn from(value: i32) -> Self {
if let Ok(index) = u32::try_from(value) {
if let Ok(index) = u32::try_from(value)
.map_err(TryFromIntError::from)
.and_then(NonMaxU32::try_from)
{
Self::Index(index)
} else {
Self::String(JsString::from(value.to_string()))
@@ -656,7 +683,7 @@ impl From<i32> for PropertyKey {
impl From<f64> for PropertyKey {
fn from(value: f64) -> Self {
use num_traits::cast::FromPrimitive;
if let Some(index) = u32::from_f64(value) {
if let Some(index) = u32::from_f64(value).and_then(|n| NonMaxU32::try_from(n).ok()) {
return Self::Index(index);
}

0 comments on commit f18bf35

Please sign in to comment.