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

Implement RFC #2 - casting hierarchy between JS values #640

Merged
merged 5 commits into from
Aug 7, 2018

Conversation

alexcrichton
Copy link
Contributor

This is an implementation of rustwasm/rfcs#2 for the wasm-bindgen suite of crates. Nothing too too tricky here and the commits inline should explain what they're doing.

One question to grapple with is that today we generate From<JsValue> for ImportedType, but that's not intended from RFC 2, so we'll want to remove it at some point. This PR currently doesn't delete it so we can avoid making a major bump to 0.3, but we may want to go ahead and bump to 0.3 depending on other schedules!

Note: at the time of this writing the RFC isn't merged yet, so this shouldn't be merged until that's done.

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.

Wonderful! Thanks @alexcrichton!

Do we want to also add extends = ... attributes to js-sys in this PR, or as a follow up?

src/cast.rs Outdated
{
/// Test whether this JS value is an instance of the type `T`.
///
/// This commit performs a dynamic check (at runtime) using the JS
Copy link
Member

Choose a reason for hiding this comment

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

"commit" -> "method" ?

src/cast.rs Outdated
///
/// This method will return `Err(self)` is `self.is_instance_of::<T>()`
/// returns `false`, and otherwise it will return `Ok(T)` manufactured with
/// an unchecked cast (verified safe via the `instanceof` operation).
Copy link
Member

Choose a reason for hiding this comment

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

"safe" -> "correct" since we aren't marking unchecked casts as unsafe since it doesn't result in memory unsafety

src/cast.rs Outdated
///
/// This method will return `None` is `self.is_instance_of::<T>()`
/// returns `false`, and otherwise it will return `Some(&T)` manufactured
/// with an unchecked cast (verified safe via the `instanceof` operation).
Copy link
Member

Choose a reason for hiding this comment

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

Ditto re "safe"

src/cast.rs Outdated
///
/// This commit performs a dynamic check (at runtime) using the JS
/// `instanceof` operator. This method returns `self instanceof T`.
fn is_instance_of<T>(&self) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

I imagine we probably want to mark all of these methods as #[inline].

src/cast.rs Outdated
/// `self` and `T` are simple wrappers around `JsValue`. This method **does
/// not check whether `self` is an instance of `T`**. If used incorrectly
/// then this method may cause runtime exceptions in both Rust and JS, this
/// shoudl be used with caution.
Copy link
Member

Choose a reason for hiding this comment

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

"shoudl" -> "should"

Copy link
Contributor

Choose a reason for hiding this comment

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

Still not fixed in 2 other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops thanks, fixed now.

fn describe() {
::wasm_bindgen::JsValue::describe();
#[allow(bad_style)]
const #const_name: () = {
Copy link
Member

Choose a reason for hiding this comment

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

lol this is a nice/nasty trick

@@ -594,18 +596,63 @@ impl ToTokens for ast::ImportType {
}
}

// TODO: remove this on the next major version
Copy link
Member

Choose a reason for hiding this comment

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

Can one mark trait impls as deprecated?

@@ -168,3 +168,25 @@ possibilities!

All of these functions will call `console.log` in JS, but each identifier
will have only one signature in Rust.

* `extends = Bar` - this can be used to say that an imported type extends
Copy link
Member

Choose a reason for hiding this comment

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

This is going to need to move to guide/src/reference/attributes/on-js-imports/extends.md and a corresponding entry in the SUMMARY.md table of contents

Group all the generated impls in a `const` block so we can use `use` without
clashing with the outside scope.
This commit implements the `JsCast` trait automatically for all imported types
in `#[wasm_bindgen] extern { ... }` blocks. The main change here was to generate
an `instanceof` shim for all imported types in case it's needed.

All imported types now also implement `AsRef<JsValue>` and `AsMut<JsValue>`
This commit implements the `extends` attribute for `#[wasm_bindgen]` to
statically draw the inheritance hierarchy in the generated bindings, generating
appropriate `AsRef`, `AsMut`, and `From` implementations.
@alexcrichton
Copy link
Contributor Author

Updated!

@@ -0,0 +1,170 @@
# Customizing import behavior
Copy link
Member

Choose a reason for hiding this comment

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

This file got accidentally resurrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, should be dead for real now

@alexcrichton alexcrichton merged commit 5b93552 into rustwasm:master Aug 7, 2018
@alexcrichton alexcrichton deleted the jscast branch August 7, 2018 22:26
@alexcrichton alexcrichton mentioned this pull request Aug 14, 2018
1 task
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.

3 participants