Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow creating Vecs of and implement TryFrom<JsValue> for strings and exported Rust types #3554

Merged

Conversation

Liamolucko
Copy link
Collaborator

This is a rebase of #2734 along with some fixes and tweaks. Sorry it took me so long to get to it!

This PR:

  • Adds support for passing Vecs of Rust structs to/from JS.
  • Adds support for passing Vec<String>s to/from JS.
  • Implements TryFrom<JsValue> for Rust structs and strings.

The Vec impls are little inefficient, since they work by passing Vec<JsValue>s to/from JS like normal, and then translating them to the desired Vec with TryFrom / Into<JsValue>, which requires throwing away the first allocation and making a second one.

Fixes #111
Fixes #2452
Fixes #168
Fixes #2231

This also mostly supersedes #3088. That PR does support slightly more functionality (getting access to &Struct instead of forcing you to take ownership of it and invalidate JS's handle), but it also does so by exposing the internal Anchor type used by RefFromWasmAbi, which I'm not a huge fan of.

I haven't bumped the schema version, since the actual schema hasn't changed and so older versions of the CLI are mostly still compatible, although they do give this cryptic error message if you try and use the new TryFrom<JsValue> impl for Rust structs:

error: import of `__wbg_foo_unwrap` doesn't have an adapter listed

(where foo is the name of the struct.)

So maybe it's worth bumping it anyway, not sure.

@Liamolucko Liamolucko changed the title Allow creating Vecs of and implement TryFrom<JsValue> for strings and exported Rust types. Allow creating Vecs of and implement TryFrom<JsValue> for strings and exported Rust types Aug 10, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick review and it LGTM.
Maybe I will find some time to make a proper review, but feel free to just merge if you are feeling confident about this.

@hamza1311
Copy link
Collaborator

Does this allow returning Vec<_> or such from externs? That's been a big limitation in my use of ducktor for complicated types

@Liamolucko
Copy link
Collaborator Author

Does this allow returning Vec<_> or such from externs? That's been a big limitation in my use of ducktor for complicated types

If it's a Vec of #[wasm_bindgen]'d structs or `Strings, yes.

sinking-point and others added 22 commits August 26, 2023 17:23
This is accomplished via conversion of the Strings to/from JsValues.
This was done by converting the structs 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
defined 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 existing String
implementations.

Converting structs from JsValues was accomplished by adding an unwrap
function to the generated JavaScript class, and calling that from Rust.
I put some work into making sure that you can tell what function the error message is coming from. You still have to dig into the call stack of the error message to see it, but hopefully that's not too big an ask?
The main reason for this, which I didn't mention before, is that I found it really confusing when I was originally reviewing this PR what the difference was between `JsValueVector` and `Vector{From,Into}WasmAbi`, since it really looks like another conversion trait at first glance.
I moved the `TryFrom<JsValue>` impl out of convert/slices.rs, it doesn't
really belong there, and also got rid of the `js_value_vectors!` macro
in favour of just implementing it for `String` directly; there's not
much point in a macro you only use for one type.
I noticed that strings and rust structs weren't implementing
`OptionVectorFromWasmAbi`, so I tried to make a failing test and... it
worked.

That threw me for a loop for a bit until I realised that it was because
I'd used `Vec<T>`, and `Vec<T>`'s impls of `Option{From,Into}WasmAbi`
didn't actually rely on `Box<[T]>` implementing the traits: they just
required that it implemented `{From,Into}WasmAbi` with an ABI of
`WasmSlice`, since from there the element type doesn't matter. So then
I thought 'well, why not do that for `Box<[T]>` too?

so that's how this commit's pile of new tests came to be.
Since vecs of strings and rust structs were describing themselves as `Box<[JsValue]>`, they got typed as such - as `any[]`. This fixes that by using `NAMED_EXTERNREF` instead of just plain `EXTERNREF` with the type we want.

This is maybe _slightly_ sketchy, since `NAMED_EXTERNREF` is meant for imported JS types, but ehhh it's fine. You can already put arbitrary TypeScript in there with `typescript_type`.
This is the nitpickiest of nitpicks, but this is my PR goddammit and I can do what I want :)
I didn't actually bump the schema version because it didn't change. If you don't use the `TryFrom<JsValue>` impl for Rust structs (or pass a `Vec` of them from JS to Rust), using an old CLI version will work fine; if you do though, you get a bit of a cryptic error message:

```
error: import of `__wbg_foo_unwrap` doesn't have an adapter listed
```

(That's from trying to pass a `Vec<Foo>` from JS to Rust.)

So idk, maybe that's worth bumping the schema version over anyway?
@Liamolucko Liamolucko force-pushed the more-vec-types-and-jsvalue-to-rust branch from cdceeca to 5f1808a Compare August 26, 2023 07:24
@Liamolucko Liamolucko merged commit 4d4851d into rustwasm:main Sep 4, 2023
25 checks passed
@Liamolucko Liamolucko deleted the more-vec-types-and-jsvalue-to-rust branch September 4, 2023 07:38
@sinking-point
Copy link
Contributor

Thank you @Liamolucko for sorting this out. It's so good to see my work being put to use.

Liamolucko added a commit to Liamolucko/wasm-bindgen that referenced this pull request Sep 7, 2023
Liamolucko added a commit that referenced this pull request Sep 7, 2023
@cschramm
Copy link
Contributor

cschramm commented Sep 9, 2023

I think this only partially fixes #111 and #168 (depending on their scopes).

Is it expected that it does not work for returning Vecs from async functions? For both String and #[wasm_bindgen]d structs I get an error that From<Vec<_>> is not implemented for JsValue.

Minimal example:

#[wasm_bindgen]
pub async fn f() -> Vec<String> {
    unimplemented!()
}

I can imagine that it might be an easy target, though.

Side node: Passing Vecs to async fns works fine.

cschramm added a commit to cschramm/wasm-bindgen that referenced this pull request Sep 19, 2023
Also implements `TryFrom<JsValue>` and `Into<JsValue>` for them.

Enums were left behind in rustwasm#3554. This adds the missing bits.
cschramm added a commit to cschramm/wasm-bindgen that referenced this pull request Sep 19, 2023
Also implements `TryFrom<JsValue>` and `Into<JsValue>` for them.

Enums were left behind in rustwasm#3554. This adds the missing bits.
@aspect
Copy link

aspect commented Nov 4, 2023

As this came into 0.2.88 it just broke all our framework bindings - #3685

TLDR; this implements a trait on types created by the developer who is using the wasm-bindgen framework, preventing the developer from creating his own TryFrom<JsValue> implementations, which can have many use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants