Enable passing String and custom struct boxed slices across ABI and implement TryFrom<JsValue> for custom structs

[sinking-point]

Closes:

#111

#2452

#168

#2231

The vectors are converted across the ABI by converting their elements to/from JsValues.

It was necessary to change the way relevant traits (e.g. WasmDescribe, IntoWasmAbi etc.) are implemented. It's impossible to implement these for Box<[#name]> in codegen.rs because implementing traits on generic types is only allowed in the crate in which the trait is defined. Naively adding a blanket implementation on the wasm-bindgen side doesn't work either because it conflicts with the implementation for JsObjects. The solution was to create traits like VectorIntoWasmAbi etc. that are implemented on the concrete type and contain the implementation for IntoWasmAbi etc. for vectors of that type. JsObjects are blanket implemented as before, and struct types are implemented in codegen.rs. Due to the way these traits are defined, Rust requires implementing types to be Sized, so they can't be used for the String implementations.

Converting structs from JsValues was accomplished by adding an unwrap function to the generated JavaScript class, and calling that from Rust.

[alexcrichton]

Thanks for the PR, but judging from the size here, the description, and the title, this seems like a significant change. Can you describe the change in some more detail at a high level and explain what's in the patch internally?

Additionally I think it'd probably be good to ensure CI is green.

[sinking-point]

@alexcrichton No worries Alex, and thank you for taking a look.

