Single error type

[newpavlov]

Currently we have two error types: getrandom::Error and rand_core::Error. To simplify things a bit it was proposed to leave only one error type. It can be done using one of the following approaches:

1. rand_core uses getrandom::Error:
   1a) getradom is an optional dependency and try_fill_bytes is feature-gate behind getrandom feature.
   1b) getrandom is a mandatory rand_core dependency.
   1c) Introduce additional feature gate for try_fill_bytes which will enable getrandom.
2. getrandom depends on rand_core, we change rand_core::Error definition to getrandom::Error.
3. We introduce yet another crate rand_error or something.

Of course we also can leave everything as-is. Personally I am inclined towards variant 1a, since we already plan to use getrandom for SeedableRng.

Thoughts?

[dhardy]

I think we already ruled out option 2? getrandom is supposed to be a minimal system interface, appropriate for embedding in std. I don't think option 3 helps much either.

So, in my opinion, if we unify the error types, we should choose a variant of 1, and minimise breakage (which probably means 1b).

The next question should be do we want to unify the error types? My main concern here is breakage, though considering the current rand_core error types is a little problematic, some breakage is IMO tolerable. It would be nice to see an implementation if that's not too much effort.

[newpavlov]

I will try to draft approach 1 then.

1b may feel a bit weird for RNG implementations, as e.g. on Linux rand_chacha will depend on libc. Same goes for rdrand crate.

[dhardy]

Are there good alternatives?

Doing as you suggest and making try_fill_bytes optional would in effect create two variants of the RngCore trait; in theory it should work but may be surprising. It also means that fallible RNGs still need to depend on getrandom.

An alternative would be to make the getrandom function within getrandom feature-gated. This sounds weird but might be a bit less weird if integrated into std.

However, it may be that the best option is to have an independent Error type which is easily converted? It could even be the exact same type (copied); conversion would still need to use .into() but I don't see this as a big deal.

[newpavlov]

Hm, so we have to choose between duplication of Error code and essentially forcing all RNGs to depend on getrandom even if they are not related to OS RNG in any way. For PRNGs it's less weird, since we can argue that it's for random seed initialization, but it does not look nice with hardware RNGs like rdrand. I am personally more inclined to the first option, though the second is not that bad as well.

The problem with feature-gating getrandom in getrandom crate (apart from the obvious weirdness) is that IIRC feature-gates in Cargo.toml do not work correctly right now, so getrandom will still depend on libc even with disabled feature.

[dhardy]

Then it sounds like we're agreed that duplication of error code is the best option, thus leaving us the question of whether and how to revise the rand_core::Error type.

Note that if we make the two behave identically we can still merge the types in the future — in fact, we could make rand_core optionally depend on getrandom and re-define the error types only if not using getrandom.

[newpavlov]

Looks like there is another reason not to do unification: Debug and Display impls of getrandom::Error use getrandom as part of the output.

[vks]

To me it seems best to have two error types, and maybe provide From impls behind a feature gate.

[dhardy]

Agreed. We need rand_core to (optionally) depend on getrandom in order to provide those From impls, so I think there's no reason not to also move from_entropy over (unless we remove it).