Stop using dummy implementation

[josephlr]

Inspired by @dhardy's comment rust-lang/rust#62082 (comment)

This would simply remove the dummy implementation, causing a compiler error on unsupported platforms. This makes it much easier to detect platform support issues (as a compiler error is permeable to a run time error). This would also make it clear that when porting libstd or rand to a different platform, an implementation in getrandom is needed.

For example, @newpavlov's code in rust-lang/rust#62082 would be changed to:

let mut buf = [0u8; 16];

// Using a constant value is acceptable on whitelisted targets.
#[cfg(not(target = "wasm32-unknown-unknown"))]
if let Err(err) = getrandom::getrandom(&mut buf) {
 panic!("getrandom failure: {:?}", err)
} 

let n = u128::from_ne_bytes(buf);
Cell::new([n as u64, (n >> 64) as u64])

Note that this would be a breaking change, so it would require 0.2

[dhardy]

Your code example is wrong though; we need to call getrandom on all targets.

[josephlr]

Your code example is wrong though; we need to call getrandom on all targets.

I think our current plan in rust-lang/rust#62082 was to have target = "wasm32-unknown-unknown" just use a fixed seed, and use random seed on all other targets.

[newpavlov]

I am on the fence regarding this. Initially in rand_os (predecessor of getrandom) I also was using a compilation error on unsupported platforms. I don't remember the exact reason, but we migrated to dummy implementation eventually. @dhardy maybe you remember the motivation?

[dhardy]

Vaguely, yes. See rust-random/rand#508 which took us from "compile errors on unsupported platforms" to "less features on unsupported platforms"... but having those big cfgs everywhere was a pain, hence the move to a dummy implementation. So it seems we're coming full circle. (Perhaps this isn't a bad thing, so long as we are careful to avoid breaking builds as happened here.)

[josephlr]

So it looks like currently the rest of the rand crates do a fairly good job to keep getrandom as an optional dependency (albeit one that is enabled by default). I think we can avoid breakage with the following rule:

• target has a supported libstd => getrandom will compile
• Note that the reverse should not be true. getrandom is perfectly capable of supporting no_std targets like x86_64-unknown-uefi and other bare metal x86 targets.

Then, we would not need a bunch of cfg attributes in any of the other crates, they would just get a compilation error when using an unsupported target. The only issue then becomes wasm32-unknown-unknown without either of the features enabled. For this we could do the following:

• add a new Error code UNSUPPORTED, and only return it for this target.
• The code in rust-lang/rust#62082 could just check for this error code and use a fixed seed in that case.
• This gives us the following desirable properties:
  • wasm32-unknown-unknown keeps working
  • No weird cfgs are needed in libstd to support randomness
  • Porting a new target to libstd requires thinking about RNG. Either marking the target as explicitly UNSUPPORTED or adding an implementation for that target.

[dhardy]

There is one other option that has been suggested: use wasm_bindgen by default on wasm32-unknown-unknown (and possibly drop support for stdweb). @koute and @alexcrichton know much more about Rust on WASM than me; is there any good reason not to do this?

[newpavlov]

I believe it will not be a correct approach. Ideally we should not support wasm32-unknown-unknown in getrandom at all, since with this target we can't make any assumptions about execution environment. It can be a browser, WASI sandbox, smart-contract VM, plugin system for a game, etc. So I hope we will get additional WASM targets eventually.

[alexcrichton]

I think that for now opt-in feature support is the best that we can do. It's not great, but it reflects the intentions of the targets which is that wasm32-unknown-unknown doesn't imply wasm-bindgen. We do have targets like wasm32-wasi which implies an implementation, however.

For the standard library this is somewhat moot though since it can't depend on wasm-bindgen anyway.

[newpavlov]

How about adding a disabled by default dummy feature, which will enable dummy implementation? If users will encounter any issues with compilation errors on unsupported targets (e.g. due to the old issue of leaking features), they will be able to fix it by simply enabling one feature.

We also can add a wasm_dummy feature, which will simplify using getrandom in std, i.e. std will enable wasm_dummy, but not dummy, so we will get a compilation error on new std targets which forgot to add getrandom support.