Assume JS on wasm32-u-u with `wasm_js` feature?

[dhardy]

As noted in #346, we could support wasm32-unknown-unknown on web when the js feature is enabled. The major problem is was (see next comment) what to do when the custom backend is also enabled:

• Defaulting to the custom backend is not okay: any crate in the dependency tree could overwrite the (secure) random source used by a binary. No longer true: the user must the the getrandom_backend cfg.
• Defaulting to the js backend is not okay: many crates depend on getrandom with the js feature for convenience, which would prevent support for a custom backend (no longer true; see next comment).

Updated suggestion in next comment.

Original suggestion

We could instead consider this a compiler error. This deviates significantly from standard usage of features which should be additive (i.e. any combination including all of them is valid).

Nevertheless, this might be workable. Any crate which could possibly be used on a non-web wasm32-unknown-unknown platform depending on getrandom with feature js would be a bug, forcing these crates not to do so (or to explicitly not support non-web wasm32-u-u targets).

This is (I believe) the simplest solution for wasm32-u-u which doesn't block getrandom from supporting non-web targets.

[dhardy]

The above is somewhat incorrect: since v0.3 we have a wasm_js feature but don't have a custom feature. Instead we use the getrandom_backend cfg to opt-in to the custom or other opt-in backend.

There is then no conflict if we assume JS on wasm32-unknown-unknown without the getrandom_backend cfg being set.

