Remove weak_rng #20773

nagisa · 2015-01-08T21:36:34Z

Documentation for the function claims it will return the fastest algorithm
available in Rust, but since its return type is hardcoded to XorShiftRng, it
would be an impossible [breaking-change], when, or if, a faster Rng is added to
Rust.

[breaking-change], since this removes public API.

Fixes #20484

r? @huonw for a rollup post 1.0α.

sfackler · 2015-01-08T22:10:59Z

I'd personally prefer an alternative where we keep weak_rng, but encapsulate the return type:

pub fn weak_rng() -> WeakRng { ... }

pub struct WeakRng(XorShiftRng);

impl Rng for WeakRng { ... }

nagisa · 2015-01-08T22:12:48Z

That’s what I originally suggested. I’m fine either way, but as always, feedback is scarce without PRs 😉

huonw · 2015-01-08T22:22:41Z

@sfackler We can always add that in later, I'm not really sure how much sense it makes to have a plethora different RNG types, several of which are just renamed/slightly tweaked versions of others. I'd like the module to feel less... sprawling.

It seems like this would be better suited to an external library which can then choose the internal WeakRng type based on platform specific performance properties (or something).

huonw · 2015-01-08T22:23:43Z

(That said, I'm very willing to be convince otherwise, so won't approve for now. Thanks for the PR, @nagisa.)

huonw · 2015-01-08T22:24:14Z

src/libstd/collections/hash/map.rs

@@ -1585,7 +1585,7 @@ mod test_map {
    use super::Entry::{Occupied, Vacant};
    use iter::{range_inclusive, range_step_inclusive, repeat};
    use cell::RefCell;
-    use rand::{weak_rng, Rng};


It may be better to change these to XorShiftRng.

As these are used for tests and XorShiftRng needs to be seeded manually now, I think preserved noise-to-signal ratio is much more valuable than those few µs we’d save by not using thread_rng.

strconv benchmarks are another story, though. Will update these tomorrow.

You can get a randomly seeded one with rand::random::<XorShiftRng>(), btw.

nagisa · 2015-01-08T22:35:33Z

Ah indeed, rand::random::<XorShiftRng>() doing the (almost) same thing as weak_rng is another good reason to remove the function.

sfackler · 2015-01-08T22:41:26Z

So I'm of the opinion that we want functions like weak_rng in the root std::rand module and less of things like ChaCha20. If I'm a random person that needs an RNG, I either want a fast RNG and don't care much about the security of it, or need a secure source of randomness for crypto or whatever. If I see weak_rng and crypto_rng functions (or whatever we want to call them), I'm happy. If I have to go hunting around to see what the hell ChaCha20, Isaac64, and XorShiftRng are and what the differences are, I'm going to be wasting time, and there's a decent chance I'm going to screw up and use an insecure RNG for something that I need a secure RNG for or use some slow and secure RNG to make dice rolls for my video game. This is kind of in the vein of bascule's talk about crypto libraries at the last meetup.

huonw · 2015-01-08T23:34:46Z

We have the "obvious" name StdRng as a default choice, and the thread RNG (which most people use) is also designed to be somewhat secure, including reseeding itself from the OS regularly. (Of course, OsRng is still the thing to use for applications that actually need security.)

I 100% agree we don't need all of those types. I'm actually thinking that we switch to ChaChaRng for StdRng and move the ISAAC RNGs to an external library, leaving us with just StdRng, ChaChaRng and XorShiftRng (and OsRng). I guess WeakRng may fit into this some how. I suppose we could do some straight-out renaming, like ChaChaRng to StdRng and/or XorShiftRng to WeakRng, without having duplication.

That said, there's going to be a close analysis of std::rand in future, before 1.0, for stabilisation; which I think is the right place to consider these sorts of things. I feel the move away from free functions brings rand more closely into line with the other parts of std, and we can consider the "do crypto right" aspect later.

(BTW, @nagisa, could you amend the commit message to include the suggested migration path, weak_rng() → random::<XorShiftRng>()? It's helpful for people reading the breaking change log.)

chris-morgan · 2015-01-13T05:42:34Z

src/libcollections/bench.rs

@@ -22,7 +22,7 @@ pub fn insert_rand_n<M, I, R>(n: uint,
    R: FnMut(&mut M, uint),
 {
    // setup
-    let mut rng = rand::weak_rng();
+    let mut rng : XorShiftRng = rand::random();


Standard code style requests that there be no space before the colon.

alexcrichton · 2015-01-20T16:38:37Z

re-r? @huonw

Documentation for the function claims it will return the fastest algorithm available in Rust, but since its return type is hardcoded to XorShiftRng, it would be an impossible [breaking-change], when, or if, a faster Rng is added to Rust. [breaking-change], since this removes public API. Uses of `weak_rng` function can be replaced with `random::<XorShiftRng>()` to achieve almost the same behaviour. Fixes rust-lang#20484

steveklabnik · 2015-01-31T08:49:27Z

This needs a rebase.

nagisa · 2015-01-31T10:47:59Z

I will rebase after the relevant RFC gets accepted because it conflicts often.

huonw · 2015-01-31T23:34:53Z

(The RFC was closed in favour of moving std::rand out of the standard library, this PR will make sense against the external crate.)

rust-highfive assigned huonw Jan 8, 2015

huonw reviewed Jan 8, 2015
View reviewed changes

nagisa force-pushed the remove-weak-rng branch from a0aded2 to 90f64b1 Compare January 9, 2015 11:54

chris-morgan reviewed Jan 13, 2015
View reviewed changes

nagisa force-pushed the remove-weak-rng branch from 90f64b1 to 118fb92 Compare January 13, 2015 13:23

nagisa force-pushed the remove-weak-rng branch from 118fb92 to c1b98aa Compare January 24, 2015 18:05

nagisa closed this Jan 31, 2015

nagisa mentioned this pull request Feb 3, 2015

Remove weak_rng rust-random/rand#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove weak_rng #20773

Remove weak_rng #20773

nagisa commented Jan 8, 2015

sfackler commented Jan 8, 2015

nagisa commented Jan 8, 2015

huonw commented Jan 8, 2015

huonw commented Jan 8, 2015

huonw Jan 8, 2015

nagisa Jan 8, 2015

huonw Jan 8, 2015

nagisa commented Jan 8, 2015

sfackler commented Jan 8, 2015

huonw commented Jan 8, 2015

chris-morgan Jan 13, 2015

nagisa Jan 13, 2015

alexcrichton commented Jan 20, 2015

steveklabnik commented Jan 31, 2015

nagisa commented Jan 31, 2015

huonw commented Jan 31, 2015

Remove weak_rng #20773

Remove weak_rng #20773

Conversation

nagisa commented Jan 8, 2015

sfackler commented Jan 8, 2015

nagisa commented Jan 8, 2015

huonw commented Jan 8, 2015

huonw commented Jan 8, 2015

huonw Jan 8, 2015

Choose a reason for hiding this comment

nagisa Jan 8, 2015

Choose a reason for hiding this comment

huonw Jan 8, 2015

Choose a reason for hiding this comment

nagisa commented Jan 8, 2015

sfackler commented Jan 8, 2015

huonw commented Jan 8, 2015

chris-morgan Jan 13, 2015

Choose a reason for hiding this comment

nagisa Jan 13, 2015

Choose a reason for hiding this comment

alexcrichton commented Jan 20, 2015

steveklabnik commented Jan 31, 2015

nagisa commented Jan 31, 2015

huonw commented Jan 31, 2015