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

Support passing both borrowed and owned Rngs #219

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Dec 26, 2018

Rng was designed to support both &mut and owned usage like this.

It's slightly odd to do this on a method since Borrow exists for that, but this helps a lot with data types so one may as well use it.

@hdevalence hdevalence changed the base branch from master to develop December 27, 2018 04:34
@hdevalence hdevalence self-requested a review December 27, 2018 04:34
@hdevalence
Copy link
Contributor

Neat. I'm not sure if we can make this change, since it alters the type parameters on public API which is covered by a stability promise.

@burdges
Copy link
Contributor Author

burdges commented Dec 27, 2018

I think this cannot break any existing code since impl<'a,R: Rng> Rng for &'a mut R exists as part of rand's public API. I've no idea how this might interact with some future edition that revamped the borrow, etc. traits or whatever though.

@hdevalence hdevalence added this to the 1.1 milestone Dec 29, 2018
@newpavlov
Copy link
Contributor

newpavlov commented Jan 5, 2019

it alters the type parameters on public API which is covered by a stability promise

IIUC you have published only a pre-release 1.0, so changing API at this stage will not count as breaking stability promise.

UPD: Ah, you've already published 1.0 for this crate, pre-release is published only for ed25519.

@hdevalence hdevalence merged commit 20fd561 into dalek-cryptography:develop Feb 14, 2019
@hdevalence
Copy link
Contributor

Thanks! This should be included in 1.1.0-pre.0, published as a prerelease to test that generalizing the trait bound doesn't cause problems.

hdevalence added a commit that referenced this pull request Feb 15, 2019
See discussion at #232 , copied below:

`1.1` changed the trait bounds for `RistrettoPoint::random` and `Scalar::random`, see #222 and #219.

These changes have two benefits:
* they unlink us from the `rand` crate and make us depend only on `rand_core`;
* they allow passing both owned and borrowed RNGs.

The change was not supposed to be a breaking change, since the new bounds are strictly more general than the old ones (as every `RngCore` is an `Rng` and every `&mut RngCore` is an `RngCore`), so the new bound is satisfied in every situation where the old bound applied.

The `1.1.0-pre.0` version didn't cause problems on the crates I tested it on, but there was an unexpected problem: https://github.com/interstellar/slingshot/blob/ce71c93a9a29ac3b4f69ce71feb987bd64d6c4ec/spacesuit/src/value.rs#L160-L161 broke, since it took a borrow as input and used it twice. So there was slight breakage.

One option is to revert the changes (probably just the ones from #219) and release 1.1.3; another would be to fix up `slingshot` and leave the new bound.
@oleganza
Copy link
Contributor

I think this cannot break any existing code since impl<'a,R: Rng> Rng for &'a mut R exists as part of rand's public API.

Looks like it did break code that was taking in &mut R and passing it without double-borrowing into two Scalar::random() calls.

hdevalence added a commit that referenced this pull request Feb 15, 2019
@burdges
Copy link
Contributor Author

burdges commented Feb 15, 2019

Interesting. It appears Distribution::sample also takes a &mut R not an R: Rng, so the rand crate itself might occasionally trigger similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants