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

Blockrng: add associated error type #306

Closed
wants to merge 3 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 16, 2018

Another thing suggested by @burdges

@pitdicker this uses the early return on inner RNG errors you didn't like, but it's only implemented if the internal RNG can actually produce an error.

This does not return any errors from reseeding. I don't really know which is better, Transient errors or no errors.

Unfortunately I can't get generate(..)? syntax to work since:

error[E0277]: the trait bound `error::Error: std::convert::From<<R as BlockRngCore>::Error>` is not satisfied
   --> rand-core/src/impls.rs:285:13
    |
285 |             self.core.generate(dest_u32)?;
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<<R as BlockRngCore>::Error>` is not implemented for `error::Error`
    |
    = help: consider adding a `where error::Error: std::convert::From<<R as BlockRngCore>::Error>` bound
    = note: required by `std::convert::From::from`

I tried adding a where bound on the associated Error type; strangely this is allowed by the compiler but doesn't seem to help.

Note: maybe we should investigate what @burdges suggested a long time ago, associated error types on RngCore.

@dhardy dhardy added X-enhancement P-high Priority: high D-review Do: needs review labels Mar 16, 2018
@pitdicker
Copy link
Contributor

I think it is an ingenious solution. But aren't we over-designing here? I still have trouble imagining when a PRNG can error.

@dhardy
Copy link
Member Author

dhardy commented Mar 16, 2018

Perhaps you are correct. I was intending to use the error type in ReseedingRng to return reseeding errors, but decided against doing that, which means normally the error type will be Void.

@burdges
Copy link
Contributor

burdges commented Mar 16, 2018

I've actually forgotten our long discussions on PRNGs throwing errors, but maybe we concluded they should never do so if you cannot seek around the stream, thus making panicing okay. I brought it up here just because BlockRng seems so low level.

@dhardy
Copy link
Member Author

dhardy commented Mar 16, 2018

If we do decide to merge #307 then I think we should definitely merge this, otherwise I'm not really sure.

I'm also not quite sure about adding the Void error type or why std's From conversion doesn't appear to work.

@pitdicker
Copy link
Contributor

I know an opinion without arguments is not worth much, but I am all for the somewhat simpler system we have now, and don't see much advantage in making Error an associated type.

Just to make sure, what are the advantages? I can think of only one: that you can spell out through the type system that some RNGs can never error.

@dhardy dhardy added E-question Participation: opinions wanted X-experimental Type: experimental change B-API Breakage: API and removed D-review Do: needs review labels Mar 17, 2018
@dhardy
Copy link
Member Author

dhardy commented Mar 20, 2018

Without #307 this has less merit. I'm going to go with @pitdicker's suggestion that since most PRNGs are infallible anyway it adds extra complexity for very little expected benefit.

@dhardy dhardy closed this Mar 20, 2018
@dhardy dhardy deleted the blockrng branch March 23, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API E-question Participation: opinions wanted P-high Priority: high X-experimental Type: experimental change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants