`rand_core:0.5.0::Error` has no raison d'etre

[nagisa]

The rand_core:0.5.0::Error is reduced down to an integer code, which has exactly as much value as no error at all. The stored integer carries no information whatsoever (esp. in generic contexts) and the error, if Displayed conveys no useful information either.

Consider for example:

fn foo<R: RandCore>(rng: R) {
    match rng.try_fill_bytes(...) {
        Err(e) => {
            // In this context "error code 42" is about as useful as 
            // "an error has occurred" or "bad things happened"
            //
            // Furthermore, each implementation of `RngCore` can map 
            // error numbers to causes in arbitrary ways, which makes 
            // whatever interpretation that could be derived in this generic
            // context misleadingly wrong, even.
        }
    }
}

With that in mind, the next version should reconsider this API change.

[dhardy]

Thanks @nagisa; great to see your input to our long discussion of error types: #800, #771. Your comments appear to be exclusively about the no_std variant of Error. But I'll let you read through those threads before repeating anything.

[nagisa]

Your comments appear to be exclusively about the no_std variant of Error.

Indeed. Usable no_std Error is what I care about the most. std Error seems fine… not that I’ve had a chance to deal with it yet, but many other crates do a similar thing and that seems to work fine.

---

long discussion of error types: #800, #771.

I have seen these issues, but haven’t read through them too thoroughly. I notice that this same issue was brought up at least once by yourself, and never really addressed:

We should pay some attention to reporting meaningful errors, not merely forwarding a code from somewhere. We should at least do this when using std; for no_std the best trade-off might be to only forward a code, or maybe we should piggy-back the log feature (or another) to add details.

I haven’t seen any exploration into usability of this trade-off, except for this small blurb (also by yourself):

On no_std this is a pure numeric code, though later with distributed slices we could potentially include error messages (though no_std often means limited memory, so also potentially not).

“Distributed slices” came up multiple times as a "solution” for no_std, but I’ve no idea what that even means. It is the first time I hear about them. log was another considered option which in my eyes is an equivalent of print-debugging, now baked into a foundational library. rand_core does not provide a list of conventional codes either (if it did, the error code should be an enum instead, bringing us back to ErrorCause).

I have a feeling that this change was made with an understanding that actual cause of the error is not that useful – transient failures are better retried internally and non-transient ones cannot be recovered anyway; this probably makes sense given that various next_* APIs do not return an error either way. I do not necessarily object to that view, but I am still unable to see the point of the error code inside Error. It will only carry useful information in situations where the RandCore as an abstraction is irrelevant.

I also saw a mention somewhere that users won’t have to care about errors to the point of knowing that an RNG failed in some way. I disagree with that, and even if I agreed, I do not see any additional value that an error code would bring here. To see why compare these hypothetical, but fairly typical error traces, as one would produce when using failure crate in their project:

With error code

error: Could not download a file
     caused by: Nonce could not be generated for an encrypted session
     caused by: RNG error code 42

Without error code

error: Could not download a file
     caused by: Nonce could not be generated for an encrypted session
     caused by: RNG error

[dhardy]

No, I hadn't heard of distributed slices before, and I remain a little sceptical considering we don't have a portable version yet, but we do have a limited implementation: https://github.com/dtolnay/linkme . @newpavlov's idea is that crates can each add their error messages to a [(NonZeroU32, &'static str)] slice shared by the whole application.

I have a feeling that this change was made with an understanding that actual cause of the error is not that useful – transient failures are better retried internally and non-transient ones cannot be recovered anyway;

This was the motivation for the previous design with ErrorKind — but we did not find much use for this in practice, and expecting users to write their own code for retry on interruption (ErrorKind::Transient) was not particularly helpful. Therefore this does not exist any more — we handle retries ourselves where sensible (in getrandom and ReadRng) instead of returning a "transient" error.

What type of RNG are you imagining where an error occurs more often than once-in-a-blue-moon? RDRAND already retries in a loop per design spec. The one case of RNG failure I know of was OsRng failing on Windows (7?) due to a DLL issue — this is a std platform however.

