clone() does not behave as expected

[ra1u]

This fails, but I think it should not.

let mut rng_tx = fastrand::Rng::new();
let mut rng_rx = rng_tx.clone();
assert!(rng_tx.get_seed() == rng_rx.get_seed());

[taiki-e]

How does rand handle this case? It would be preferable to provide the same behavior as they do.

[ra1u]

It is auto derived. So it basically works by cloning internal state.
https://docs.rs/rand/latest/src/rand/rngs/small.rs.html#80
Or basically
https://docs.rs/rand_xoshiro/latest/src/rand_xoshiro/xoshiro256plusplus.rs.html#22

[taiki-e]

Hmm, looking at the PR that added Clone impl (#4 by @cloudhead), it seems that the current behavior is intentional?

[cloudhead]

It was intentional yes, as pointed out by the docs. Perhaps the clone function should be renamed to fork, and clone should be a regular clone.

[ra1u]

I am not sure that was intentional. For example current pull request 3e11c10 still passes same doctest as it was defined in https://github.com/smol-rs/fastrand/blob/master/src/lib.rs#L101-L113 and covers clone() related properties.

[cloudhead]

See the first line of the PR: Useful for "splitting" the RNG when passing it down to sub-components/functions.

By "splitting" it's meant that we generate a new RNG that isn't equal to the previous one. The documentation also explains that we derive a generator from the previous one. The test is also carefully chosen not to compare the derived generator with the original generator.

The problem is really the name chosen: "clone", so I'd propose we implement actual cloning under the clone function, and move the current clone functionality to a new function called "fork".

[ra1u]

I see.
Related to split: I think that split Rng should not be correlated with sourced Rng and implementation of split()/fork() should address that.

[ra1u]

Let me bump this with suggested improvement to fork()

Once rng is forked it is expected that output of each generator is uncorrelated with each other. For example if one splits generator for use in multiple threads, each generator should generate different numbers.

I suggest we have public function that makes forking clear.

pub fn fork(self) -> (Self,Self)

And implementation, for example

pub fn fork(self) -> (Self, Self) {
    let rand_a: u64 = 0xc0d43516ee92d71f;
    let rand_b: u64 = 0x91b6dcec5810da71;
    (
        Self::with_seed(self.gen_u64() ^ rand_a),
        Self::with_seed(self.gen_u64() ^ rand_b),
    )
}

[cloudhead]

That is creating two forks though. It's a bit impractical as an API because you move self, and you return a tuple, so you can't just do eg.:

let base = Rng::new();
thread1(base.fork());
thread2(base.fork());
thread3(base.fork());

[ra1u]

I see your point @cloudhead . It less ergonomic compared to moving tuple around.

I guess we have two options. What is your opinion of

fn fork(&self) -> Self

vs

fn fork(&mut self) -> Self

[cloudhead]

Hmm, so I would opt for fork(&self) simply because the other functions like i32(..), u8(..) etc. all take a &self, and this function is similar in a way: it is producing a random value which happens to be another Rng:

Rng::alphabetic(&self) -> char
Rng::bool(&self) -> bool
Rng::fork(&self) -> Rng

You could almost call the function Rng::rng in that sense, though I think fork is more elegant.

[notgull]

My personal vote is for fork(&self).

[Bluefinger]

I went with fork for my turborand library after initially copying fastrand API regarding cloning, so to me it makes sense.

[ra1u]

There is PR available and should include features and interface as discussed here. #38
Let me know if I missed something and how we can push this forward.

[fogti]

fork(&mut self) would now make more sense, given we removed interior mutability.

[ra1u]

Related to PR #49. Is it bug or feature that forked rng is correlated with sourced rng?

[notgull]

Is it bug or feature that forked rng is correlated with sourced rng?

Could you please clarify?

[ra1u]

Could you please clarify?

I did, but it seems that my reasoning was incorrect.
Current implementation of fork() looks fine.