Refactor builtin Map intrinsics to follow more closely the spec

[jedel1043]

Changes:

• Change OrderedMap to reset its empty_count on clear.
• Document many methods of the Map object.
• Refactor some patterns to be more idiomatic.
• Implement AddEntriesFromIterable.
• Modify the API of IteratorResult.
• Move Context to the end of all related Iterator methods.

[github-actions]

Test262 conformance changes:

Test result	master count	PR count	difference
Total	80,804	80,804	0
Passed	33,006	33,038	+32
Ignored	15,896	15,896	0
Failed	31,902	31,870	-32
Panics	0	0	0
Conformance	40.85%	40.89%	+0.04%

Fixed tests (32):

test/built-ins/Map/iterable-calls-set.js [strict mode] (previously Failed)
test/built-ins/Map/iterable-calls-set.js (previously Failed)
test/built-ins/Map/iterator-close-failure-after-set-failure.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-close-failure-after-set-failure.js (previously Failed)
test/built-ins/Map/iterator-item-first-entry-returns-abrupt.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-item-first-entry-returns-abrupt.js (previously Failed)
test/built-ins/Map/iterator-item-second-entry-returns-abrupt.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-item-second-entry-returns-abrupt.js (previously Failed)
test/built-ins/Map/iterator-next-failure.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-next-failure.js (previously Failed)
test/built-ins/Map/iterator-items-are-not-object-close-iterator.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-items-are-not-object-close-iterator.js (previously Failed)
test/built-ins/Map/map-iterable-throws-when-set-is-not-callable.js [strict mode] (previously Failed)
test/built-ins/Map/map-iterable-throws-when-set-is-not-callable.js (previously Failed)
test/built-ins/Map/get-set-method-failure.js [strict mode] (previously Failed)
test/built-ins/Map/get-set-method-failure.js (previously Failed)
test/built-ins/Map/iterator-value-failure.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-value-failure.js (previously Failed)
test/built-ins/Map/iterator-close-after-set-failure.js [strict mode] (previously Failed)
test/built-ins/Map/iterator-close-after-set-failure.js (previously Failed)
test/built-ins/Map/map-no-iterable.js [strict mode] (previously Failed)
test/built-ins/Map/map-no-iterable.js (previously Failed)
test/built-ins/Map/prototype/size/does-not-have-mapdata-internal-slot-set.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/size/does-not-have-mapdata-internal-slot-set.js (previously Failed)
test/built-ins/Map/prototype/size/length.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/size/length.js (previously Failed)
test/built-ins/Map/prototype/size/size.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/size/size.js (previously Failed)
test/built-ins/Map/prototype/size/does-not-have-mapdata-internal-slot.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/size/does-not-have-mapdata-internal-slot.js (previously Failed)
test/built-ins/Map/prototype/size/name.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/size/name.js (previously Failed)

[raskad]

Looks really nice.
I've got a few little things.

[Razican]

The conformance numbers look great, and the code seems good, but I'll need some convincing in some design decisions :)

[Razican]

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.

[jedel1043]

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.

[raskad]

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.

[jedel1043]

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

[Razican]

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

[jedel1043]

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.

[raskad]

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.

[jedel1043]

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.

[Razican]

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

[RageKnify]

LGTM, only exception is having the extra methods in Object like @Razican said. In my opinion it doesn't abstract that much code away and is one more thing people need to know about.

[jedel1043]

LGTM, only exception is having the extra methods in Object like @Razican said. In my opinion it doesn't abstract that much code away and is one more thing people need to know about.

Ah, I forgot to push my last changes 😅