Add support for wasm-bindgen #541

alexcrichton · 2018-07-06T05:15:35Z

This commit adds support to implement the rand crate on the
wasm32-unknown-unknown target with the wasm-bindgen-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes #478

dhardy · 2018-07-06T09:10:20Z

Thanks, but I hope someone else can review. (I don't know enough about the target platforms.)

alexcrichton · 2018-07-06T14:07:50Z

@fitzgen want to help take a look at this?

fitzgen · 2018-07-06T15:53:55Z

CI is failing on cargo build --no-default-features:

error: Could not compile `rand`.
warning: build failed, waiting for other jobs to finish...
error: unmatched visibility `pub`
   --> src/lib.rs:897:9
    |
897 |         pub type This;
    |         ^^^
error: aborting due to previous error

fitzgen

LGTM! A few comments below, but I'm also not super familiar with the rand code base.

fitzgen · 2018-07-06T15:54:51Z

README.md

@@ -119,6 +119,10 @@ optional features are available:
 - `serde1` enables serialization for some types, via Serde version 1.
 - `stdweb` enables support for `OsRng` on `wasm-unknown-unknown` via `stdweb`
  combined with `cargo-web`.
+- `stdweb` enables support for `OsRng` on `wasm-unknown-unknown` via


s/stdweb/wasm-bindgen/

Also wasm-unknown-unknown should be wasm32-unknown-unknown here and the existing spot above.

fitzgen · 2018-07-06T16:00:12Z

src/rngs/os.rs

+    use super::OsRngImpl;
+
+    #[derive(Clone)]
+    pub enum OsRng {


Is this definition exposed publicly, or does it get wrapped? If the former, might want to hide the fact that it is an enum:

pub struct OsRng(OsRngKind); enum OsRngKind { Node(..), Browser(..), }

Ah yeah this is wrapped above

alexcrichton · 2018-07-06T16:19:59Z

Updated!

dhardy · 2018-07-07T08:18:22Z

src/rngs/os.rs

+    impl OsRngImpl for OsRng {
+        fn new() -> Result<OsRng, Error> {
+            if this.window().is_undefined() {
+                return Ok(OsRng::Node(node_require("crypto")))


Is the logic inverted here or is it simply assumed that node_require("crypto") will not fail?

I assume that window() being undefined implies we can't use browser-crypto, so I guess this is just missing a check that NodeCrypto is available?

(If it really cannot fail then I don't understand why you return errors below instead of falling back to Node.)

Currently it's assumed that require("crypto") won't fail as I figured that node has had that API for quite some time. Is that something that should be checked for?

I figured below it'd be good to test that if we see we're in a browser whether crypto actually works, and falling back to an error if it looks like an older browser or something like that.

The crypto module has been around since Node 0.x so it's safe to assume importing it won't fail.

crypto.randomFillSync was backported to Node 6.x, the oldest version still being maintained, so that's also safe to use blindly.

Thanks! I've updated with a comment about this

dhardy · 2018-07-07T08:24:36Z

src/rngs/os.rs

+            }
+            let crypto: BrowserCrypto = crypto.into();
+            if crypto.get_random_values_fn().is_undefined() {
+                let msg = "crypto.getRandomValues is undefined";


Should be "window.crypto.getRandomValues is undefined"?

Either way these aren't very user-friendly messages, though I guess they may not need to be.

At least for browsers window is the ambient environment so I believe that window.crypto is basically the same as a bare crypto

jsheard · 2018-07-07T14:42:39Z

Don't forget to update the docs in os.rs (lines 62-64)

/// The bare Wasm target `wasm32-unknown-unknown` tries to call the javascript
/// methods directly, using `stdweb` in combination with `cargo-web`.
/// `wasm-bindgen` is not yet supported.

jsheard · 2018-07-08T16:23:58Z

src/rngs/os.rs

+            //
+            // > A QuotaExceededError DOMException is thrown if the requested
+            // > length is greater than 65536 bytes.
+            65536


There doesn't appear to be a limit in Nodes API, should this return 65536 in browsers and usize::MAX on node?

dhardy · 2018-07-08T20:27:18Z

Ah, thanks for the extra docs; makes that clear 👍

dhardy · 2018-07-11T09:56:12Z

@alexcrichton can you rebase please? I fixed the build errors in the master branch.

Once the CI is green I think this is good to go.

This commit adds support to implement the `rand` crate on the wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This supports being run both in node.js as well as browsers by delegating appropriately. Closes rust-random#478

alexcrichton · 2018-07-11T14:07:55Z

Done!

David-OConnor · 2018-07-29T05:53:26Z

It compiles on unknown-unknown for me using the latest nightly, but if I call rand::random from a wasm-bindgen lib, I receive the following error in browser:

wasm-0054f422-3479:2 Uncaught (in promise) RuntimeError: unreachable
    at core::ptr::drop_in_place::h725cef5c01ce1c6b (wasm-function[3479]:1)
    at core::ptr::drop_in_place::h1438bb682828fcde (wasm-function[3478]:30)
    at __rdl_dealloc (wasm-function[3473]:444)
    at std::sys_common::util::abort::hcc613e7d553b3a50 (wasm-function[3471]:122)
    at __rdl_alloc (wasm-function[3472]:95)
    at core::alloc::Layout::from_size_align_unchecked::h7987ae01fa261dac (wasm-function[3126]:206)
    at core::num::_$LT$impl$u20$u32$GT$::to_le::hbbc43fbc8982e72f (wasm-function[3023]:518)
    at core::mem::size_of::hd49dd597db7ba2e6 (wasm-function[3125]:113)
    at alloc::alloc::alloc::h0e20e5218e896b1d (wasm-function[3016]:52)
    at core::num::NonZeroUsize::new_unchecked::h991b4a788ad6ce7a (wasm-function[3018]:246)

Including rand in cargo.toml doesn't trigger this alone, nor does importing it. Using rand::random does, even if the value isn't passed to JS.

alexcrichton · 2018-07-30T17:54:27Z

@David-OConnor interesting! The "am I in a browser" check I think may be subtly wrong here (as I later discovered) which may be what's at fault here perhaps? In any case that's a pretty bad error message :(

alexcrichton force-pushed the wbg branch from 6cf5c72 to 8ab78fd Compare July 6, 2018 05:16

alexcrichton mentioned this pull request Jul 6, 2018

Support feature detecting if an API is available rustwasm/wasm-bindgen#398

Open

alexcrichton force-pushed the wbg branch from 8ab78fd to bb5c1be Compare July 6, 2018 14:07

fitzgen approved these changes Jul 6, 2018

View reviewed changes

alexcrichton force-pushed the wbg branch from bb5c1be to d78953e Compare July 6, 2018 16:19

dhardy reviewed Jul 7, 2018

View reviewed changes

alexcrichton force-pushed the wbg branch from d78953e to 94758ae Compare July 8, 2018 15:29

jsheard reviewed Jul 8, 2018

View reviewed changes

alexcrichton force-pushed the wbg branch from 94758ae to 832c79e Compare July 8, 2018 16:30

Add support for wasm-bindgen

1403855

This commit adds support to implement the `rand` crate on the wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This supports being run both in node.js as well as browsers by delegating appropriately. Closes rust-random#478

alexcrichton force-pushed the wbg branch from 832c79e to 1403855 Compare July 11, 2018 14:07

dhardy merged commit da2f7fa into rust-random:master Jul 11, 2018

dhardy mentioned this pull request Jul 13, 2018

Tracker: 0.6 release #520

Closed

28 tasks

alexcrichton deleted the wbg branch July 30, 2018 17:53

JeanMertz mentioned this pull request Jun 26, 2019

rand 0.6 lipanski/mockito#73

Closed

sync-by-unito bot mentioned this pull request Dec 15, 2020

Bump rand_core from 0.5.1 to 0.6.0 AleoHQ/leo#503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for wasm-bindgen #541

Add support for wasm-bindgen #541

alexcrichton commented Jul 6, 2018

dhardy commented Jul 6, 2018

alexcrichton commented Jul 6, 2018

fitzgen commented Jul 6, 2018

fitzgen left a comment

fitzgen Jul 6, 2018

fitzgen Jul 6, 2018

alexcrichton Jul 6, 2018

alexcrichton commented Jul 6, 2018

dhardy Jul 7, 2018

alexcrichton Jul 8, 2018

jsheard Jul 8, 2018

alexcrichton Jul 8, 2018

dhardy Jul 7, 2018

alexcrichton Jul 8, 2018

jsheard commented Jul 7, 2018

jsheard Jul 8, 2018

alexcrichton Jul 8, 2018

dhardy commented Jul 8, 2018

dhardy commented Jul 11, 2018

alexcrichton commented Jul 11, 2018

David-OConnor commented Jul 29, 2018 •

edited

alexcrichton commented Jul 30, 2018

Add support for wasm-bindgen #541

Add support for wasm-bindgen #541

Conversation

alexcrichton commented Jul 6, 2018

dhardy commented Jul 6, 2018

alexcrichton commented Jul 6, 2018

fitzgen commented Jul 6, 2018

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsheard commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Jul 8, 2018

dhardy commented Jul 11, 2018

alexcrichton commented Jul 11, 2018

David-OConnor commented Jul 29, 2018 • edited

alexcrichton commented Jul 30, 2018

David-OConnor commented Jul 29, 2018 •

edited