My motivation for writing this PR was frustration with wasm-bindgen's limited ability to pass vector types (Vec<T> and Box<[T]>) back and forth between Rust and JS. Previously, it was only possible to do this where T is a primitive type, a JsValue, or a type implementing JsCast (which is for types representing a value in JavaScript, where it's possible to cast them to a JsValue for free). The ability to pass vectors of other types such as String (#168) and especially exported structs (#111) has been a popular request. So far people have had to use some unfortunate workarounds to do this.

There are a few traits that are necessary to implement to allow for bidirectional conversion of a type between Rust and JS:

1. WasmDescribe, which serialises the type (not a value but the type itself) so that the CLI tool can generate JS code to correctly handle the conversion.
2. IntoWasmAbi, which converts a value of the type into a form that can be passed directly to JS. For anything other than primitive types this is essentially a pointer and a length, so the JS side can read the data itself from the WASM memory.
3. FromWasmAbi, which is the inverse of IntoWasmAbi. This takes the simple values that can be passed directly from JS and constructs a value of the type.

These traits are blanket implemented for Vec<T> wherever they are implemented for Box<[T]>.

The basis of this PR is the use of the existing implementations of these traits for Box<[JsValue]> for other types, by converting these types to and from JsValues. For a type T: Into<JsValue> + TryFrom<JsValue> we can implement IntoWasmAbi for Box<[T]> by mapping it to convert each value to a JsValue using into(), and then convert the resulting Box<[JsValue]> as normal. Similarly FromWasmAbi can be implemented by converting the given ABI values to a Box<[JsValue]> and then converting it back to Box<[T]> using try_from() in a map. My implementation panics if any element's conversion fails. I figured this is acceptable based on the behaviour of other implementations of FromWasmAbi when the wrong type is passed which seems to just be undefined.

The JS side can just treat this the same way as any other JsValue vector, so the implementation of WasmDescribe for Box<[JsValue]> can be used.

The downside of this approach is that it involves a lot of calls between Rust and JS, which might be a performance bottleneck in some situations. At least it's no worse than the workarounds people are doing anyway. It can be optimised in future if anyone finds a more performant way of doing it.

A naive approach would be to implement these traits with a blanket implementation on Box<[T]> where T: Into<JsValue> + TryFrom<JsValue>, which I attempted but it didn't compile because it conflicts with the blanket implementations for implementors of JsObject. So I created another trait: JsValueVector, which has the blanket implementation. I wrote a macro js_value_vectors that generates concrete implementations of IntoWasmAbi etc. using the blanket implementation of JsValueVector for the given types. Currently the only thing calling this macro is String.

Implementing the needed traits for exported structs was a little more challenging.

Firstly, it's impossible to implement e.g. IntoWasmAbi for Box<[T]> from outside of the wasm-bindgen crate. This is because of the rule that traits can only be implemented on arbitrary types in the crate in which they are defined. I solved this problem by creating another set of traits: WasmDescribeVector, VectorIntoWasmAbi etc. that basically define the behaviour of WasmDescribe, IntoWasmAbi etc. respectively for Box<[T]> when they are defined on a concrete type T. I created blanket implementations so that implementing e.g. VectorIntoWasmAbi for T automatically implements IntoWasmAbi for Box<[T]>. I had to refactor out the blanket implementations for JsObject so that they implement my new traits since the original traits can only have one blanket implementation.

Secondly, there was no good way to recover an instance of an exported struct from a JsValue with which to implement the necessary TryFrom<JsValue> for JsValueVector to work. I added an __unwrap function to the JS wrapper for exported structs. This simply unwraps it to the underlying exported struct instance.

Could you or maybe @chinedufn advise me on https://github.com/rustwasm/wasm-bindgen/blob/main/crates/shared/src/schema_hash_approval.rs ? The comment reads:

// Whenever the lib.rs changes, the SCHEMA_FILE_HASH environment variable will change and the
// schema_version test below will fail.
// Proceed as follows:
//
// If the schema in this library has changed then:
//  1. Change this APPROVED_SCHEMA_FILE_HASH to the new hash.
//
// If the schema in this library has changed then:
//  1. Bump the version in `crates/shared/Cargo.toml`
//  2. Change the `SCHEMA_VERSION` in this library to this new Cargo.toml version

I think the top option might be supposed to say 'not changed'? Should version numbers be bumped in general in PRs for this project? I assumed this would be done on master when it's time for a new release.

Please let me know if anything needs to be clarified or if there are any amendments you would like.

[abi]

Any follow-ups on this PR? Would be a useful feature to have when working with wasm-bindgen.

[sinking-point]

@abi I haven't been able to get clarification on the failing test I mentioned in my previous comment.

@chinedufn @alexcrichton could either of you advise me on that? If you're too busy at the moment please just leave a comment so I know you've seen this and are aware that I need advice to continue.

[angelorodem]

Hi! any follow-ups on this? this is a very nice/important feature that is missing in in bindgen, it would do great for this project for this to be reviewed

[Liamolucko]

This looks great! I have a couple comments, and it needs to be rebased, but after that this looks good to merge. Some tests would also be good.

I'm happy to handle rebasing and such if @sinking-point is no longer interested.

[Liamolucko]

It feels kind of wrong to be throwing away the initial allocation and making a new one here, but it works for an MVP and can be optimised later if necessary.

I have some ideas how this can be avoided by doing more work in JS, but I have no idea if that'd actually end up being faster, so let's just go with this for now.

[sinking-point]

Exactly my thinking, let's not let perfect be the enemy of good.

[Liamolucko]

It feels like a bad idea to silently throw away array elements that fail to be converted; I'd prefer if this used unwrap_throw to propagate the error instead.

[sinking-point]

Agreed. No idea why I did this tbh. I thought I made it so it would panic.

[Liamolucko]

This trait doesn't seem super necessary; you could replace it with a bunch of regular generic functions instead, like this:

pub fn js_value_vector_into_abi<T>(vector: Box<[T]>) -> <Box<[JsValue]> as IntoWasmAbi>::Abi
where
    T: Into<JsValue>,
{
    let js_vals: Box<[JsValue]> = vector.into_vec().into_iter().map(|x| x.into()).collect();

    IntoWasmAbi::into_abi(js_vals)
}

pub unsafe fn js_value_vector_from_abi<T>(js: <Box<[JsValue]> as FromWasmAbi>::Abi) -> Box<[T]>
where
    T: TryFrom<JsValue>,
    <T as TryFrom<JsValue>>::Error: core::fmt::Debug,
{
    let js_vals = <Vec<JsValue> as FromWasmAbi>::from_abi(js);

    js_vals
        .into_iter()
        .map(|x| x.try_into().unwrap_throw())
        .collect()
}

// `describe` and `none` are so simple I don't really see the point in putting
// functions for them here, but you can if you want.

[sinking-point]

You may be right. I don't really have the context anymore to properly defend my decisions. Maybe I did it this way simply because I thought it was neater. I think it's nice to have it bundled up into a trait like this, so that if anyone wants to implement the same functionality for another type, they know exactly what they have to do. But if you would rather have generic functions instead, I don't mind.

[Liamolucko]

Suggested change

	type Error = ();
	type Error = JsValue;

It's not great practice to use () as an error type. Normally I'd probably suggest creating a new error type for this, but all of wasm-bindgen's existing TryFrom<JsValue> impls use JsValue for their error type and so I think it's a better idea to keep things consistent.

[sinking-point]

Thanks for the review @Liamolucko . I did this work last year because I wanted this feature for a project I was working on at the time. However, it's no longer a priority for me. I have other work to do and I don't forsee myself making time for this in the foreseeable future.

Please feel free to pick up where I left off. I had accepted, painfully, that this work had gone to waste, so I'm very grateful that you're rescuing it. Thank you.

[daxpedda]

Was replaced by #3554.