Ultimately, I agree that we should have a solution, and I'm not confident that distributed slices will be available "soon" (@newpavlov?).

[hdevalence]

What problem is this actually solving? Your proposed solution (distributed slices) requires either language changes or "linker shenanigans"; what benefit does this actually give to an RNG user when they hit an error?

[newpavlov]

The rand_core:0.5.0::Error is reduced down to an integer code, which has exactly as much value as no error at all.

I strongly disagree. For end-user having some information about error is better than having none, plus usually you know which RNG you use, so you will be able to check this error code in RNG documentation. This design was heavily influenced by development of getrandom crate, but I think it also really well fits constraints of no_std applications: has a small size, trivially extensible, error descriptions can be removed from binary by dead code removal if they are not used anywhere.

What problem is this actually solving? Your proposed solution (distributed slices) requires either language changes or "linker shenanigans"; what benefit does this actually give to an RNG user when they hit an error?

Distributed slices are only needed for nicer error messages, so instead of RNG error code 42 you would get RNG error: quantum field instability.

[burdges]

Any errors an Rng might generate are either os errors, including special hardware errors, or stream cipher length errors, yes?

There is an interesting research problem of doing a non-locking reentrant PRNG, but ideally such a beast should not produce any error either. In essence, the Rng would be a smart pointer with their own unique secret seed that prevented accesses from different threads from inverting one another.

[nagisa]

plus usually you know which RNG you use

Is usually good enough for a foundational generic interface? If "usually" is good enough, then the whole concept of the RngCore trait is starting to lose its meaning – you could just use the API of the concrete RNG type after all.

EDIT: I do agree that if you do know the concrete RNG type, then it is definitely possible to communicate extra information back to the user. However, if those scenarios were all that we needed to handle, they would be handled equally well by an associated Error type on RngCore. With that alternative on the table, a numeric code is just a bad API design.

Any errors an Rng might generate are either os errors, including special hardware errors

Not sure if I would consider hardware errors the same as OS error – hardware isn’t always uniformly abstracted by an OS.

[newpavlov]

Your claim was:

integer (error) code <..> has exactly as much value as no error at all

I find it's really strange that you find some information exactly equivalent to no information at all. Even in generic context we are able to match on error codes (e.g. Error::UNKNOWN or Error::UNAVAILABLE), in future we also could add additional codes or even error prefixes for common error classes.

As for using associated error type, in my experience they are less convenient to handle in generic contexts. Yes, we could define rand_core::Error trait and use associated error type in RngCore bound by this trait, but it looks like an over-engineered solution to me (plus, it will not be object-safe).

What exactly do you want to be able to do with rand_core::Error in generic contexts? The only potentially useful thing I can remember is an ability to distinguish retryable vs non-retryable errors. Previously I proposed adding is_retryable method, but no one has expressed interest in it.

[nagisa]

Your claim
...
I find it's really strange that you find some information exactly equivalent to no information at all

With what’s available in 0.5.0 I stand by that claim. Some information will exist in an integer, sure, but without means to interpret the number this information provides no knowledge and therefore no value can be derived.

And just to reiterate, "By knowing that I’m using crate rand_lava, and looking at the source code, I now know that error code 42 occurs when somebody unplugged the lava lamp” is not a satisfactory approach to error code interpretation.

Even in generic context we are able to match on error codes (e.g. Error::UNKNOWN or Error::UNAVAILABLE)

So far I’m not seeing either Error::UNKNOWN nor Error::UNAVAILABLE or any other codes to compare against anywhere in the public API of rand_core. Am I just blind? Why does Display not format those into sensible textual descriptions of the error?

What exactly do you want to be able to do with rand_core::Error in generic contexts? The only potentially useful thing I can remember is an ability to distinguish retryable vs non-retryable errors. Previously I proposed adding is_retryable method, but no one has expressed interest in it.

I’ll be happy if I’m able to:

