Skip to content

Commit

Permalink
Allow creating Vecs of and implement TryFrom<JsValue> for strings…
Browse files Browse the repository at this point in the history
… and exported Rust types (#3554)

* Enable passing String vectors and boxed slices across ABI

This is accomplished via conversion of the Strings to/from JsValues.

* Enable passing custom struct vectors over ABI

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.

* Remove unneeded require

* Move uses out of if_std

* Add documentation

* Move incorrect use statements

* Fix mistake in comment

* Throw on invalid array elements instead of silently removing them

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?

* Get rid of `JsValueVector`

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.

* Respect the configured `wasm_bindgen` crate path

* Change the error type for String and rust structs' TryFrom<JsValue> impls to JsValue

* test string vecs too

* Refactor `String` impls

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.

* Don't require manual `OptionVector{From,Into}WasmAbi` impls

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.

* fix clippy

* Fix generated typescript

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`.

* reorder some impls

This is the nitpickiest of nitpicks, but this is my PR goddammit and I can do what I want :)

* Update schema hash

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?

* undo some unnecessary refactors

* don't pointlessly use assert.deepStrictEqual for numbers

* Update the guide

* update reference tests

* add WASI check

* Extremely nitpicky tweaks

---------

Co-authored-by: Billy Bradley <billy@squeno.com>
Co-authored-by: Billy Bradley <billy.jack.bradley@gmail.com>
  • Loading branch information
3 people authored Sep 4, 2023
1 parent 86fd961 commit 4d4851d
Show file tree
Hide file tree
Showing 23 changed files with 511 additions and 85 deletions.
73 changes: 72 additions & 1 deletion crates/backend/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,10 @@ impl ToTokens for ast::Struct {
let name = &self.rust_name;
let name_str = self.js_name.to_string();
let name_len = name_str.len() as u32;
let name_chars = name_str.chars().map(|c| c as u32);
let name_chars: Vec<u32> = name_str.chars().map(|c| c as u32).collect();
let new_fn = Ident::new(&shared::new_function(&name_str), Span::call_site());
let free_fn = Ident::new(&shared::free_function(&name_str), Span::call_site());
let unwrap_fn = Ident::new(&shared::unwrap_function(&name_str), Span::call_site());
let wasm_bindgen = &self.wasm_bindgen;
(quote! {
#[automatically_derived]
Expand Down Expand Up @@ -293,6 +294,76 @@ impl ToTokens for ast::Struct {
#[inline]
fn is_none(abi: &Self::Abi) -> bool { *abi == 0 }
}

#[allow(clippy::all)]
impl #wasm_bindgen::__rt::core::convert::TryFrom<#wasm_bindgen::JsValue> for #name {
type Error = #wasm_bindgen::JsValue;

fn try_from(value: #wasm_bindgen::JsValue)
-> #wasm_bindgen::__rt::std::result::Result<Self, Self::Error> {
let idx = #wasm_bindgen::convert::IntoWasmAbi::into_abi(&value);

#[link(wasm_import_module = "__wbindgen_placeholder__")]
#[cfg(all(target_arch = "wasm32", not(any(target_os = "emscripten", target_os = "wasi"))))]
extern "C" {
fn #unwrap_fn(ptr: u32) -> u32;
}

#[cfg(not(all(target_arch = "wasm32", not(any(target_os = "emscripten", target_os = "wasi")))))]
unsafe fn #unwrap_fn(_: u32) -> u32 {
panic!("cannot convert from JsValue outside of the wasm target")
}

let ptr = unsafe { #unwrap_fn(idx) };
if ptr == 0 {
#wasm_bindgen::__rt::std::result::Result::Err(value)
} else {
// Don't run `JsValue`'s destructor, `unwrap_fn` already did that for us.
#wasm_bindgen::__rt::std::mem::forget(value);
unsafe {
#wasm_bindgen::__rt::std::result::Result::Ok(
<Self as #wasm_bindgen::convert::FromWasmAbi>::from_abi(ptr)
)
}
}
}
}

impl #wasm_bindgen::describe::WasmDescribeVector for #name {
fn describe_vector() {
use #wasm_bindgen::describe::*;
inform(VECTOR);
inform(NAMED_EXTERNREF);
inform(#name_len);
#(inform(#name_chars);)*
}
}

impl #wasm_bindgen::convert::VectorIntoWasmAbi for #name {
type Abi = <
#wasm_bindgen::__rt::std::boxed::Box<[#wasm_bindgen::JsValue]>
as #wasm_bindgen::convert::IntoWasmAbi
>::Abi;

fn vector_into_abi(
vector: #wasm_bindgen::__rt::std::boxed::Box<[#name]>
) -> Self::Abi {
#wasm_bindgen::convert::js_value_vector_into_abi(vector)
}
}

impl #wasm_bindgen::convert::VectorFromWasmAbi for #name {
type Abi = <
#wasm_bindgen::__rt::std::boxed::Box<[#wasm_bindgen::JsValue]>
as #wasm_bindgen::convert::FromWasmAbi
>::Abi;

unsafe fn vector_from_abi(
js: Self::Abi
) -> #wasm_bindgen::__rt::std::boxed::Box<[#name]> {
#wasm_bindgen::convert::js_value_vector_from_abi(js)
}
}
})
.to_tokens(tokens);

Expand Down
27 changes: 27 additions & 0 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub struct ExportedClass {
generate_typescript: bool,
has_constructor: bool,
wrap_needed: bool,
unwrap_needed: bool,
/// Whether to generate helper methods for inspecting the class
is_inspectable: bool,
/// All readable properties of the class
Expand Down Expand Up @@ -935,6 +936,20 @@ impl<'a> Context<'a> {
));
}

if class.unwrap_needed {
dst.push_str(&format!(
"
static __unwrap(jsValue) {{
if (!(jsValue instanceof {})) {{
return 0;
}}
return jsValue.__destroy_into_raw();
}}
",
name,
));
}

if self.config.weak_refs {
self.global(&format!(
"const {}Finalization = new FinalizationRegistry(ptr => wasm.{}(ptr >>> 0));",
Expand Down Expand Up @@ -2247,6 +2262,10 @@ impl<'a> Context<'a> {
require_class(&mut self.exported_classes, name).wrap_needed = true;
}

fn require_class_unwrap(&mut self, name: &str) {
require_class(&mut self.exported_classes, name).unwrap_needed = true;
}

fn add_module_import(&mut self, module: String, name: &str, actual: &str) {
let rename = if name == actual {
None
Expand Down Expand Up @@ -3213,6 +3232,14 @@ impl<'a> Context<'a> {
See https://rustwasm.github.io/wasm-bindgen/reference/cli.html#--split-linked-modules for details.", path))
}
}

AuxImport::UnwrapExportedClass(class) => {
assert!(kind == AdapterJsImportKind::Normal);
assert!(!variadic);
assert_eq!(args.len(), 1);
self.require_class_unwrap(class);
Ok(format!("{}.__unwrap({})", class, args[0]))
}
}
}

Expand Down
35 changes: 29 additions & 6 deletions crates/cli-support/src/wit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -934,17 +934,40 @@ impl<'a> Context<'a> {
self.aux.structs.push(aux);

let wrap_constructor = wasm_bindgen_shared::new_function(struct_.name);
if let Some((import_id, _id)) = self.function_imports.get(&wrap_constructor).cloned() {
self.add_aux_import_to_import_map(
&wrap_constructor,
vec![Descriptor::I32],
Descriptor::Externref,
AuxImport::WrapInExportedClass(struct_.name.to_string()),
)?;

let unwrap_fn = wasm_bindgen_shared::unwrap_function(struct_.name);
self.add_aux_import_to_import_map(
&unwrap_fn,
vec![Descriptor::Externref],
Descriptor::I32,
AuxImport::UnwrapExportedClass(struct_.name.to_string()),
)?;

Ok(())
}

fn add_aux_import_to_import_map(
&mut self,
fn_name: &String,
arguments: Vec<Descriptor>,
ret: Descriptor,
aux_import: AuxImport,
) -> Result<(), Error> {
if let Some((import_id, _id)) = self.function_imports.get(fn_name).cloned() {
let signature = Function {
shim_idx: 0,
arguments: vec![Descriptor::I32],
ret: Descriptor::Externref,
arguments,
ret,
inner_ret: None,
};
let id = self.import_adapter(import_id, signature, AdapterJsImportKind::Normal)?;
self.aux
.import_map
.insert(id, AuxImport::WrapInExportedClass(struct_.name.to_string()));
self.aux.import_map.insert(id, aux_import);
}

Ok(())
Expand Down
5 changes: 5 additions & 0 deletions crates/cli-support/src/wit/nonstandard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,11 @@ pub enum AuxImport {
/// The Option may contain the contents of the linked file, so it can be
/// embedded.
LinkTo(String, Option<String>),

/// This import is a generated shim which will attempt to unwrap JsValue to an
/// instance of the given exported class. The class name is one that is
/// exported from the Rust/wasm.
UnwrapExportedClass(String),
}

/// Values that can be imported verbatim to hook up to an import.
Expand Down
3 changes: 3 additions & 0 deletions crates/cli-support/src/wit/section.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ fn check_standard_import(import: &AuxImport) -> Result<(), Error> {
format!("wasm-bindgen specific link function for `{}`", path)
}
AuxImport::Closure { .. } => format!("creating a `Closure` wrapper"),
AuxImport::UnwrapExportedClass(name) => {
format!("unwrapping a pointer from a `{}` js class wrapper", name)
}
};
bail!("import of {} requires JS glue", item);
}
Expand Down
12 changes: 6 additions & 6 deletions crates/macro/ui-tests/missing-catch.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ error[E0277]: the trait bound `Result<wasm_bindgen::JsValue, wasm_bindgen::JsVal
*const T
*mut T
Box<[T]>
Box<[f32]>
Box<[f64]>
Box<[i16]>
Box<[i32]>
Box<[i64]>
and 35 others
Clamped<T>
Option<T>
Option<f32>
Option<f64>
Option<i32>
and $N others
2 changes: 1 addition & 1 deletion crates/macro/ui-tests/traits-not-implemented.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
&'a (dyn Fn(A, B, C, D, E) -> R + 'b)
&'a (dyn Fn(A, B, C, D, E, F) -> R + 'b)
&'a (dyn Fn(A, B, C, D, E, F, G) -> R + 'b)
and 84 others
and $N others
= note: this error originates in the attribute macro `wasm_bindgen` (in Nightly builds, run with -Z macro-backtrace for more info)
7 changes: 7 additions & 0 deletions crates/shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@ pub fn free_function(struct_name: &str) -> String {
name
}

