Add custom-fallback crate feature

[newpavlov]

TODO: update readme, changelog, and CI

cc @daxpedda

[dhardy]

This has the same issue as #672: any crate which adds a custom backend for wasm-js (as a convenience to wasm-web users) precludes any user from providing their own custom backend.

The only way this is "superior" to the "js" feature of v0.2 is that it moves wasm-js code outside of getrandom proper.

[dhardy]

I feel like any further points here would just be re-hashing prior discussions. My preference is still #675 (possibly with a long and very specific feature name and allow override via getrandom_backend cfg).

[newpavlov]

any crate which adds a custom backend for wasm-js (as a convenience to wasm-web users) precludes any user from providing their own custom backend.

As I wrote here, I believe it's much less likely to happen in practice js-sys implementing the custom fallback backend. Adding a new direct dependency is usually perceived as a heavier action than enabling a feature in an existing transitive dependency (despite it potentially pulling new transitive dependencies).

[dhardy]

I don't think that matters though:

1. We can allow non-web WASM users to override the backend when a wasm-js feature flag is enabled anyway, using the getrandom_backend cfg
2. It isn't really our problem to solve in any case when crate foo depends on getrandom with feature wasm-js in cases where it shouldn't. Non-web users can file a bug report against the crate.

[newpavlov]

We can allow non-web WASM users to override the backend when a wasm-js feature flag is enabled anyway, using the getrandom_backend cfg

IIRC it does not work in practice. Pulling js-sys/wasm-bindgen automatically screws up non-Web WASM developers and we can not disable/enable a dependency using cfgs.

It's much easier to see which dependency pulls js-sys/wasm-bindgen in your dependency tree, than to search for a crate which enables the wasm_js feature in getrandom.

It isn't really our problem to solve in any case when crate foo depends on getrandom with feature wasm-js in cases where it shouldn't. Non-web users can file a bug report against the crate.

Theoretically, you are right. But as our history with v0.2 has shown, in practice it does not work well. My aim with this PR (and the wasm-bindgen proposal) is to discourage such behavior as much as possible while minimally hurting ergonomics.

[dhardy]

It's also a matter of consistency. We reject PRs which add built-in support for environments without proper Rust targets associated with it (e.g. #716) and wasm_js is the only exception here.

Regarding consistency, firstly this indicates that there possibly should be a wasm-web target (off topic), and secondly I do not see the consistency as mattering much. We've had far more requests for WASM-web support than Ariel OS.

Allowing targets like Ariel to more easily support getrandom would be an argument for this PR, but when it comes to WASM-web I disagree because I think mis-use (wrong dependency on a getrandom-for-WASM-JS custom backend) is rather likely.

We could do both #675 and this PR if you like (in which case the custom backend should probably take priority where both are enabled). This would leave getrandom_backend mostly as a tool of last resort; acceptable?

[dhardy]

we can not disable/enable a dependency using cfgs

Unless the dependency itself is causing a problem this doesn't matter? (Obviously there could be Cargo.lock bloat still.)

It's much easier to see which dependency pulls js-sys/wasm-bindgen in your dependency tree

True, though there shouldn't be many dependencies on getrandom anyway.

I just think the possibility of defaulting to WASM-JS but still allowing custom backends is the best option. Is there a reason this wouldn't work?

[newpavlov]

I just think the possibility of defaulting to WASM-JS but still allowing custom backends is the best option. Is there a reason this wouldn't work?

As I wrote above, IIRC simply having wasm-bindgen in your dependency tree breaks non-Web WASM builds even if we do not use it. We have the same problem with the current wasm_js feature, but since users are unlikely to enable it for "convenience" (because we require cfg on top of it), it's much less impactful in practice.

This is why I think it's advantageous to not have js-sys as a getrandom dependency in the first place.

We could do both #675 and this PR if you like

I don't think it makes much sense to do both. It looks like we are at an impasse. So I guess @josephlr could act as a tie breaker here.

[dhardy]

As I wrote above, IIRC simply having wasm-bindgen in your dependency tree breaks non-Web WASM builds even if we do not use it.

