Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor builtin Map intrinsics to follow more closely the spec #1572

Merged
merged 3 commits into from
Sep 12, 2021
Merged

Refactor builtin Map intrinsics to follow more closely the spec #1572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52cef22
Refactor builtin `Map` object and document methods
jedel1043 Sep 11, 2021
0306648
Replace some calls to `is_function` with `is_callable`
jedel1043 Sep 11, 2021
cd1cb06
Remove `expect_map` methods and cleanup some `expect`s
jedel1043 Sep 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 10 additions & 13 deletions boa/src/builtins/array/array_iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ impl ArrayIterator {
///
/// [spec]: https://tc39.es/ecma262/#sec-createarrayiterator
pub(crate) fn create_array_iterator(
context: &Context,
array: JsValue,
kind: PropertyNameKind,
context: &Context,
) -> JsValue {
let array_iterator = JsValue::new_object(context);
array_iterator.set_data(ObjectData::array_iterator(Self::new(array, kind)));
Expand All @@ -68,9 +68,9 @@ impl ArrayIterator {
let index = array_iterator.next_index;
if array_iterator.array.is_undefined() {
return Ok(create_iter_result_object(
context,
JsValue::undefined(),
true,
context,
));
}
let len = array_iterator
Expand All @@ -82,33 +82,30 @@ impl ArrayIterator {
if array_iterator.next_index >= len {
array_iterator.array = JsValue::undefined();
return Ok(create_iter_result_object(
context,
JsValue::undefined(),
true,
context,
));
}
array_iterator.next_index = index + 1;
match array_iterator.kind {
return match array_iterator.kind {
PropertyNameKind::Key => {
Ok(create_iter_result_object(context, index.into(), false))
Ok(create_iter_result_object(index.into(), false, context))
}
PropertyNameKind::Value => {
let element_value = array_iterator.array.get_field(index, context)?;
Ok(create_iter_result_object(context, element_value, false))
Ok(create_iter_result_object(element_value, false, context))
}
PropertyNameKind::KeyAndValue => {
let element_value = array_iterator.array.get_field(index, context)?;
let result =
Array::create_array_from_list([index.into(), element_value], context);
Ok(create_iter_result_object(context, result.into(), false))
Ok(create_iter_result_object(result.into(), false, context))
}
}
} else {
context.throw_type_error("`this` is not an ArrayIterator")
};
}
} else {
context.throw_type_error("`this` is not an ArrayIterator")
}
context.throw_type_error("`this` is not an ArrayIterator")
}

/// Create the %ArrayIteratorPrototype% object
Expand All @@ -117,7 +114,7 @@ impl ArrayIterator {
/// - [ECMA reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-%arrayiteratorprototype%-object
pub(crate) fn create_prototype(context: &mut Context, iterator_prototype: JsValue) -> JsObject {
pub(crate) fn create_prototype(iterator_prototype: JsValue, context: &mut Context) -> JsObject {
let _timer = BoaProfiler::global().start_event(Self::NAME, "init");

// Create prototype
Expand Down
6 changes: 3 additions & 3 deletions boa/src/builtins/array/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2528,9 +2528,9 @@ impl Array {
context: &mut Context,
) -> JsResult<JsValue> {
Ok(ArrayIterator::create_array_iterator(
context,
this.clone(),
PropertyNameKind::Value,
context,
))
}

Expand All @@ -2546,9 +2546,9 @@ impl Array {
/// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/values
pub(crate) fn keys(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
Ok(ArrayIterator::create_array_iterator(
context,
this.clone(),
PropertyNameKind::Key,
context,
))
}

Expand All @@ -2568,9 +2568,9 @@ impl Array {
context: &mut Context,
) -> JsResult<JsValue> {
Ok(ArrayIterator::create_array_iterator(
context,
this.clone(),
PropertyNameKind::KeyAndValue,
context,
))
}

Expand Down
40 changes: 13 additions & 27 deletions boa/src/builtins/iterable/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,22 @@ impl IteratorPrototypes {
let iterator_prototype = create_iterator_prototype(context);
Self {
array_iterator: ArrayIterator::create_prototype(
context,
iterator_prototype.clone().into(),
context,
),
set_iterator: SetIterator::create_prototype(context, iterator_prototype.clone().into()),
set_iterator: SetIterator::create_prototype(iterator_prototype.clone().into(), context),
string_iterator: StringIterator::create_prototype(
context,
iterator_prototype.clone().into(),
context,
),
regexp_string_iterator: RegExpStringIterator::create_prototype(
context,
iterator_prototype.clone().into(),
context,
),
map_iterator: MapIterator::create_prototype(context, iterator_prototype.clone().into()),
map_iterator: MapIterator::create_prototype(iterator_prototype.clone().into(), context),
for_in_iterator: ForInIterator::create_prototype(
context,
iterator_prototype.clone().into(),
context,
),
iterator_prototype,
}
Expand Down Expand Up @@ -85,7 +85,7 @@ impl IteratorPrototypes {
/// CreateIterResultObject( value, done )
///
/// Generates an object supporting the IteratorResult interface.
pub fn create_iter_result_object(context: &mut Context, value: JsValue, done: bool) -> JsValue {
pub fn create_iter_result_object(value: JsValue, done: bool, context: &mut Context) -> JsValue {
// 1. Assert: Type(done) is Boolean.
// 2. Let obj be ! OrdinaryObjectCreate(%Object.prototype%).
let obj = context.construct_object();
Expand All @@ -101,12 +101,12 @@ pub fn create_iter_result_object(context: &mut Context, value: JsValue, done: bo
}

/// Get an iterator record
pub fn get_iterator(context: &mut Context, iterable: JsValue) -> JsResult<IteratorRecord> {
pub fn get_iterator(iterable: &JsValue, context: &mut Context) -> JsResult<IteratorRecord> {
let iterator_function = iterable.get_field(WellKnownSymbols::iterator(), context)?;
if iterator_function.is_null_or_undefined() {
return Err(context.construct_type_error("Not an iterable"));
}
let iterator_object = context.call(&iterator_function, &iterable, &[])?;
let iterator_object = context.call(&iterator_function, iterable, &[])?;
let next_function = iterator_object.get_field("next", context)?;
if next_function.is_null_or_undefined() {
return Err(context.construct_type_error("Could not find property `next`"));
Expand Down Expand Up @@ -158,8 +158,8 @@ impl IteratorRecord {
let next = context.call(&self.next_function, &self.iterator_object, &[])?;
let done = next.get_field("done", context)?.to_boolean();

let next_result = next.get_field("value", context)?;
Ok(IteratorResult::new(next_result, done))
let value = next.get_field("value", context)?;
Ok(IteratorResult { value, done })
}

/// Cleanup the iterator
Expand Down Expand Up @@ -203,20 +203,6 @@ impl IteratorRecord {

#[derive(Debug)]
pub struct IteratorResult {
value: JsValue,
done: bool,
}

impl IteratorResult {
fn new(value: JsValue, done: bool) -> Self {
Self { value, done }
}

pub fn is_done(&self) -> bool {
self.done
}

pub fn value(self) -> JsValue {
self.value
}
pub value: JsValue,
pub done: bool,
Comment on lines +206 to +207
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I don't like. Is this needed? Using public members makes this structure modifiable almost anywhere and with no control, and makes it much more difficult if we ever want to change the inner structure.

The getter and setter patterns are very well established in Rust for a reason. They are less error prone and easier to maintain external compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using getters and setters is justifiable if we had to maintain some state internally. This struct is just a wrapper for a (Value, bool) to represent an iterator item, I don't see the utility on having only getters if the struct will have just is_done and get_value as methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are right that there is no functionality with this struct, I think the private fields preserve the nature of the struct. It should only be used to access the result of an iterator next call. If the fields are public, the struct is really only a wrapper for (JsValue, bool). In that case we could also just work with that tuple directly instead of using a struct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could return a tuple, but using an struct makes more explicit what each type represents. That's the same reason why it's more idiomatic to represent a 2d point as a Point {x: f64, y: f64} instead of a (f64, f64).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, if it's not meant to be modified, we don't even need the setters, and not having them would prevent accidental mutation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is meant to be modified. Or at least is meant to be destructured and used as an owned value, like in:

boa/boa/src/syntax/ast/node/array/mod.rs

Lines 54 to 60 in 2f8c35d

let next = iterator_record.next(context)?;
if next.is_done() {
break;
}
let next_value = next.value();
//next_index += 1;
elements.push(next_value.clone());

or in

boa/boa/src/syntax/ast/node/new/mod.rs

Lines 61 to 66 in 2f8c35d

let next = iterator_record.next(context)?;
if next.is_done() {
break;
}
let next_value = next.value();
v_args.push(next_value.clone());

And no function takes an IteratorResult as a parameter, so we don't need to ensure that IteratorResult is not modified. If we used IteratorResult as a parameter on any function, I would completely agree with using getters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about this, is there a reason that IteratorResult is pub instead of pub(crate) at all? We neither return nor accept this anywhere in the public api.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about this, is there a reason that IteratorResult is pub instead of pub(crate) at all? We neither return nor accept this anywhere in the public api.

Yeah, I just copied the current visibility, but maybe we shouldn't expose it to the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API is not very well defined yet, and we will probably need to change many visibilities. I'm OK with these changes.

}
156 changes: 68 additions & 88 deletions boa/src/builtins/map/map_iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
};
use gc::{Finalize, Trace};

use super::{ordered_map::MapLock, Map};
use super::ordered_map::MapLock;
/// The Map Iterator object represents an iteration over a map. It implements the iterator protocol.
///
/// More information:
Expand All @@ -16,7 +16,7 @@ use super::{ordered_map::MapLock, Map};
/// [spec]: https://tc39.es/ecma262/#sec-array-iterator-objects
#[derive(Debug, Clone, Finalize, Trace)]
pub struct MapIterator {
iterated_map: JsValue,
iterated_map: Option<JsObject>,
map_next_index: usize,
map_iteration_kind: PropertyNameKind,
lock: MapLock,
Expand All @@ -25,17 +25,6 @@ pub struct MapIterator {
impl MapIterator {
pub(crate) const NAME: &'static str = "MapIterator";

/// Constructs a new `MapIterator`, that will iterate over `map`, starting at index 0
fn new(map: JsValue, kind: PropertyNameKind, context: &mut Context) -> JsResult<Self> {
let lock = Map::lock(&map, context)?;
Ok(MapIterator {
iterated_map: map,
map_next_index: 0,
map_iteration_kind: kind,
lock,
})
}

/// Abstract operation CreateMapIterator( map, kind )
///
/// Creates a new iterator over the given map.
Expand All @@ -45,17 +34,29 @@ impl MapIterator {
///
/// [spec]: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-createmapiterator
pub(crate) fn create_map_iterator(
context: &mut Context,
map: JsValue,
map: &JsValue,
kind: PropertyNameKind,
context: &mut Context,
) -> JsResult<JsValue> {
let map_iterator = JsValue::new_object(context);
map_iterator.set_data(ObjectData::map_iterator(Self::new(map, kind, context)?));
map_iterator
.as_object()
.expect("map iterator object")
.set_prototype_instance(context.iterator_prototypes().map_iterator().into());
Ok(map_iterator)
if let Some(map_obj) = map.as_object() {
if let Some(map) = map_obj.borrow_mut().as_map_mut() {
let lock = map.lock(map_obj.clone());
let iter = MapIterator {
iterated_map: Some(map_obj.clone()),
map_next_index: 0,
map_iteration_kind: kind,
lock,
};
let map_iterator = JsValue::new_object(context);
map_iterator.set_data(ObjectData::map_iterator(iter));
map_iterator
.as_object()
.expect("map iterator object")
.set_prototype_instance(context.iterator_prototypes().map_iterator().into());
return Ok(map_iterator);
}
}
context.throw_type_error("`this` is not a Map")
}

/// %MapIteratorPrototype%.next( )
Expand All @@ -67,77 +68,56 @@ impl MapIterator {
///
/// [spec]: https://tc39.es/ecma262/#sec-%mapiteratorprototype%.next
pub(crate) fn next(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
jedel1043 marked this conversation as resolved.
Show resolved Hide resolved
if let JsValue::Object(ref object) = this {
let mut object = object.borrow_mut();
if let Some(map_iterator) = object.as_map_iterator_mut() {
let m = &map_iterator.iterated_map;
let mut index = map_iterator.map_next_index;
let item_kind = &map_iterator.map_iteration_kind;
let iterator_object = match this {
JsValue::Object(obj) if obj.borrow().is_map_iterator() => obj,
_ => return context.throw_type_error("`this` is not a MapIterator"),
};

if map_iterator.iterated_map.is_undefined() {
return Ok(create_iter_result_object(
context,
JsValue::undefined(),
true,
));
}
let mut iterator_object = iterator_object.borrow_mut();

let map_iterator = iterator_object
.as_map_iterator_mut()
.expect("checked that obj was a map iterator");

if let JsValue::Object(ref object) = m {
if let Some(entries) = object.borrow().as_map_ref() {
let num_entries = entries.full_len();
while index < num_entries {
let e = entries.get_index(index);
index += 1;
map_iterator.map_next_index = index;
if let Some((key, value)) = e {
match item_kind {
PropertyNameKind::Key => {
return Ok(create_iter_result_object(
context,
key.clone(),
false,
));
}
PropertyNameKind::Value => {
return Ok(create_iter_result_object(
context,
value.clone(),
false,
));
}
PropertyNameKind::KeyAndValue => {
let result = Array::create_array_from_list(
[key.clone(), value.clone()],
context,
);
return Ok(create_iter_result_object(
context,
result.into(),
false,
));
}
}
}
let mut index = map_iterator.map_next_index;
let item_kind = map_iterator.map_iteration_kind;

if let Some(obj) = map_iterator.iterated_map.take() {
let map = obj.borrow();
let entries = map.as_map_ref().expect("iterator should only iterate maps");
let num_entries = entries.full_len();
while index < num_entries {
let e = entries.get_index(index);
index += 1;
map_iterator.map_next_index = index;
if let Some((key, value)) = e {
let item = match item_kind {
PropertyNameKind::Key => {
Ok(create_iter_result_object(key.clone(), false, context))
}
PropertyNameKind::Value => {
Ok(create_iter_result_object(value.clone(), false, context))
}
} else {
return Err(context.construct_type_error("'this' is not a Map"));
}
} else {
return Err(context.construct_type_error("'this' is not a Map"));
PropertyNameKind::KeyAndValue => {
let result = Array::create_array_from_list(
[key.clone(), value.clone()],
context,
);
Ok(create_iter_result_object(result.into(), false, context))
}
};
drop(map);
map_iterator.iterated_map = Some(obj);
return item;
}

map_iterator.iterated_map = JsValue::undefined();
Ok(create_iter_result_object(
context,
JsValue::undefined(),
true,
))
} else {
context.throw_type_error("`this` is not an MapIterator")
}
} else {
context.throw_type_error("`this` is not an MapIterator")
}

Ok(create_iter_result_object(
JsValue::undefined(),
true,
context,
))
}

/// Create the %MapIteratorPrototype% object
Expand All @@ -146,7 +126,7 @@ impl MapIterator {
/// - [ECMA reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-%mapiteratorprototype%-object
pub(crate) fn create_prototype(context: &mut Context, iterator_prototype: JsValue) -> JsObject {
pub(crate) fn create_prototype(iterator_prototype: JsValue, context: &mut Context) -> JsObject {
let _timer = BoaProfiler::global().start_event(Self::NAME, "init");

// Create prototype
Expand Down