1. Format the error into a user-readable and interpretable description of the cause behind rand_core::Error at least for the most common error situations (i.e. “random number generator is unavailable” and not “error code 711”) that has been returned by arbitrary implementor of the RngCore trait; while
2. Having the ability to quickly and easily specify these common situations as an implementor of the RngCore trait.

At the most basic level I’d be content with the aforementioned

impl Error {
    const UNAVAILABLE: _ = ...;
    const TRANSIENT: _ = ...;
    const etc...
}

but I would also find that to be fairly clunky to work with as far as APIs in Rust go.

[dhardy]

So far our convention has been

// —— in getrandom::error ——
// A randomly-chosen 24-bit prefix for our codes
pub(crate) const CODE_PREFIX: u32 = 0x57f4c500;
const CODE_UNKNOWN: u32 = CODE_PREFIX | 0x00;
const CODE_UNAVAILABLE: u32 = CODE_PREFIX | 0x01;

// —— in rand_jitter::error ——
/// All variants have a value of 0x6e530400 = 1850934272 plus a small
/// increment (1 through 5).
pub enum TimerError {
    /// No timer available.
    NoTimer = 0x6e530401,
    ...

... and all our defined error codes could be considered "unknown" or "unavailable".

We could tweak this and assign meaning to different parts of the code (e.g. 16 bits for a non-zero crate identifier, 8 bits for a "status" code and 8 bits for lib-specific details), then provide functions like fn is_unavailable(&self) -> bool which operate on the status bits for no_std.

But: it's a breaking API change for both no_std and std and still doesn't provide user-readable error messages.

To get messages, I think the only options for no_std are (1) &'static str, (2) distributed slices or (3) some ugly hack using static memory (set_err_msg(&'static str)).

[newpavlov]

I think @nagisa means that those constants should be duplicated in the rand_core with an appropriate Debug and Display impl modifications. I don't have objections against it.

Format the error into a user-readable and interpretable description of the cause behind rand_core::Error

Note that with an enabled std feature you will get a meaningful error description instead of bare error code for some OS errors (see this part and usage of error_msg_inner in getrandom). The same approach can be used by RNG crates, with an enabled std feature they can use Error::new and without it they can fall back to bare error codes.

[newpavlov]

@josephlr

I has just assumed that the RngCore trait would just have a type Error parameter, and each generator would chose its own type. is there a reason things are not done that way?

With this approach RngCore (and IIUC as a consequence Rng trait as well) will not be object safe anymore. If we want to query additional information about error, then we will have to introduce an additional rand_core::Error trait. To me it looks a bit too complicated compared to the current concrete type which in future can be extended in a backwards compatible way, this is why I've dropped this idea during discussions of Error type redesign and it was not brought by anyone else.

One advantage of associated error type would be an ability to use ! (or uninhabited type for now) to indicate impossibility of RNG failure. But I am not sure if it's worth losing object safety and simplicity of concrete type. Also we will have to release rand_core v0.6 and bump minor versions for other rand crates.

[dhardy]

IIRC there was some difficulty providing a blanket implementation of the following type due to the lack of specialisation and Rust enforcing no overlapping impls.

impl<E: Into<Error>, R: RngCore<E>> RngCore<Error> for R { ... }

Thus not only does using an associated type make RngCore not object-safe, it means no object-safe version of RngCore can be implemented automatically for all RNGs. (I guess though we could have used a separate ObjSafeRngCore trait.)

[burdges]

If you need a separate trait, then maybe make the rarely used trait the not object safe one, ala

pub trait TryRngCore {
    type Error;
    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>;
}

I've no idea if anyone wants try_fill_bytes on an object safe trait, but if so then one could always imagine the completely crazy "not my problem" version. lol

pub trait RngCore {
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
    fn fill_bytes(&mut self, dest: &mut [u8]);
    fn try_fill_bytes<E>(&mut self, dest: &mut [u8]) -> Result<(), E>
    where E: for<'a> Into<&'a dyn ErrorTrait>;
}