The problem is that we would violate "wasm32-unknown-unknown should not be special-cased" (#671). What are the real consequences of this?

1. A run-time error of some kind if it turns out that the target is not wasm32-unknown-unknown (edit: this was supposed to say JS / web crypto). This may be acceptable.
2. "This breaks builds for users who target non-Web WASM." Except, I don't believe it does: these users can still set the getrandom_backend cfg.
3. "Even worse, just having wasm-bindgen in project's dependency tree can be problematic in such cases". This may be referring to this post; the issue was actually the size of the Cargo.lock. The wasm_js feature flag will still control whether wasm-bindgen gets added here. (This implies that libraries should not enable wasm_js on getrandom unless they are reasonably sure users do want the extra bloat, but honestly, that's their problem not ours.)
4. If we had another backend for wasm32-unknown-unknown, this would prevent a similar solution there. But we don't. Given that the JS (web crypto) backend is our only backend for this platform (with the only alternative in this crate's six year history being for Node.js, removed in wasm_js: remove the separate codepath for Node.js and TLS caching #557), it seems clear what the most important backend for this target is.

So, actually, I think that the consequences are smaller than the benefit (make getrandom easy to use in the browser with the wasm32-u-u target).

[dhardy]

@josephlr @newpavlov @briansmith do you see any major issues here now that custom is not a feature (and thus overriding the backend is still possible)?

Follow-up question: would this be considered a breaking change? I don't see why it should be.

[bushrat011899]

I think this should equally apply to wasm32v1-none, but otherwise I think this is the smallest change that would address the general concerns of the web users.

[josephlr]

I think we should do this, and I'll start work on a PR. Of course, I'll want input from @newpavlov before we make any changes.

@josephlr @newpavlov @briansmith do you see any major issues here now that custom is not a feature (and thus overriding the backend is still possible)?

We may want to emit a warning if wasm_js is provided as a feature but not provided via --cfg getrandom_backend=wasm_js. That way if a user doesn't intend to use wasm_js they get some breadcrumbs pointing in the correct direction.

Follow-up question: would this be considered a breaking change? I don't see why it should be.

I personally would not consider this to be a breaking change as any non-JS users of wasm32-unknown-unknown (or wasm32v1-none) would need to specify a backend to get anything working regardless.

I think this should equally apply to wasm32v1-none, but otherwise I think this is the smallest change that would address the general concerns of the web users.

I also agree, we should continue doing what we currently do, and check the target with

#[cfg(all(target_arch = "wasm32", any(target_os = "unknown", target_os = "none")))]

to treat both targets (and any future targets) in the same way.

There is then no conflict if we assume JS on wasm32-unknown-unknown without the getrandom_backend cfg being set.

The problem is that we would violate "wasm32-unknown-unknown should not be special-cased" (#671). What are the real consequences of this?

1. A run-time error of some kind if it turns out that the target is not wasm32-unknown-unknown. This may be acceptable.

Due to how feature unification works (and the lack of support for rust-lang/cargo#1197), we will need to handle the case where wasm_js is specified on a non-Wasm target (like x86_64-unknown-linux-gnu). We should probably handle this the same way we do today, i.e. do nothing if the feature is enabled on an irrelevant target.

2. "This breaks builds for users who target non-Web WASM." Except, I don't believe it does: these users can still set the getrandom_backend cfg.

Agreed, the main consequence is that non-Web users of these targets get a worse error message, but that's a small pain point (especially if we have documentation to help out).

3. "Even worse, just having wasm-bindgen in project's dependency tree can be problematic in such cases". This may be referring to this post; the issue was actually the size of the Cargo.lock. The wasm_js feature flag will still control whether wasm-bindgen gets added here. (This implies that libraries should not enable wasm_js on getrandom unless they are reasonably sure users do want the extra bloat, but honestly, that's their problem not ours.)

Agreed. Issues of dependency bloat are dealt with via the wasm_js feature, and are orthogonal to backend selection.

4. If we had another backend for wasm32-unknown-unknown, this would prevent a similar solution there. But we don't. Given that the JS (web crypto) backend is our only backend for this platform (with the only alternative in this crate's six year history being for Node.js, removed in wasm_js: remove the separate codepath for Node.js and TLS caching #557), it seems clear what the most important backend for this target is.>

Also, even if we need Node-specific code, the detection of Web vs Node can be done at runtime, so we don't need an additional backend in that case.

So, actually, I think that the consequences are smaller than the benefit (make getrandom easy to use in the browser with the wasm32-u-u target).

Very well put, thanks for writing all this out!

[dhardy]

We may want to emit a warning if wasm_js is provided as a feature but not provided via --cfg getrandom_backend=wasm_js. That way if a user doesn't intend to use wasm_js they get some breadcrumbs pointing in the correct direction.

Most likely this situation will be encountered sometimes due to some library enabling the wasm_js feature (where arguably the library shouldn't have since it didn't know the target), so I don't know that warning will be useful.

Unless you are implying that users should still specify the getrandom_backend cfg. My intention was that this should not be considered necessary.

[bushrat011899]

Unless you are implying that users should still specify the getrandom_backend cfg. My intention was that this should not be considered necessary.

I don't see any issues with encouraging users to still use the RUSTFLAGS via a warning (although I'm not sure how to emit a compiler warning ). The issue right now is a user must configure it before compilation will succeed, which is annoying in CI and a bad experience for web developers trying Rust.

[newpavlov]

this situation will be encountered sometimes due to some library enabling the wasm_js feature

This is my main concern and why I dislike this approach. We already had this experience with the old js feature where some (even purely computational!) crates unconditionally enable the feature for "convenience" and I do not want to return to it. Yes, we could argue that it's not our fault, but I would prefer to not have this feature in the first place.

Personally, I would prefer to merge #672 and introduce a separate getrandom-js crate (or, ideally, ask js-sys developers to expose the custom symbol behind a feature) and deprecate the wasm_js feature.

[dhardy]

Personally, I would prefer to merge #672 and introduce a separate getrandom-js crate (or, ideally, ask js-sys developers to expose the custom symbol behind a feature) and deprecate the wasm_js feature.

These solutions make getrandom less opinionated at the cost of not supporting an alternative (custom) backend, and at the cost of worse error messages for build errors.

[newpavlov]

These solutions make getrandom less opinionated at the cost of not supporting an alternative (custom) backend, and at the cost of worse error messages for build errors.

I agree about worse error messages in the case of the feature being enabled without proper implementation, but I am not sure what you meant with "not supporting an alternative (custom) backend". We can still support getrandom_backend="custom" and make it higher priority than the fallback backend (it would involve introducing different symbol names for the fallback backend, but it's not a high cost to pay).

[dhardy]

it would involve introducing different symbol names for the fallback backend, but it's not a high cost to pay

That sounds like a different proposal than #672 which also has special wasm32 handling.

In the interests of simplicity (less code, more robust), I still prefer this solution (#675) or even #674 (though that needs testing — maybe we should just do that, since the solution can be entirely orthogonal to and compatible with any other solution, and abandoned later if it doesn't work out).

[newpavlov]

That sounds like a different proposal than #672 which also has special wasm32 handling.

Please take a look at it again. It does not have any special wasm32 handling. I do not suggest to merge it in the current form, but I prefer its general idea more than #674 and #675.