Should the "custom" feature take precedence over the "js" feature?

[josephlr]

Currently the "custom" feature is last in our precedence order for selecting implementations. The original idea behind this was to ensure that a crate couldn't (accidentally or intentionally) change the RNG source for a program.

Sometimes folks are targeting wasm32-unknown-unknown, bu they are not targeting the web, but their deps are enabling the js feature. #342 CC: @nickray

I would propose the following new precedence order:

• If on a supported target, use the standard implementation
• If custom feature is enabled, use the custom implementation
• If on wasm32-unknown-unknown and js is enabled, use the wasm-bindgen implementation.

[newpavlov]

And what will happen when someone sets the custom feature (e.g. by erroneously adding a custom crate to [dependencies] instead of [dev-dependencies])? Enabling rdrand or js outside of binary crates is a clear misuse of the features, so such libraries should be fixed either way. Also, wouldn't it be technically a breaking change?

All in all, I am not strongly against the proposed reordering, but I can't say I like it either.

Ideally, we should use configuration flags instead of crate features (i.e. #[cfg(getrandom_rdrand)] instead of #[cfg(feature = rdrand)]). But my guess is that users who compile code for Web WASM will not be happy with such change...

[newpavlov]

Maybe we should raise compilation errors if more than one feature is used out of js, rdrand, custom list? We specify that those features should not be enabled in library crates outside of tests, so it should not happen in practice and if it does, then someone misuses the features.

[briansmith]

The answer and reasoning for JS is totally different for RDRAND. We should discuss the JS situation in its own issue. We should discuss the RDRAND vs Custom in #230.

[josephlr]

The answer and reasoning for JS is totally different for RDRAND. We should discuss the JS situation in its own issue. We should discuss the RDRAND vs Custom in #230.

Changed the name and description of this issue to just track the js vs custom issue.

Copying from #438

Ideally, we would have the custom feature take precedence over the JS implementation (#346), so even if the js feature is unconditionally enabled in a user's dependency graph (which is fairly common), a user in a non-web/node environment can provide their own implementation.

If we did this, my initial thought was that we might be able to get rid of the js feature entirely, and have the "JS" implementation be the default for wasm32-unknown-unknown while still allowing users to select their own implementation if they are using wasm32-unknown-unknown not in a JS environment (uncommon but possible). However, given some of the issues observed by the ahash crate (tkaitchuck/aHash#140, tkaitchuck/aHash#143, tkaitchuck/aHash#145), I'm not sure we can do this.

[briansmith]

I have been told there are some popular real-world uses of wasm32-unknown-unknown that aren't Web Platform(-ish) targets, so I don't think making crypto.getRandomValues the default for that target is the right thing to do.

Now, in the case where the user is targeting the web platform, then crypto.getRandomValues is the definitive system number generator, analogous to the getrandom syscall on Linux. So if we'd not allow overriding the system CSPRNG on Linux then why would we do it for the web platform?

Ideally we're replace the web platform use of wasm32-unknown-unknown with distinct new wasm32-unknown-browser triple which would make our lives much easier, but I don't see that happening any time soon. I think the current state of affairs is as close to the ideal as we can get.