pub fn unwrap_function(struct_name: &str) -> String {
let mut name = "__wbg_".to_string();
name.extend(struct_name.chars().flat_map(|s| s.to_lowercase()));
name.push_str("_unwrap");
name
}

pub fn free_function_export_name(function_name: &str) -> String {
function_name.to_string()
}
Expand Down
2 changes: 1 addition & 1 deletion crates/shared/src/schema_hash_approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// 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
const APPROVED_SCHEMA_FILE_HASH: &str = "12040133795598472740";
const APPROVED_SCHEMA_FILE_HASH: &str = "5679641936258023729";

#[test]
fn schema_version() {
Expand Down
2 changes: 1 addition & 1 deletion guide/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
- [Imported JavaScript Types](./reference/types/imported-js-types.md)
- [Exported Rust Types](./reference/types/exported-rust-types.md)
- [`JsValue`](./reference/types/jsvalue.md)
- [`Box<[JsValue]>`](./reference/types/boxed-jsvalue-slice.md)
- [`Box<[T]>` and `Vec<T>`](./reference/types/boxed-slices.md)
- [`*const T` and `*mut T`](./reference/types/pointers.md)
- [Numbers](./reference/types/numbers.md)
- [`bool`](./reference/types/bool.md)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
# `Box<[JsValue]>`
# `Box<[T]>` and `Vec<T>`

| `T` parameter | `&T` parameter | `&mut T` parameter | `T` return value | `Option<T>` parameter | `Option<T>` return value | JavaScript representation |
|:---:|:---:|:---:|:---:|:---:|:---:|:---:|
| Yes | No | No | Yes | Yes | Yes | A JavaScript `Array` object |

Boxed slices of imported JS types and exported Rust types are also supported. `Vec<T>` is supported wherever `Box<[T]>` is.
You can pass boxed slices and `Vec`s of several different types to and from JS:

- `JsValue`s.
- Imported JavaScript types.
- Exported Rust types.
- `String`s.

[You can also pass boxed slices of numbers to JS](boxed-number-slices.html),
except that they're converted to typed arrays (`Uint8Array`, `Int32Array`, etc.)
instead of regular arrays.

## Example Rust Usage

Expand Down
45 changes: 43 additions & 2 deletions src/convert/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,14 @@ use core::mem::{self, ManuallyDrop};
use crate::convert::traits::WasmAbi;
use crate::convert::{FromWasmAbi, IntoWasmAbi, LongRefFromWasmAbi, RefFromWasmAbi};
use crate::convert::{OptionFromWasmAbi, OptionIntoWasmAbi, ReturnWasmAbi};
use crate::{Clamped, JsError, JsValue};
use crate::{Clamped, JsError, JsValue, UnwrapThrowExt};

if_std! {
use std::boxed::Box;
use std::convert::{TryFrom, TryInto};
use std::fmt::Debug;
use std::vec::Vec;
}

unsafe impl WasmAbi for () {}

Expand Down Expand Up @@ -321,7 +328,7 @@ impl IntoWasmAbi for () {
/// - u32/i32/f32/f64 fields at the "leaf fields" of the "field tree"
/// - layout equivalent to a completely flattened repr(C) struct, constructed by an in order
/// traversal of all the leaf fields in it.
///
///
/// This means that you can't embed struct A(u32, f64) as struct B(u32, A); because the "completely
/// flattened" struct AB(u32, u32, f64) would miss the 4 byte padding that is actually present
/// within B and then as a consequence also miss the 4 byte padding within A that repr(C) inserts.
Expand Down Expand Up @@ -386,3 +393,37 @@ impl IntoWasmAbi for JsError {
self.value.into_abi()
}
}

if_std! {
// Note: this can't take `&[T]` because the `Into<JsValue>` impl needs
// ownership of `T`.
pub fn js_value_vector_into_abi<T: Into<JsValue>>(vector: Box<[T]>) -> <Box<[JsValue]> as IntoWasmAbi>::Abi {
let js_vals: Box<[JsValue]> = vector
.into_vec()
.into_iter()
.map(|x| x.into())
.collect();

js_vals.into_abi()
}

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

let mut result = Vec::with_capacity(js_vals.len());
for value in js_vals {
// We push elements one-by-one instead of using `collect` in order to improve
// error messages. When using `collect`, this `expect_throw` is buried in a
// giant chain of internal iterator functions, which results in the actual
// function that takes this `Vec` falling off the end of the call stack.
// So instead, make sure to call it directly within this function.
//
// This is only a problem in debug mode. Since this is the browser's error stack
// we're talking about, it can only see functions that actually make it to the
// final wasm binary (i.e., not inlined functions). All of those internal
// iterator functions get inlined in release mode, and so they don't show up.
result.push(value.try_into().expect_throw("array contains a value of the wrong type"));
}
result.into_boxed_slice()
}
}
Loading

0 comments on commit 4d4851d

Please sign in to comment.