Common error details, as @newpavlov's design #30

dhardy · 2017-10-31T11:19:21Z

Implement new design in #10.

Error should be 32 bytes in size (two enums padded for alignment + a fat pointer). It could potentially be reduced to 24 bytes by combining the two enums, but that would be ugly. Otherwise it could only be much smaller by omitting the details altogether. Hopefully the size doesn't make too much of a difference anyway; I don't know.

Most of the "details" implementation is hidden and "kind" is still not exhaustively matchable.

Doc here

pitdicker

Looks good.

Have you measured the type is 32 bytes?
Edit: it is.

pitdicker · 2017-10-31T11:57:57Z

rand_core/src/lib.rs

-        }
+        write!(f, "RNG {} (details: {})",
+            self.kind.description(),
+            self.details().unwrap_or("NA"))


We maybe should not print the details here, see rust-lang-deprecated/error-chain#154 (comment)

pitdicker · 2017-10-31T12:02:54Z

rand_core/src/lib.rs

-        self.cause.as_ref().map(|e| &**e)
+        match self.details {
+            ErrorDetails::Error(ref e) => Some(&**e),
+            // unforunately we cannot report &str cause here


I think we can if ErrorDetails where to implement std::error::Error. Or by implementing std::error::Error for ErrorDetails::Str(...)

pitdicker · 2017-10-31T12:11:28Z

rand_core/src/lib.rs

+    /// Create a new instance, with a static string describing the details.
+    /// 
+    /// This may be used in both `std` and `no_std` enviroments.
+    pub fn new_str(kind: ErrorKind, details: &'static str) -> Self {


The name could be a little bit confusing. Maybe a more verbose new_with_cause_str?

Why the long name?

I agree with new_str being a bit confusing, but I personally can't come up with the better alternative, so I guess we can leave it up to docs.

pitdicker · 2017-10-31T12:29:58Z

rand_core/src/lib.rs

-        Error {
-            kind: kind,
-            cause: cause,
+    pub fn new_err<E>(kind: ErrorKind, cause: E) -> Self


Does it make sense to have a function that takes both a kind and a string, and use one of them depending on whether the std feature is enabled? Now implementations of Rng need a std feature flag to give the right type of cause.

I don't get what you mean? This is more generic than new_str and can still accept a string but is only available with the std library (we could probably depend on alloc instead, but that's another matter; we need Box).

See this comment.

The Into where clause it neat, wouldn't have thought about that myself. Although maybe it is common, and I am just missing something :-).

I was thinking about cases like this, where a Rng implementation needs to use different functions to construct an error depending on whether the std feature is available.

Then a helper function is maybe useful:

The `Into` where clause it neat, wouldn't have thought about that myself. Although maybe it is common, and I am just missing something :-). ``` rust #[cfg(feature="std")] pub fn new_with_cause(kind: ErrorKind, cause: E, details: &'static str) -> Self { new_str(kind, details) } #[cfg(not(feature="std"))] pub fn new_with_cause(kind: ErrorKind, cause: E, details: &'static str) -> Self { new_err(kind, cause) }

In no_std environment you simply can't have implementation of Error trait, as it's defined only in the std. So either way crate authors will have to write cfgs to handle switching.

Or authors supporting no_std can simply use new_str with a string description everywhere. There's no advantage to using new_err if the details are just a string.

I updated my branch with JitterRng to the new error handling pitdicker@b223c60.

The second commit shows what I mean pitdicker@f6b3531.
In theory an RNG implementation can support both std and no_std compatible error causes, and the choice is made in rand_core.

It was just an idea, but I don't think it helps in enough cases. Lets leave the two functions as is.

Your new_with_cause isn't even going to compile in no_std mode since it depends on std::error::Error.

I see what you mean, but I'm not convinced it's worth returning a chained Error type in this case; there is no added information (but maybe your description() strings should include all the details from the Display impl).

Oops. Ok, please forget I said anything :-)

newpavlov · 2017-10-31T17:26:33Z

rand_core/src/lib.rs

+    /// 
+    /// In the case of a chained error, the actual error may be obtained via
+    /// `std::error::Error::cause`.
+    pub fn details(&self) -> Option<&str> {


Maybe "will be obtained"? Also not description, but cause.

Um, actually I think my description is correct, but I'll try to make it clearer.

I meant in reverse. :) In the code description() method is used, but in the docstring you write about cause.

This is the details() function. Anyway, the terms are a bit confused, although description should be distinct from details/cause

newpavlov · 2017-10-31T17:29:14Z

