rand_core::Error new version

[newpavlov]

A slightly modified variant of approach proposed in #738.

Unfortunately with this approach we lose an ability to differentiate between retryable errors. We can bring it back by returning ErrorKind field or by replacing it with retryable flag, but I am not sure if it's worth the trouble.

[dhardy]

Still have to think about this. Stripping out the little-used kind feature does allow some simplification. Stripping out the msg does reduce the size of the type, but last time I looked at the effect of error-type size on performance I saw no effect (I may have missed something, but without evidence for this I don't believe we should concern ourselves much with the size).

This is a big breaking change for anyone doing almost anything with Error aside from Error::from and the std::error impl, so we should think carefully before merging (I don't know if it's possible to use Crater to test this).

[dhardy]

Is it worth having two separate examples? The one below does demonstrate direct usage of next_u32.

[newpavlov]

Well, I wanted to remove the second example to keep this file more in sync with rand_os, but I thought you added reseeding example for a reason. So if you don't want to keep it I will remove it.

[dhardy]

It is one of the major use-cases for OsRng, hence I showed both.

[newpavlov]

In the example below you demonstrate u32 generation using StdRng, not OsRng.

[dhardy]

Both, actually. Maybe it's not clear enough.

[newpavlov]

I will return msg field a bit later. I thought that it's a bit redundant (mainly because we don't have a good way to generate nice messages from getrandom::Error) and reducing size was a nice bonus, but on a second thought it may be worth to return it, and write something like this:

pub struct Error {
    msg: &'static str,
    #[cfg(not(feature="std"))]
    code: ErrorCode,
    #[cfg(feature="std")]
    cause: Box<dyn std::error::Error + Send + Sync>,
}

As for breakage, I thought you will publish rand_core v0.5 with getrandom changes (also using this opportunity it will be possible to move block stuff out), but if not, then this PR can be postponed/closed. Though in this case #738 will stay open as well.

[vks]

also using this opportunity it will be possible to move block stuff out

Where should it be moved? It is nice to have when implementing RNGs, so I think it makes sense in rand_core.

[newpavlov]

Where should it be moved?

To a separate crate. To me those block utilities look similar to how block-buffer relates to digest. Yes, most hash functions (but not all!) use block-buffer, but I prefer to divide traits and helper utilities (though there is a dirty impl_write! workaround in the digest crate, so I am not a total purist in this regard). I do not insist on doing it, simply expressing my preferences.

[dhardy]

mainly because we don't have a good way to generate nice messages from getrandom::Error

Maybe we should make getrandom::Error::msg public. That returns Option<&'static str>.

Yes, this would require bumping the rand_core version, but that is not sufficient justification for a major breaking change in a widely used crate. Actually, I'd like to know how many downstream crates this would affect.

I don't know that we need more micro-cratering. We could put the block stuff behind a feature-gate, perhaps, but I think most users of rand_core will need it anyway (because I think most users will either be using crypto-rngs or using the main Rand crate for its algorithms).

[newpavlov]

Maybe we should make getrandom::Error::msg public.

