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

Add support for optional slice types #507

Merged
merged 9 commits into from Jul 19, 2018

Conversation

alexcrichton
Copy link
Contributor

This commit adds support for optional slice types to wasm-bindgen, allowing these types to be used for exported and imported APIs. They currently map to undefined in JS for None in Rust, and Some in Rust for other values in JS.

@alexcrichton
Copy link
Contributor Author

After this I hope to change the macro generation for imported types to also generate Into/From wasm abi implementations for Option<Foo> to allow optional custom types as well.

@alexcrichton
Copy link
Contributor Author

I've updated to tweak this a bit, it now interprets both null and undefined as mapping to None. The motivation for this that I've arbitrarily chosen Headers.get to fill out support for. In WebIDL this returns an optional ByteString but the actual behavior is that it returns null.

This means that when you pass None to JS it'll always generate undefined but when JS passes Rust either null or undefined it'll map to None. Hopefully this behavior will work for more APIs!

@alexcrichton
Copy link
Contributor Author

Urgh actually so after implementing more of this I don't believe this strategy will work. impl ForeignTrait for Option<LocalType> is denied by the orphan rules, and I didn't realize that! That means we'll need a different strategy here for foreign types defined in other crates (like web-sys).

@alexcrichton
Copy link
Contributor Author

Ok! I've taken a different approach now for how the underlying infrastructure works and I've also implemented automatic implementations for #[wasm_bindgen] imported types, so should be good to go now!

Hopefully this'll make the organization a little nicer over time!
This commit starts adding support for optional types to wasm-bindgen as
arguments/return values to functions. The strategy here is to add two new
traits, `OptionIntoWasmAbi` and `OptionFromWasmAbi`. These two traits are used
as a blanket impl to implement `IntoWasmAbi` and `FromWasmAbi` for `Option<T>`.

Some consequences of this design:

* It should be possible to ensure `Option<SomeForeignType>` implements to/from
  wasm traits. This is because the option-based traits can be implemented for
  foreign types.
* A specialized implementation is possible for all types, so there's no need for
  `Option<T>` to introduce unnecessary overhead.
* Two new traits is a bit unforutnate but I can't currently think of an
  alternative design that works for the above two constraints, although it
  doesn't mean one doesn't exist!
* The error messages for "can't use this type here" is actually halfway decent
  because it says these new traits need to be implemented, which provides a good
  place to document and talk about what's going on here!
* Nested references like `Option<&T>` can't implement `FromWasmAbi`. This means
  that you can't define a function in Rust which takes `Option<&str>`. It may be
  possible to do this one day but it'll likely require more trait trickery than
  I'm capable of right now.
This commit adds support for optional slice types, things like strings and
arrays. The null representation of these has a pointer value of 0, which should
never happen in normal Rust. Otherwise the various plumbing is done throughout
the tooling to enable these types in all locations.
These don't have a reference count as they're always expected to work, so avoid
actually dropping a reference on them.
This commit adds support for optional imported class types. Each type imported
with `#[wasm_bindgen]` automatically implements the relevant traits and now
supports `Option<Foo>` in various argument/return positions.
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,6 +1,3 @@
//! This is mostly an internal module, no stability guarantees are provided. Use
//! at your own risk.
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to make this stuff part of the stable interface now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah I hope to still keep this as an internal implementation detail (these docs just moved to src/convert/mod.rs). I'm not sure how long we'll be able to pull this off but I figured we may as well try!

@alexcrichton alexcrichton merged commit cbeb301 into rustwasm:master Jul 19, 2018
@alexcrichton alexcrichton deleted the optional branch July 19, 2018 19:44
@alexcrichton alexcrichton mentioned this pull request Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants