TryFromJs from JsMap for HashMap & BtreeMap

[Nikita-str]

Seems like TryFromJs didn't convert correctly from JsMap into HashMap / BtreeMap

This PR fix it

Example
Next code

use boa_engine::{value::TryFromJs, Context, JsResult, Source};
use std::collections::HashMap;

fn main() -> JsResult<()> {
    let js_code = "new Map([['a', 2], ['b', 3]])";
    let mut context = Context::default();

    let js_value = context.eval(Source::from_bytes(js_code))?;
    println!("{}", js_value.display());

    let map = HashMap::<String, i32>::try_from_js(&js_value, &mut context)?;
    println!("{map:?}");
    Ok(())
}

before this changes will return:

Map { "a" → 2, "b" → 3 }
{}

and after changes returns:

Map { "a" → 2, "b" → 3 }
{"a": 2, "b": 3}

Notes
I don't find more fast way(than the one used in for_each_elem_in_js_map) to get (key, value)'s from JsMap.
Maybe there is a way?
Or maybe I should write it (for example by function like next but that just return Option<(JsValue, JsValue)>)?

[jedel1043]

Thank you! I have a small suggestion to simplify things.

[jedel1043]

Here I think you can use IteratorResult to do all the checks for you. We'll probably want to make next return that instead of the raw JsValue, but constructing it manually should be fine for now.

[jedel1043]

On second thought, this looks like something that should replace JsMap::for_each, right? I think it would be a lot more ergonomic instead of our current API.

[Nikita-str]

~Yes, but I actually not sure how to do it via JsMp::for_each because you should to create an JsFunction and how I can see it can be done via NativeFunctionPointer but you can't capture in such function outer map Rust object (HashMap/BTreeMap). And if you use closure that will capture the map, then it seems like you can't create JsFunction.

Actually we don't want to create extra js objects here (like JsFunction), and therefor maybe it makes sense to create an function in JsMap that will do for_each but for Rust fn (for F: FnMut), and maybe make it public only in the crate.

[jedel1043]

Yes, I was more talking about substituting JsMap::for_each with this new function, since it seems that passing JsFunction directly is a lot more painful overall.

[Nikita-str]

Done

Is riust_for_each acceptable name?

Just for sureness, is this equivalent to the next:

let map = map
    .downcast_ref::<OrderedMap<JsValue>>()
    .expect("checked that `this` was a map");

map.iter().try_for_each(|(k, v)|{
    let args = [v.clone(), k.clone(), this.clone()];
    callback.call(this_arg, &args, context).map(|_|())
})?;
Ok(JsValue::undefined())

If they are not the same, then seems like I incorrectly impl the fn rust_for each

If they the same, maybe simplify it?

[jedel1043]

Not really sure, but I think we have pretty good coverage. You can try replacing it to see if there are any regressions on the test262 suite.

[Nikita-str]

No, they are not the same because JsMap can be changed in for_each and it will panic because of mut and non-mut borrowing at the same time

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 52.83%. Comparing base (6ddc2b4) to head (b74a521).
Report is 264 commits behind head on main.

Files with missing lines	Patch %	Lines
core/engine/src/builtins/map/mod.rs	58.82%	7 Missing ⚠️
...e/src/value/conversions/try_from_js/collections.rs	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3998      +/-   ##
==========================================
+ Coverage   47.24%   52.83%   +5.58%     
==========================================
  Files         476      480       +4     
  Lines       46892    46709     -183     
==========================================
+ Hits        22154    24677    +2523     
+ Misses      24738    22032    -2706     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.