The problem here is that it is not extensible (i.e. you can't add message for your custom system entropy source). We may add get_error_message(e: Error) -> &'static str function, which will be overwritable together with getrandom, but I am not sure if it's worth the trouble.

[dhardy]

Looking at how the new error type affects our code, it appears sufficient, other than one place it would be nice to include an extra message.

It isn't ideal however; e.g. in no_std mode I think it will only be able to output an error code, not a message.

[dhardy]

I think we want this (and it's also not incompatible with the current Error type).

[dhardy]

It would be nicer not to have to use a cfg here (e.g. make from_code work either way). That means the std feature is also not needed on this crate.

[dhardy]

In theory it's nice to be able to include an extra message here (error manifesting and cause), but not critical.

[dhardy]

Both, actually. Maybe it's not clear enough.

[dhardy]

As before: shouldn't need the cfg here.

[dhardy]

This still needs deciding. My current thoughts are that since these changes aren't actually fixing anything and the current Error type isn't too bad, it would be better to leave it alone than tweak it.

[newpavlov]

these changes aren't actually fixing anything

What about #738? As I've wrote earlier you can close/postpone this PR if you are unwilling to publish v0.5 or break backwards compatibility promise a bit. We probably could've kept backward compatibility if kind and msg fields were private by deprecating existing methods, but unfortunately it's not the case.

My current thoughts on ideal internal structure of rand_core::Error type: I think error code and &'static str messages are mutually redundant. If we use error codes, then we can define a function which will map codes to messages (this function even could internally use a pointer to an array, which can be overwritable by user code), and if we have error messages then codes are not needed, as we can match on messages if needed. Error codes have smaller size (4 bytes vs. 2 words) and IIUC will allow dead code elimination to remove error messages if they are not used anywhere in the code, but on the other hand they will require slightly more code for mapping infrastructure. If error messages will be chosen over codes, then we could use the same approach I've used for codes for collapsing everything into one field. As for ErrorKind I agree with @vks and I also don't see much use for it.

BTW removing block stuff from rand_core will mean that we will not need serde1 feature anymore in rand_core.

[dhardy]

I believe you are correct about this fixing #738; for some reason I thought the fundamental problem was TimerError not implementing std::error::Error.

Semver allows breaking changes in minor versions (before 1.0) and in major versions; this isn't the issue. The issue is simply how much work needs to be done to fix downstream crates each time they bump the rand_* version.

---

I think error code and &'static str messages are mutually redundant.

Mapping is difficult to do in an extensible way, as you already noted for getrandom. Since this is the only error type RNGs can report, extensibility should be considered.

Since the contents of error messages are not part of the API, I wouldn't advise anybody to match against them. However, I've yet to see any good reason to match against the error code anyway, hence I see no need to support this.

The kind was never intended to be an error code in this sense. However, as you noted, it doesn't seem to have much purpose since there aren't enough "recoverable errors" to worry about and blocking is more sensible than returning "NotReadyYet". I don't object to its removal, besides more breakage (perhaps this part is best removed anyway).

---

About the "cause": this is supposed to be an optional extra detail, not the sole detail (e.g. in ReadRng; probably the message should be changed to "ReadRng failed", but still followed by the cause).

Suggestion:

/// Error type of random number generators
#[derive(Debug)]
pub struct Error {
    /// The error message
    pub msg: &'static str,
    #[cfg(feature="std")]
    cause: Option<Box<stdError + Send + Sync>>,
}

impl Error {
    /// Create a new instance, with specified message.
    pub fn new(msg: &'static str) -> Self {
        #[cfg(feature="std")] {
            Error { msg, cause: None }
        }
        #[cfg(not(feature="std"))] {
            Error { msg }
        }
    }
    
    /// Create a new instance, with specified message, and a chained cause.
    /// 
    /// Note: `stdError` is an alias for `std::error::Error`.
    #[cfg(feature="std")]
    pub fn with_cause<E>(msg: &'static str, cause: E) -> Self
        where E: Into<Box<stdError + Send + Sync>>
    {
        Error { kind, msg, cause: Some(cause.into()) }
    }
    
    /// Take the cause, if any. This allows the embedded cause to be extracted.
    /// This uses `Option::take`, leaving `self` with no cause.
    #[cfg(feature="std")]
    pub fn take_cause(&mut self) -> Option<Box<stdError + Send + Sync>> {
        self.cause.take()
    }
}

(Or make msg private.) This is minimally disruptive but still removes kind and the no_std version of with_cause.

It's worth noting that std::error::Error::cause has been deprecated in favour of source; since we're bumping the compiler version anyway we can switch to the new trait.

[dhardy]

Okay, my thoughts on this. As you mentioned...

https://xkcd.com/1024/

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.

As discussed, there may be sufficient value in minimising the size of the error type to avoid more than a single pointer. This doesn't mean we need to exclude details like the error message however; IMO it would be better to double-Box to include this.

Of course, we should learn our lessons from #738 and from the breakage we now need: the API should, as far as possible, not change between std and no_std configurations (although behaviour might), and we shouldn't have pub fields. I suggest:

pub fn new(msg: &'static str) -> Self;
pub fn with_code(msg: &'static str, code: NonZeroU32) -> Self;
pub fn with_cause<E>(msg: &'static str, cause: E) -> Self;

[cfg(feature="std")] pub fn take_cause(...;
pub fn msg(&self) -> &str; // may return a default string with `no_std`
// possibly also fn code(&self) -> Option<NonZeroU32> ?

The problem is that this doesn't solve #738. I don't like the proposed solution because it forces crates like rand_jitter and now rand_os to use a cfg. I wonder if we could have a generic version (like the current no_std version of with_cause) and use specialisation to capture the cause only when this lib and the error source both support std?

[newpavlov]

Closed in favor of #800.