[[HostDefined]] Refactor

[johnyob]

Context

Currently HostDefined doesn't permit you to mutably borrow two objects from the [[HostDefined]] field since the FxHashMap is wrapped under a GcRefCell.

Description

This PR instead wraps all Box<dyn NativeObject> entries in the FxHashMap with a GcRefCell, permitting a more granular borrowing scheme for each entry.

Additionally, this PR takes the opportunity to provide automatic downcasting on the insert and remove methods for HostDefined.

[codecov]

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (fee4048) 45.40% compared to head (72e5a2d) 44.82%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3370      +/-   ##
==========================================
- Coverage   45.40%   44.82%   -0.59%     
==========================================
  Files         483      488       +5     
  Lines       49790    50499     +709     
==========================================
+ Hits        22609    22637      +28     
- Misses      27181    27862     +681     

Files	Coverage Δ
boa_engine/src/realm.rs	51.66% <0.00%> (ø)
boa_examples/src/bin/host_defined.rs	0.00% <0.00%> (ø)
boa_engine/src/host_defined.rs	0.00% <0.00%> (ø)

... and 33 files with indirect coverage changes

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

[jedel1043]

Downcasting on insert and delete looks great! However, I'm having second thoughts about having that amount of granularity on the API.

My problem comes from the thought that we'd be worsening the API for users that just want to store one or two types.

On the other hand, if a user wants to mutably borrow two separate types from the map, they can just put both in a wrapper type, then insert that instead, which should allow the same use case. Another workaround would be to just borrow the map multiple times. That should be a lot easier to handle than having to deal with 2 levels of borrowing
(borrowed map + borrowed type, borrowed map + mutably borrowed type, mutably borrowed map + borrowed type, mutably borrowed map + mutably borrowed type)

[johnyob]

My problem comes from the thought that we'd be worsening the API for users that just want to store one or two types.

On the other hand, if a user wants to mutably borrow two separate types from the map, they can just put both in a wrapper type, then insert that instead, which should allow the same use case. Another workaround would be to just borrow the map multiple times. That should be a lot easier to handle than having to deal with 2 levels of borrowing
(borrowed map + borrowed type, borrowed map + mutably borrowed type, mutably borrowed map + borrowed type, mutably borrowed map + mutably borrowed type)

The primary pain point (for us) with the current implementation is that we cannot mutably borrow two host-defined objects (defined by separate extensions) in a single function.

The possible pain points with the proposed implementation are:

1. Lifetimes of all host-defined objects are bounded by the lifetime of the GcRef/GcRefMut you obtain by borrowing the host_defined map.

   In the context of writing a runtime, we've not encountered many issues with this. The solution to this is usually to .borrow() the host_defined map at the top of your native function. However, you do need to carefully scope the borrowed reference, or explicitly drop the reference (as shown in the example) when interleaving between Rust code and JavaScript code if the reference is mutable.

2. The 2 levels of borrowing.

   I agree that this definitely comes with a mental overhead wrt to the borrowing semantics. But it can often result in clearer code when the semantics of borrowing are more explicit.

   Another workaround would be to just borrow the map multiple times

   This isn't possible if you want mutable references to two (or more) entries in the host-defined map.

I think with perhaps clearer documentation on the borrowing semantics (something that was still lacking with the current implementation wrt to borrowing 2 or more mutable entries in host-defined), this change could still be seen as an improvement over the current API.

[jedel1043]

This isn't possible if you want mutable references to two (or more) entries in the host-defined map.

Yeah, and I'd argue this is a design problem, not an API problem. This would still happen if this was a simple HashMap without any relation to Boa itself. Usually, this is resolved by shortening the lifetimes of the references and ensuring only one reference is used at once.

Leaving that aside, I think a better solution to this would be to expose the get_many_mut method, which should allow this without having to deal with the complexity of multiple RefCells. This will require using hashbrown::HashMap instead of std::HashMap though, but we already have the dependency on our tree thanks to WeakMap, so no problems from pulling it into boa_engine itself.

[johnyob]

