Initial implementation of Map()

[joshwd36]

This Pull Request closes #401.

It changes the following:

• Adds the Map object, backed by a HashMap.
• Adds most of the associated methods.
• Adds a display implementation for the map.

There are a few things that currently aren't implemented:

• Methods that return an iterator.
• The map is not ordered by insertion. Currently this only affects the display output and the forEach method.
• The Map() constructor can be called as a function to construct a map, rather than throwing an exception.
• The data is stored as a HashMap in ObjectData. This makes cloning ObjectData quite expensive now, and also means actions such as adding data clone the HashMap. It should probably be placed in a wrapper containing an Rc<RefCell<_>>.
• Adding the map to itself seems to work, but the display implementation I have overflows the stack when called.

[codecov]

Codecov Report

Merging #550 into master will increase coverage by 0.49%.
The diff coverage is 83.80%.

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   69.87%   70.37%   +0.49%     
==========================================
  Files         172      175       +3     
  Lines       10811    11145     +334     
==========================================
+ Hits         7554     7843     +289     
- Misses       3257     3302      +45     

Impacted Files	Coverage Δ
boa/src/builtins/array/mod.rs	80.97% <ø> (ø)
boa/src/builtins/bigint/mod.rs	69.49% <ø> (ø)
boa/src/builtins/boolean/mod.rs	34.48% <ø> (ø)
boa/src/builtins/error/mod.rs	33.33% <ø> (ø)
boa/src/builtins/error/range.rs	57.14% <ø> (ø)
boa/src/builtins/error/reference.rs	76.19% <ø> (ø)
boa/src/builtins/error/syntax.rs	33.33% <ø> (ø)
boa/src/builtins/error/type.rs	76.19% <ø> (ø)
boa/src/builtins/mod.rs	20.68% <0.00%> (-0.74%)	⬇️
boa/src/builtins/number/mod.rs	67.28% <ø> (ø)
... and 18 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11e85eb...213747b. Read the comment docs.

[joshwd36]

I'm not entirely sure what to do about the mutable key types lint. I think it should be OK because the values are cloned when they're placed in the HashMap and then again when they're removed, so it shouldn't be possible to modify that particular copy, but I'm probably missing something

[joshwd36]

I'm not entirely sure what to do about the mutable key types lint. I think it should be OK because the values are cloned when they're placed in the HashMap and then again when they're removed, so it shouldn't be possible to modify that particular copy, but I'm probably missing something

I've just done some additional testing and it seems the current implementation works. I tried creating an object and using it as the key in a map, then modifying the object and using it to get from the map, and it still got the value.

[HalidOdat]

The map is not ordered by insertion. Currently this only affects the display output and the forEach method.

About this I don't think HashMap is sufficient we need something that keeps it order of insertion and can be indexed like an array, I think we need the indexmap crate it provides the IndexMap it seems perfect for the job and it also provides a IndexSet to which is needed for Set #400

[HalidOdat]

The data is stored as a HashMap in ObjectData. This makes cloning ObjectData quite expensive now, and also means actions such as adding data clone the HashMap. It should probably be placed in a wrapper containing an Rc<RefCell<_>>.

when we clone a GcObject which Value has (Value::Object(GcObject)), It does not do a deep clone of the object, only a reference to the object is cloned, in a way it is like an Rc

The way we extracting the data is a bit awkward though.

[joshwd36]

The map is not ordered by insertion. Currently this only affects the display output and the forEach method.

About this I don't think HashMap is sufficient we need something that keeps it order of insertion and can be indexed like an array, I think we need the indexmap crate it provides the IndexMap it seems perfect for the job and it also provides a IndexSet to which is needed for Set #400

That seems like it could work well. Unfortunately, there are a few places that might be a bit inefficient, but it would at least be correct. In order to maintain insertion order with removals, we'd have to use the shift_remove function, which is O(n). Likewise, when inserting a duplicate key the original would have to be removed so that the new one is placed at the back, rather than swapped in. For now, though, this would be good, and I'll work on swapping over.

The data is stored as a HashMap in ObjectData. This makes cloning ObjectData quite expensive now, and also means actions such as adding data clone the HashMap. It should probably be placed in a wrapper containing an Rc<RefCell<_>>.

when we clone a GcObject which Value has (Value::Object(GcObject)), It does not do a deep clone of the object, only a reference to the object is cloned, in a way it is like an Rc

The way we extracting the data is a bit awkward though.
Ah, I missed how the GcObject works, that's good to hear. Just out of interest, I notice that other objects such as BigInt and String use Rc wrapper types. Is there a reason for this?