I found #176 (Cargo.lock bloat: a minor issue) and eventually #633 (maybe a more serious issue, though could be further investigated: wasm-bindgen should not need to be linked if a cfg is used to select another backend).

I'm happy enough with the idea of placing the JS impl in js-sys or a third-party crate, though I think it might be worth using a different extern "Rust" fn name and feature name to still allow a custom backend to override the JS one — assuming that the override can actually work. Testing required.

[Pauan]

@newpavlov As I wrote above, IIRC simply having wasm-bindgen in your dependency tree breaks non-Web WASM builds even if we do not use it.

It shouldn't, if it does that's a bug.

Especially if you mark the wasm-bindgen dependency as optional:

[features]
wasm_js = ["dep:wasm-bindgen", "dep:js-sys"]

[dependencies]
wasm-bindgen = { version = "0.2.103", optional = true }
js-sys = { version = "0.3.80", optional = true }

Now the wasm-bindgen and js-sys dependencies will only be included if you enable the wasm_js feature.

[newpavlov]

@Pauan
We discuss the case when js-sys is a dependency (i.e. when we enable the wasm_js feature), but is not used (e.g. because it was overwritten using cfg).

[dhardy]

Testing required.

About that: does anyone know of non-web WASM targets we can actually test against? I think most uses are probably some type of block-chain compute platform, but we need something we can test in our CI runners.

It's not really hard to guess why people are happy to assume that WASM-means-web.

I don't care enough to argue this further; @newpavlov's preference of moving the wasm-js impl to another crate via a custom backend is good enough.

[dhardy]

Should say:

# Use the custom backend when getrandom_backend cfg is not set

[dhardy]

Concern: security.

Currently getrandom either "just works" or needs some form of configuration. Custom backends must be explicitly configured by whoever builds binaries.

This change would allow getrandom to "just work" on WASM via inclusion of an external dependency. Intermediate crates would have the ability to supply some random source.

Implication: verifying security of getrandom output on WASM-web is harder since the backend might be imported anywhere in the tree. (Though since at most one backend can be provided, this is not such a big attack vector.)

Ultimately since Rust is not strictly memory safe and therefore every crate in the build must be verified, this is not strictly worse than the alternative, though I would still prefer in-crate implementation (#675).

[dhardy]

De-approving because I dislike the idea of any crate in the dependency tree being able to supply a custom random source on WASM where the user forgets/doesn't know to configure this.

[Pauan]

@dhardy About that: does anyone know of non-web WASM targets we can actually test against? I think most uses are probably some type of block-chain compute platform, but we need something we can test in our CI runners.

The best options right now are wasm32-wasip1 and wasm32-wasip2

[dhardy]

@Pauan those targets already have support. This issue is about wasm32-unknown-unknown.

[Pauan]

@dhardy Non-JS usages have generally moved away from wasm32-unknown-unknown and instead switched to WASI (e.g. Cloudflare).

EWasm potentially would have used wasm32-unknown-unknown, but it has stalled.

CosmWasm does use wasm32-unknown-unknown.

Casper uses wasm32-unknown-unknown.

It's actually quite difficult to find non-JS uses of wasm32-unknown-unknown.

[dhardy]

@Pauan that's helpful thanks. My opinion is therefore that we should care less about non-js wasm32-unknown-unknown and adopt #675, but @newpavlov disagrees. @josephlr can we ask you to weigh in on this, please?

[josephlr]

I don't think we should do this. This is a generic functionality, but it really only makes sense in the wasm_js context. Going with #675 seems better, see my comment: #675 (comment)

[newpavlov]

It also makes sense for experimental targets (e.g. see the recent PR for Ariel OS). Such targets could use this feature in their respective "sys" crates.

Technically, I believe we should view Web WASM as such experimental target, despite its popularity.

[Pauan]

@newpavlov Technically, I believe we should view Web WASM as such experimental target, despite its popularity.

In a very strict technical sense, yes... but it is a very stable target, things are not breaking on a weekly basis (like they used to back when it was bleeding edge). In a practical sense it is stable, well supported, and widely used.

The lack of a dedicated wasm32-web target is an unfortunate historical quirk, but it has mostly worked out okay thanks to the existence of WASI.