(Prefixing my response: I'm still fairly new to Rust so please forgive my ignorance to existing libraries, common design patterns, etc)

This would still happen if this was a simple HashMap without any relation to Boa itself

Would it? In Rust, what I'd expect is to be given a &'a mut HashMap for which I can get the mutable references for two (or more) entires, each of which has a lifetime 'ai bounded by 'a. This is what this the approach that this PR provides (but obviously with some foot guns due to the use of GcRefCells)

Leaving that aside, I think a better solution to this would be to expose the get_many_mut method, which should allow this without having to deal with the complexity of multiple RefCells. This will require using hashbrown::HashMap instead of std::HashMap though, but we already have the dependency on our tree thanks to WeakMap, so no problems from pulling it into boa_engine itself.

But is this possible? As I understand currently, the interface we expose with HostDefined is one of a dependently-typed map, where the type of the entry depends on the type of the key. From what I can deduce from this get_many_mut method, we're not able to provide the same interface since we'd have to simply pass an array of TypeIds in an recieve an array of NativeObjects with the user doing the necessary downcasting.

[jedel1043]

Would it? In Rust, what I'd expect is to be given a &'a mut HashMap for which I can get the mutable references for two (or more) entires, each of which has a lifetime 'ai bounded by 'a. This is what this the approach that this PR provides (but obviously with some foot guns due to the use of GcRefCells)

Rust doesn't allow borrowing two mutable entries to the hashmap at the same time, even if the keys aren't equal.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=958b5889ab29439c6be993473c342b49

But is this possible? As I understand currently, the interface we expose with HostDefined is one of a dependently-typed map, where the type of the entry depends on the type of the key. From what I can deduce from this get_many_mut method, we're not able to provide the same interface since we'd have to simply pass an array of TypeIds in an recieve an array of NativeObjects with the user doing the necessary downcasting.

I think this is solvable with a NativeTuple trait that has a as_typeid_array method that returns the type ids of each one of its members. We can use that to query using a tuple instead, which would result in an API such as:

use std::any::TypeId;

trait NativeTuple: Sealed {
    type BorrowMutTuple<'a>;
    fn as_type_ids() -> Vec<TypeId>; 
}

// Impls for tuples of size 1 to N

impl HostDefined {
    // More methods
    
    fn get_many_mut<Tup: NativeTuple>(&mut self) -> Tup::BorrowMutTuple<'_> {
        // Impl
    }
}

Or something like that, which I think it's pretty feasible. I'll try to come up with a prototype today.

[johnyob]

Rust doesn't allow borrowing two mutable entries to the hashmap at the same time, even if the keys aren't equal.

Thanks for letting me know, I forgot that HashMap doesn't support interior mutability an entires 😅 I've had too many GcRefCells in my life recently

I think this is solvable with a NativeTuple trait that has a as_typeid_array method that returns the type ids of each one of its members. We can use that to query using a tuple instead, which would result in an API such as:

Whoa, this is really neat. If you're strapped for time, I'd be happy to try and prototype it myself! (Though I may up asking a lot of stupid questions over discord)

[jedel1043]

Whoa, this is really neat. If you're strapped for time, I'd be happy to try and prototype it myself! (Though I may up asking a lot of stupid questions over discord)

Sure, you can give it a go! It'll be a bit hard because of limitations from rustc around const generics,
but I think I have the trickiest part solved:

pub fn get_many_mut<Tup, const SIZE: usize>(&mut self) -> Option<Tup::BorrowedMutTuple<'_>>
    where
        Tup: Tuple<SIZE>,
    {
        let ids = Tup::as_type_ids();
        let refs: [&TypeId; SIZE] = ids.iter().collect::<Vec<_>>().try_into().expect("tuple should be of size `SIZE`");
        self.types
            .get_many_mut(refs)
            .and_then(|anys| Tup::mut_refs_from_anys(anys))
    }

pub trait Tuple<const SIZE: usize> {
    type BorrowedMutTuple<'a>;

    fn as_type_ids() -> Vec<TypeId>;

    fn mut_refs_from_anys(
        anys: [&'_ mut Box<dyn NativeObject>; SIZE],
    ) -> Option<Self::BorrowedMutTuple<'_>>;
}

Ideally we'd have the SIZE parameter as an associated const, but that's not possible at the moment (rust-lang/rust#60551). We can use that workaround, then lift the limitation when the compiler supports it in the future.

[johnyob]

Closing in favour of #3460