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

Make PRNGs implement Clone instead of Copy #20199

Closed
wants to merge 1 commit into from

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Dec 24, 2014

Remove derivations of Copy trait from ChaChaRng, IsaacRng, Isaac64Rng
and StdRng and implement the Clone trait instead.

Copying is an implicit operation and the copy shares its internal state with
the original generator. This means the the two generators will yield exactly
the same sequence of values. This behaviour is considerably easy to trigger
by, for example, passing the generator by value to a function.

Clone trait, on the other hand, does no implicit copying. The user will only
be able to either move the generator or ask for a copy explicitly, via the
clone method.

The only downside to this patch is the fact that the implementations of
Clone::clone methods do not optimise down to a single memcpy, like the
derived Copy implementations did.

Although the Copy implementations of these random number generators are
suspected to not be used much, this is a [breaking-change].

Fixes #20170.

Remove derivations of `Copy` trait from `ChaChaRng`, `IsaacRng`, `Isaac64Rng`
and `StdRng` and implement the `Clone` trait instead.

Copying is an implicit operation and the copy shares its internal state with
the original generator. This means the the two generators will yield exactly
the same sequence of values. This behaviour is considerably easy to trigger
by, for example, passing the generator by value to a function.

`Clone` trait, on the other hand, does no implicit copying. The user will only
be able to either move the generator or ask for a copy explicitly, via the
`clone` method.

The only downside to this patch is the fact that the implementations of
`Clone::clone` methods do not optimise down to a single memcpy, like the
derived `Copy` implementations did.

Although the `Copy` implementations of these random number generators are
suspected to not be used much, this is a [breaking-change].

Fixes rust-lang#20170.
@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@thestinger
Copy link
Contributor

Copy is used in places like Cell as a generic bound. If it's removed, useful functionality will be lost. I don't think opt-in copy should be used to remove Copy implementations based on very subjective coding style ideas. The same copying semantics will be there for clone but without fulfilling some generic bounds.

@emberian
Copy link
Member

emberian commented Jan 2, 2015

I agree with @thestinger here. It is somewhat of a footgun if you accidentally copy your RNG, but Copy has other very useful properties as a generic bound.

@nagisa
Copy link
Member Author

nagisa commented Jan 2, 2015

It’s a fair point and, honestly, I can’t think of counter-argument to that…

@nagisa nagisa closed this Jan 2, 2015
@huonw
Copy link
Member

huonw commented Jan 2, 2015

I'm not sure how much sense it makes to use most RNGs with Cell, since they're often large so the overhead of Cell/the memcpys associated with Copy could easily be larger than that of RefCell. Not implementing Copy is certainly the most backward-compatible thing to do.

In any case, it would be good to have these structs implement Clone, a PR that adds that functionality (without removing Copy) would be accepted.

(It might be good to have some tests that check that let mut rng = ...; let mut rng2 = rng.clone(); give the same sequence.)

@aturon
Copy link
Member

aturon commented Jan 7, 2015

i likewise agree with @huonw that not implementing Copy is the most conservative choice, and in some cases may help avoid footguns; and on the other hand there are very few places where Copy bounds are required -- and Copy can always be reintroduced later if those use cases turn out to outweigh other concerns.

@ranma42 ranma42 mentioned this pull request Jan 10, 2015
nagisa added a commit to nagisa/rand that referenced this pull request Feb 8, 2015
Copying is an implicit operation and the copy shares its internal state with
the original generator. This means the the two generators will yield exactly
the same sequence of values. This behaviour is considerably easy to trigger
by, for example, passing the generator by value to a function.

See:
* http://internals.rust-lang.org/t/copy-impl-for-random-number-generators/1218
* rust-lang/rust#20199

[breaking-change]. Use `clone` method implemented on the relevant RNGs instead.
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.

Random number generators should not implement Copy
7 participants