Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise rand_core::Error (alt #771) #800

Merged
merged 10 commits into from May 29, 2019
3 changes: 2 additions & 1 deletion rand_core/Cargo.toml
Expand Up @@ -18,10 +18,11 @@ travis-ci = { repository = "rust-random/rand" }
appveyor = { repository = "rust-random/rand" }

[features]
std = ["alloc"] # use std library; should be default but for above bug
std = ["alloc", "getrandom", "getrandom/std"] # use std library; should be default but for above bug
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this coupled to getrandom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

alloc = [] # enables Vec and Box support without std
serde1 = ["serde", "serde_derive"] # enables serde for BlockRng wrapper

[dependencies]
serde = { version = "1", optional = true }
serde_derive = { version = "^1.0.38", optional = true }
getrandom = { version = "0.1", optional = true }
73 changes: 47 additions & 26 deletions rand_core/src/error.rs
Expand Up @@ -9,13 +9,13 @@
//! Error types

use core::fmt;

#[cfg(feature="std")]
use std::error::Error as stdError;
#[cfg(feature="std")]
use std::io;
#[cfg(not(feature="std"))]
use core::num::NonZeroU32;


// A randomly-chosen 24-bit prefix for our codes.
#[cfg(not(feature="std"))]
pub(crate) const CODE_PREFIX: u32 = 0x517e8100;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Error type of random number generators
///
Expand All @@ -30,7 +30,9 @@ pub struct Error {
/// The error message
pub msg: &'static str,
#[cfg(feature="std")]
cause: Option<Box<stdError + Send + Sync>>,
cause: Option<Box<std::error::Error + Send + Sync>>,
#[cfg(not(feature="std"))]
code: NonZeroU32,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl Error {
Expand All @@ -40,39 +42,45 @@ impl Error {
Error { msg, cause: None }
}
#[cfg(not(feature="std"))] {
Error { msg }
Error { msg, code: NonZeroU32::new(CODE_PREFIX).unwrap() }
}
}

/// Create a new instance, with a message and a chained cause.
///
/// Note: `stdError` is an alias for `std::error::Error`.
///
/// If not targetting `std` (i.e. `no_std`), this function is replaced by
/// another with the same prototype, except that there are no bounds on the
/// type `E` (because both `Box` and `stdError` are unavailable), and the
/// `cause` is ignored.
///
/// This function is only available with the `std` feature.
// NOTE: with specialisation we could support both.
#[cfg(feature="std")]
pub fn with_cause<E>(msg: &'static str, cause: E) -> Self
where E: Into<Box<stdError + Send + Sync>>
where E: Into<Box<std::error::Error + Send + Sync>>
{
Error { msg, cause: Some(cause.into()) }
}

/// Create a new instance, with a message and a chained cause.
///
/// In `no_std` mode the *cause* is ignored.
/// Create a new instance, with a message and an error code.
///
/// This function is only available without the `std` feature.
#[cfg(not(feature="std"))]
pub fn with_cause<E>(msg: &'static str, _cause: E) -> Self {
Error { msg }
pub fn with_code(msg: &'static str, code: NonZeroU32) -> Self {
Error { msg, code }
}

/// Take the cause, if any. This allows the embedded cause to be extracted.
/// This uses `Option::take`, leaving `self` with no cause.
///
/// This function is only available with the `std` feature.
#[cfg(feature="std")]
pub fn take_cause(&mut self) -> Option<Box<stdError + Send + Sync>> {
pub fn take_cause(&mut self) -> Option<Box<std::error::Error + Send + Sync>> {
self.cause.take()
}

/// Retrieve the error code.
///
/// This function is only available without the `std` feature.
#[cfg(not(feature="std"))]
pub fn code(&self) -> NonZeroU32 {
self.code
}
}

impl fmt::Display for Error {
Expand All @@ -87,20 +95,33 @@ impl fmt::Display for Error {
}
}

#[cfg(feature="getrandom")]
impl From<getrandom::Error> for Error {
fn from(error: getrandom::Error) -> Self {
let msg = "getrandom error";
#[cfg(feature="std")] {
Error { msg, cause: Some(Box::new(error)) }
}
#[cfg(not(feature="std"))] {
Error { msg, code: error.code() }
}
}
}

#[cfg(feature="std")]
impl stdError for Error {
impl std::error::Error for Error {
fn description(&self) -> &str {
self.msg
}

fn cause(&self) -> Option<&stdError> {
self.cause.as_ref().map(|e| e.as_ref() as &stdError)
fn cause(&self) -> Option<&std::error::Error> {
self.cause.as_ref().map(|e| e.as_ref() as &std::error::Error)
}
}

#[cfg(feature="std")]
impl From<Error> for io::Error {
impl From<Error> for std::io::Error {
fn from(error: Error) -> Self {
io::Error::new(io::ErrorKind::Other, error)
std::io::Error::new(std::io::ErrorKind::Other, error)
}
}
3 changes: 2 additions & 1 deletion rand_os/src/lib.rs
Expand Up @@ -88,7 +88,8 @@ impl RngCore for OsRng {
// (And why waste a system call?)
if dest.len() == 0 { return Ok(()); }

getrandom(dest).map_err(|e| Error::with_cause("OsRng failed", e))
getrandom(dest)?;
Ok(())
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/rngs/os.rs
Expand Up @@ -60,7 +60,8 @@ impl RngCore for OsRng {
}

fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
getrandom(dest).map_err(|e| Error::with_cause("OsRng failed", e))
getrandom(dest)?;
Ok(())
}
}

Expand Down