rand_core/src/lib.rs

-            ErrorKind::Other => write!(f, "An unspecified RNG error occurred."),
-            ErrorKind::__Nonexhaustive => unreachable!(),
-        }
+        write!(f, "RNG {}", self.kind.description())


I think "RNG Error: {}" will be better.

newpavlov · 2017-10-31T17:34:16Z

rand_core/src/lib.rs

+impl ::std::error::Error for ErrorDetails {
+    fn description(&self) -> &str {
+        match *self {
+            ErrorDetails::None => "NA", // case shouldn't be used


It's better to write unreachable!() here if this case indeed can not be used anywhere.

dhardy · 2017-10-31T17:57:47Z

Ok, I addressed some of your points.

I don't see much point renaming the new_* functions. As it is they are descriptive of the type of the details (str or something coercible to an Error). We could renamed to new_with_*, but that's longer for little extra clarity. We could use names like new_cause (or new_chained) and new_msg instead, but I'm not sure that's any clearer.

pitdicker · 2017-11-01T06:28:37Z

rand_core/src/lib.rs

-            ErrorKind::Other => write!(f, "An unspecified RNG error occurred."),
-            ErrorKind::__Nonexhaustive => unreachable!(),
-        }
+        write!(f, "RNG error: {}", self.kind.description())


According to the book description is supposed to be a short description, and Display a user-facing representation of the error (a proper sentence with capital letter, dot at the end, and a bit less condensed text).

I think this is okay, but like the previous version more.

Does it? I skimmed through and saw an example similar to this, but in general the section is long and a little vague.

WARNING: this is a change in policy for ReseedingRng

See #10 (comment)

Fixes #26

Common error details, as @newpavlov's design

0674202

pitdicker reviewed Oct 31, 2017

View reviewed changes

Impl Error for ErrorDetails; add simple tests

87b0fba

newpavlov reviewed Oct 31, 2017

View reviewed changes

Address some minor review points

1196513

pitdicker reviewed Nov 1, 2017

View reviewed changes

pitdicker mentioned this pull request Nov 1, 2017

Add error handling to ReseedingRng #32

Closed

dhardy mentioned this pull request Nov 1, 2017

Error handling: Error type #10

Closed

dhardy added 7 commits November 2, 2017 08:59

Reseeding: tweak SeedableRng test

b8bac95

Add ErrorKind::should_retry and should_wait functions

4a6f1bf

Handle reseeding errors: retry, delay, don't reseed, forward error

dbbd64d

WARNING: this is a change in policy for ReseedingRng

Revise Error type; optional *extra* cause, no ErrorDetails type

8fb1e05

See #10 (comment)

Tidy up OS error handling.

37f9629

Fixes #26

Improve Error documentation

e624c4e

Improve doc and message

d30318b

This was referenced Nov 8, 2017

Revise error type (copy from error_details branch) #34

Merged

Reseeding: handle/forward failures from source RNG #35

Merged

dhardy closed this Nov 8, 2017

dhardy deleted the error_details branch November 19, 2017 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common error details, as @newpavlov's design #30

Common error details, as @newpavlov's design #30

dhardy commented Oct 31, 2017 •

edited

Loading

pitdicker left a comment •

edited

Loading

pitdicker Oct 31, 2017

pitdicker Oct 31, 2017

pitdicker Oct 31, 2017

dhardy Oct 31, 2017

newpavlov Oct 31, 2017

pitdicker Oct 31, 2017

dhardy Oct 31, 2017

dhardy Oct 31, 2017

pitdicker Oct 31, 2017

newpavlov Oct 31, 2017 •

edited

Loading

dhardy Oct 31, 2017

pitdicker Nov 1, 2017

pitdicker Nov 1, 2017

dhardy Nov 1, 2017

pitdicker Nov 1, 2017

newpavlov Oct 31, 2017 •

edited

Loading

dhardy Oct 31, 2017

newpavlov Oct 31, 2017

dhardy Oct 31, 2017

newpavlov Oct 31, 2017

newpavlov Oct 31, 2017

dhardy commented Oct 31, 2017

pitdicker Nov 1, 2017

dhardy Nov 1, 2017

Common error details, as @newpavlov's design #30

Common error details, as @newpavlov's design #30

Conversation

dhardy commented Oct 31, 2017 • edited Loading

pitdicker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Oct 31, 2017 •

edited

Loading

pitdicker left a comment •

edited

Loading

newpavlov Oct 31, 2017 •

edited

Loading

newpavlov Oct 31, 2017 •

edited

Loading