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

Associated error type for RngCore #307

Closed
wants to merge 4 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 16, 2018

Note: PR is built on top of #306 code.

Interesting idea, but one major problem comes up: implementation-agnostic dynamic dispatch is impossible, i.e. no full type erasure:

// we are required to specify the Error type here:
let mut r = &mut rng as &mut RngCore<Error=Void>;

@burdges I don't think this is avoidable?

Possibly we could try something like this I suppose:

impl<R: RngCore> RngCore<Error=Error> for R
where Error: From<<R as RngCore>::Error> {...}

@dhardy
Copy link
Member Author

dhardy commented Mar 16, 2018

No, we can't use that impl, because it would require R to implement RngCore multiple times.

We may instead be able to use type parameters, but I'm not sure I want to go there:

impl<E, R: RngCore<E>> RngCore<Error> for R where Error: From<E> {...}

@burdges
Copy link
Contributor

burdges commented Mar 16, 2018

It might be because &mut RngCore<Error=Void> is a trait object, which may require all types to be known sooner. It possible monomorphisation can infer more if you avoid the trait object. It appears you only use the trait object in tests though, so maybe no big deal.

@dhardy
Copy link
Member Author

dhardy commented Mar 16, 2018

It would be nice if it is still possible for users to write a function like this, though maybe the idea is overrated, I don't know:

// accepts any RNG type
fn foo(rng: &mut RngCore);

@burdges
Copy link
Contributor

burdges commented Mar 16, 2018

We encountered this issue with associated error types not being so ergonomic for trait objects before I suppose. And it likely contributed to the "just panic" decision before.

@dhardy dhardy added E-question Participation: opinions wanted B-API Breakage: API F-new-int Functionality: new, within Rand P-medium X-experimental Type: experimental change labels Mar 17, 2018
@dhardy
Copy link
Member Author

dhardy commented Mar 20, 2018

Okay, I'm going to close it. It isn't like there wasn't already a huge amount of thought put into the design of RngCore. I do wonder whether specialisation or negative type bounds would solve the problems with this design, but lets not wait forever on Rustc features.

@dhardy dhardy closed this Mar 20, 2018
@dhardy dhardy mentioned this pull request Apr 6, 2018
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 F-new-int Functionality: new, within Rand X-experimental Type: experimental change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants