Revise rand_core::Error (alt #771)

[dhardy]

This is an alternative to #771.

This does not currently make any attempt to reduce the size of Error; we could use a second Box around internals for std (or even alloc) mode and omit the msg in no_std mode. If this does offer performance improvements, I expect these will only be in isolated cases, therefore it may be best to use a feature flag (e.g. perf_error_size) to implement this (there are no breaking changes other than msg and fmt having different results).

We can avoid adding a std feature to rand_os as in #771 by using From<getrandom::Error>. (We could use a special function including a message, but I don't think there's much point.)

Additionally, this moves the from_entropy function as previously discussed and changes a few docs (though it appears several doc links are broken; this will need fixing later).

Any comments @vks @newpavlov @burdges / everyone?

[vks]

Why is this coupled to getrandom?

[dhardy]

Do you mean we should support std without getrandom? We could try, but (1) getrandom supports (pretty-much) any std platform anyway and (2) I'm not sure Cargo allows us to imply getrandom/std only when both std and getrandom are enabled.

[newpavlov]

At a brief glance this code looks broken. Enabling std feature will break builds for all crates which use with_code method. At minimum you have to use the same workaround with private ErrorCode as was done in #771.

I wanted to draft an RFC for distributed slices first, which not only will neatly solve error code to error message conversion issue, but also will be really useful for other use cases as well. But I couldn't find time to work on it.

Personally I like approach in #771 a bit more. Not only it results in a smaller footprint for Error type, but also feels less redundant as you don't have to pass msg. Yes, there is an issue with extending error messages, but I think for the time being we can simply define a number of constants for general error types which other crates can use until introduction of distributed slices. For OS errors we may add some possible error codes for supported platforms to the error const slice.

[vks]

Why do we need this?

[dhardy]

The current no_std code uses a code, but one isn't always provided. This is in line with what happens in getrandom, but may not be optimal.

[dhardy]

Thanks @newpavlov for the response. I like the idea, but even with distributed slice support it wouldn't be perfect: it relies on all other error sources not forgetting to register their messages, and it only records the origin of the error, not where it manifests. (Though knowing something like OsRng failed with a chained cause is not especially useful, and often enough when encountering an error, the top-level message gets printed but not all chained causes.)

Also, we cannot rely on a future RFC. We could of course just return numeric codes for now but this isn't ideal (also because distributed slices might never happen or never be portable enough).

As mentioned, we can make the Error footprint smaller here by double-boxing and dropping the msg in no_std — though this could mean no details at all on no_std, so your approach may be better (by forcing all potential no_std dependencies to use a code).

I disagree that the msg is completely redundant however, e.g. check ReadRng and JitterRng errors (this could be worked around by expanding the TimerError description strings and including similar strings for ReadRng errors, along with error codes for each).

[newpavlov]

As mentioned, we can make the Error footprint smaller here by double-boxing and dropping the msg in no_std

Personally I think double boxing is not too important, usually 2 words instead of 1 word is not a deal breaker for std targets. Either way it will be possible to use double boxing later in a backwards compatible way, so we can postpone this decision.

If msg will be dropped for no_std, I don't see any reason why we should keep it for std. Box<dyn Error> is significantly more powerful than &'static str, so msg field seems redundant.

[dhardy]

I wasn't proposing dropping msg for std; I was proposing a feature flag to optionally drop it.

You are right, Box<dyn Error> is more powerful. But your proposed model also has drawbacks:

• reporting errors in a no_std compatible way means only using a NonZeroU32 code, which means that until we get distributed slices there will be no error messages and also means that all error sources need to define error numbers
• reporting errors with more information means creating a use-specific error type
• having messages on std and no_std compatibility means implementing both systems behind cfgs until those distributed slices are available (if ever)

So in my opinion there is still a lot to be said for a more compatible system using &'static str (though probably I should copy your ErrorCode struct).

[dhardy]

Alternative on the above that probably makes more sense if we keep msg as a required constructor argument: omit the code and keep the message (but this requires retrieving the message from getrandom::Error).

It comes back to the discussion we had for getrandom's error type: do we prefer a code or a message. For consistency's sake, maybe we should break with Rand's tradition and use a code instead of a message (but I don't like the idea of only being able to report a "meaningless" code for no_std).

[dhardy]

It seems we have two options:

Option A is @newpavlov's model:

pub struct Error {
    #[cfg(feature="std")]
    inner: Box<std::error::Error + Send + Sync + 'static>,
    #[cfg(not(feature="std"))]
    code: NonZeroU32,
}

On std, this uses the most flexible option: any boxed Error trait object (though not the most convenient: e.g. ReadRng should probably have its own error type in this case). 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).

Option B is a cut-down version of my model:

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

Here, we can support a simple fn new(msg: &'static str) -> Self constructor on both std and no_std, along with an optional cause (std only). We'll have to tweak getrandom a bit for compatibility. Disadvantages are less flexibility (probably not a big deal considering RNG errors are not something you usually need to show users) and obligatory strings with no_std (potentially larger binaries).

Why not other options: msg and code are redundant. If we're using error strings on no_std, we may as well use them on std too. Conversely, if we're using error codes on no_std, we should support them on std (via the ErrorCode struct), and then there's not much point having strings too.

Opinions (or even better evidence) please?

[newpavlov]

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).

Small addition: even without distributed slices we can define a static array as part of rand_core which will include some generic error codes (and/or prefixes), which other crates will be able to use. Also I think if conversion from error code to message is not used anywhere, then dead code elimination should remove (distributed) slice altogether, so this approach theoretically is truly "zero cost".

[dhardy]

I adjusted this to use @newpavlov's model, which it seems is the best option.

So long as CI passes, I think this is ready. Review please?

What's missing: currently rand_core::Error does not get error messages from getrandom (with no_std). This needs changes to getrandom and I don't want to address it now (also it's low priority).

[dhardy]

Good, the only CI failure is this.

[newpavlov]

LGTM! Though I do not quite understand the getrandom_package part.

Conversion of error code to messages can be done later. I think the easiest approach will be to define array of error codes/messages as part of rand_core (including selected cfged platform-specific codes), but indeed it's a low priority and can be done later in a backwards compatible way.

[dhardy]

Though I do not quite understand the getrandom_package part.

Optional dependencies can be used as feature flags, but I don't think it's possible to make them depend on anything transitively. I want getrandom to imply "rand_core/getrandom"; this is a hack to do that.

[newpavlov]

Ah, indeed. IIUC it should be fixed with rust-lang/cargo#5565 eventually.

[vks]

Doesn't this still have the issue that the error types are incompatible for different features, making the std feature non-additive?

[newpavlov]

No, with enabled std feature NonZeroU32 code gets wrapped into ErrorCode, so public API gets only extended with new methods and no_std part of API does not change in any way.

[dhardy]

Functionally this is the case (and the code method can no longer guarantee to have Some(NonZeroU32) result), but API changes are only additive.

[dhardy]

Great, sounds like we've finally solved this one (two steps towards 0.7)! 🎉

[burdges]

There are no concerns about the Error type itself being incompatible with different feature sets because that's always the case, right?

[dhardy]

Cargo unifies features for each crate in the build. The only concern should be a dependency using an API incompatible with the API derived from the feature specification; a dependency may use API additions that come with a feature flag it uses, but if it does not use those additions (or require the feature), it should be compatible both with and without the feature (hence features may only make additive changes to the API).