But yes, modifying the data does feel a bit awkward, as updating the data requires cloning the hashmap and replacing the original. Using a RefCell could work, as could instance methods taking a mutable reference, although that might be quite a large architectural change.

[HalidOdat]

Unfortunately, there are a few places that might be a bit inefficient, but it would at least be correct. In order to maintain insertion order with removals, we'd have to use the shift_remove function, which is O(n).

Sadly yes. but that's what the spec requires.

likewise, when inserting a duplicate key the original would have to be removed so that the new one is placed at the back, rather than swapped in. For now, though, this would be good, and I'll work on swapping over.

for insertion it replaces or appends if there isn't a duplicate key, right? https://tc39.es/ecma262/#sec-map.prototype.set step 4.a.i Set p.[[Value]] to value.

For now, though, this would be good, and I'll work on swapping over.

Yes. for now we can use HashMap

Ah, I missed how the GcObject works, that's good to hear. Just out of interest, I notice that other objects such as BigInt and String use Rc wrapper types. Is there a reason for this?

It's because the Value primitives Value::String, Value::BigInt, Value::Symbol contain an Rc to make the Value trivially copyable so we don't have to clone a String or BigInt every time we clone since they are immutable, this also shrank the size of Value from 40 bytes to 24 bytes, and it to also shrink the size of ObjectData and so we don't have to clone the String when we convert a string primitive to a string object. see #465 and #498 for more information.

[HalidOdat]

But yes, modifying the data does feel a bit awkward, as updating the data requires cloning the hashmap and replacing the original. Using a RefCell could work, as could instance methods taking a mutable reference, although that might be quite a large architectural change.

Yes the way we do it is far from ideal.

Edit: We can use Rc<RefCell> and in the future when we refactor Object we can change it :)

[joshwd36]

likewise, when inserting a duplicate key the original would have to be removed so that the new one is placed at the back, rather than swapped in. For now, though, this would be good, and I'll work on swapping over.

for insertion it replaces or appends if there isn't a duplicate key, right? https://tc39.es/ecma262/#sec-map.prototype.set step 4.a.i Set p.[[Value]] to value.

Yes, sorry, you're completely right. That does simplify things then.

But yes, modifying the data does feel a bit awkward, as updating the data requires cloning the hashmap and replacing the original. Using a RefCell could work, as could instance methods taking a mutable reference, although that might be quite a large architectural change.

Yes the way we do it is far from ideal.

Edit: We can use Rc<RefCell> and in the future when we refactor Object we can change it :)

I don't think RefCell would work because Trace isn't implemented for it. GcCell works though. I also don't think Rc should be necessary, as there should only ever be one owner of the data.

[HalidOdat]

The Map() constructor can be called as a function to construct a map, rather than throwing an exception.

For this we need to set this to false for Map function: https://github.com/boa-dev/boa/blob/master/boa/src/builtins/function/mod.rs#L109

Maybe we should make https://github.com/boa-dev/boa/blob/master/boa/src/builtins/function/mod.rs#L416-L467 accept another parameter or an enum with field Constructable Callable ConstructableAndEnumerable or maybe a bitfield (we already use the bitfields crate in other places) and do CONSTRUCTABLE | CALLABLE and set depending on the argument the callable and constructable fields respectively like we do with constructable https://github.com/boa-dev/boa/blob/master/boa/src/builtins/function/mod.rs#L429

I think the bitfield would be much nicer and it would shrink the size of Function struct too.

[HalidOdat]

Methods that return an iterator.

About this, we don't have iterator anywhere, also we don't support well-known symbols (Symbol.iterator)

If you want we can leave this to be a separate PR, implementing MapIterator object, if you want :)

[joshwd36]

Methods that return an iterator.

About this, we don't have iterator anywhere, also we don't support well-known symbols (Symbol.iterator)

If you want we can leave this to be a separate PR, implementing MapIterator object, if you want :)

That's probably best, it would probably pollute the PR quite a lot, and I'm not sure whether I'd be able to implement it, although I'll have a go

[Razican]

I see many changes in the diff not directly related to this PR, why is that? Could we rebase?

[joshwd36]

I see many changes in the diff not directly related to this PR, why is that? Could we rebase?

I think this is due to how github is displaying the diffs, as described here. I changed the base branch away and back as described and it seems to be better now.

[HalidOdat]

Looks good to me! :)

[Razican]

Looks good to me. Should we add some benchmarks?